Re: [PATCH net-next v15 12/15] quic: add crypto packet encryption and decryption

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 · Low: 2

- [Medium] quic_crypto_encrypt() records crypto->key_update_send_time =
  quic_ktime_get_us() *before* attempting to protect the packet…
- [Medium] The new packet protection paths
  (quic_crypto_encrypt()/quic_crypto_decrypt() ->…
- [Low] The comments on the new token helpers describe an earlier
  revision of the code and are now wrong in three ways.
- [Low] Reported concern: quic_crypto_generate_token() computes len =
  sizeof(flag) + addrlen + sizeof(u64) + conn_id->len + QUIC_TAG_LEN and…

--

quic: add crypto packet encryption and decryption

This patch adds packet-level AEAD encryption and decryption for QUIC,
including header protection and its removal, plus helpers for the Retry
integrity tag and for generating and verifying address validation tokens.

A few questions on the new code below.

[ ... ]

[ ... ]

+/* AEAD Usage. */
+static int quic_crypto_payload_protect(struct quic_crypto *crypto,
+				       struct sk_buff *skb, bool enc, gfp_t gfp)
+{

[ ... ]

+	aead_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+				  quic_crypto_done, skb);
+	*(struct quic_crypto **)ctx = crypto;
+	atomic_inc(&crypto->async_pending[phase]);
+	cb->crypto_ctx = ctx; /* Async free context for quic_crypto_done() */
+	err = enc ? crypto_aead_encrypt(req) : crypto_aead_decrypt(req);

[Severity: Medium]
This is the single place where every QUIC packet is protected or
unprotected, so it looks like the natural home for the AEAD usage limits
of rfc9001#section-6.6.  Are those limits enforced anywhere?

struct quic_crypto in net/quic/crypto.h keeps secrets, IVs,
async_pending[], the key update timestamps and the bit flags, but has no
field for the number of packets protected with a key, nor for the number
of packets that failed authentication, and a grep over net/quic at the end
of the series finds no confidentiality or integrity limit check.

On the RX side quic_crypto_decrypt() just returns the AEAD error, so a
peer can keep submitting forgery attempts against the same key without the
connection ever being closed, which rfc9001#section-6.6 requires once the
integrity limit is reached:

	err = quic_crypto_payload_protect(crypto, skb, false, gfp);
	if (err) {
		if (err == -EINPROGRESS)
			return err;
		...
		return err;
	}

TLS_CIPHER_AES_CCM_128 is supported here (quic_crypto_is_cipher_ccm() and
the "ccm(aes)" ciphers[] entry) and has the lowest integrity limit of the
QUIC AEADs.  Is the intent to add the per-key counters and the limit checks
in a later patch?

+	if (err == -EINPROGRESS || err == -EBUSY) {
+		memzero_explicit(nonce, sizeof(nonce));
+		return -EINPROGRESS;
+	}
+	atomic_dec(&crypto->async_pending[phase]);
+	cb->crypto_ctx = NULL;
+
+out:
+	kfree_sensitive(ctx);
+	memzero_explicit(nonce, sizeof(nonce));
+	return err;
+}
+
+/* Encrypts a QUIC packet before transmission.  This function performs AEAD
+ * encryption of the packet payload and applies header protection. It handles
+ * key phase tracking and key update timing.
+ *
+ * Return: 0 on success, or a negative error code.
+ */
+int quic_crypto_encrypt(struct quic_crypto *crypto, struct sk_buff *skb,
+			gfp_t gfp)
+{
+	struct quic_skb_cb *cb = QUIC_SKB_CB(skb);
+	int err;
+
+	/* Packet payload is already encrypted (e.g., resumed from async),
+	 * proceed to header protection only.
+	 */
+	if (cb->resume)
+		goto out;
+
+	cb->key_phase = crypto->key_phase;
+	/* If a key update is pending and this is the first packet using the
+	 * new key, save the current time. Later used to clear old keys after
+	 * some time has passed (see quic_crypto_decrypt()).
+	 */
+	if (crypto->key_pending && !crypto->key_update_send_time)
+		crypto->key_update_send_time = quic_ktime_get_us();
+
+	err = quic_crypto_payload_protect(crypto, skb, true, gfp);
+	if (err)
+		return err;

[Severity: Medium]
Should key_update_send_time be committed only after the packet has actually
been protected?

Here the timestamp is stored before quic_crypto_payload_protect() runs, and
neither this error return nor the header protection failure below restores
it.  If payload protection fails (skb_cow_data() returning -ENOMEM,
quic_crypto_aead_mem_alloc() returning -ENOMEM, an skb_to_sgvec() error, or
an AEAD error) the skb is dropped by the caller, but
crypto->key_update_send_time now records a time at which nothing was sent.

quic_crypto_decrypt() then uses that value as the start of the old key
retention window:

	if (crypto->key_pending && cb->key_phase == crypto->key_phase) {
		time = crypto->key_update_send_time;
		if (time &&
		    quic_ktime_get_us() - time >= crypto->key_update_time) {
			crypto->key_pending = 0;

Can the window therefore already be expired when the first packet in the
new phase is really received, so that key_pending is cleared immediately and
a later reordered old-phase packet is treated as a fresh key update
(phase != crypto->key_phase && !crypto->key_pending), overwriting the old
key slot before those packets can be decrypted?

+out:
+	return quic_crypto_header_protect(crypto, skb, true, gfp);
+}
+

[ ... ]

[ ... ]

+/* Derives a key and IV using HKDF, configures the AEAD transform and performs
+ * AEAD encryption/decryption for the provided token.
+ */
+static int quic_crypto_token_protect(struct quic_crypto *crypto, u8 *token,
+				     u32 len, u32 adlen, bool enc)
+{
+	/* Reuse TX AEAD (phase 1) in Initial crypto. */
+	struct crypto_aead *tfm = crypto->tx_tfm[1];

[Severity: Low]
Do these three comments still match the code after the v14 rework?

quic_crypto_token_protect() is documented as "Derives a key and IV using
HKDF, configures the AEAD transform", but the body has no HKDF call, no
crypto_aead_setkey() and no crypto_aead_setauthsize(); it only reuses the
key and IV that quic_crypto_set_token_secret() installed into
crypto->tx_tfm[1] and crypto->tx_iv[1]:

net/quic/crypto.c:quic_crypto_set_token_secret() {
	...
	err = crypto_aead_setauthsize(tfm, QUIC_TAG_LEN);
	if (err)
		goto out;
	err = crypto_aead_setkey(tfm, key, TLS_CIPHER_AES_GCM_128_KEY_SIZE);
	...
}

That also makes quic_crypto_set_token_secret() a hard prerequisite, since
crypto_aead_encrypt() returns -ENOKEY while CRYPTO_TFM_NEED_KEY is set.
Could the comment record that ordering requirement instead?

The quic_crypto_generate_token() comment says:

 * Encrypts the token (excluding the first flag byte) using AES-GCM with a key
 * and IV derived via HKDF.

but the call passes adlen = sizeof(flag) + addrlen + tslen, so the flag,
the client address and the timestamp are associated data, authenticated but
sent in the clear; only conn_id->len bytes of ODCID are encrypted.

The quic_crypto_verify_token() comment says:

 * Decrypts the token using derived key and IV. Checks that the decrypted
 * address matches the provided address, validates the embedded timestamp
 * against current time with a version-specific timeout.

The address compared by crypto_memneq() is never encrypted, and the timeout
comes from the token flag (QUIC_TOKEN_FLAG_REGULAR versus the
QUIC_TOKEN_TIMEOUT_RETRY default), not from crypto->version.

+	u32 extra = enc ? QUIC_TAG_LEN : 0, tslen;

[ ... ]

+/* Generate a token for Retry or address validation.
+ *
+ * Builds a token with the format: [flag][client address][timestamp][original
+ * DCID][auth tag]
+ *

[ ... ]

+int quic_crypto_generate_token(struct quic_crypto *crypto, void *addr,
+			       u32 addrlen, struct quic_conn_id *conn_id,
+			       u8 *token, u32 *tlen)
+{
+	u8 *token_buf, *p, flag = *token;
+	u64 ts = quic_ktime_get_us();
+	u32 len, tslen = sizeof(ts);
+	int err;
+
+	len = sizeof(flag) + addrlen + tslen + conn_id->len + QUIC_TAG_LEN;

[ ... ]

+	memcpy(token, token_buf, len);
+	*tlen = len;

[Severity: Low]
This isn't a bug today, but the interface of quic_crypto_generate_token()
carries no capacity for the token buffer.  len is computed from the
caller supplied addrlen and conn_id->len and then copied into token
without any bound, and *tlen is written only after the copy.

At the end of the series there is no caller at all
(quic_packet_retry_create_and_xmit() is still a stub returning
-EOPNOTSUPP), and the length is bounded in practice because conn_id->len
can only be set through quic_conn_id_update(), which rejects anything
larger than QUIC_CONN_ID_MAX_LEN:

net/quic/common.h:quic_conn_id_update() {
	if (WARN_ON_ONCE(len > QUIC_CONN_ID_MAX_LEN))
		return;
	memcpy(conn_id->data, data, len);
	conn_id->len = (u8)len;
}

Would it be worth passing the destination size in, so a future caller
cannot get this wrong?  The flag byte being read back out of token[0] as
an in/out use of the output buffer is also not mentioned in the comment.

+out:
+	kfree_sensitive(token_buf);
+	return err;
+}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789393775.git.lucien.xin%40gmail.com