Re: [PATCH net-next v16 11/15] quic: add crypto key derivation and installation

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] quic_crypto_keys_derive_and_install() (net/quic/crypto.c) sets
  `crypto->rx_fails[phase] = 0` every time it installs RX keys.
- [Low] Two comments in net/quic/crypto.c do not match the code.

--

quic: add crypto key derivation and installation

This patch adds the quic_crypto component. It derives and installs QUIC
packet protection keys for the Initial, Handshake, 0-RTT and 1-RTT
levels. Its helpers cover initial key installation, transform allocation,
traffic secret installation, key updates, and secret derivation for
stateless reset tokens and session resumption, and a new
quic_sock_destruct() frees the crypto state.

> diff --git a/net/quic/crypto.c b/net/quic/crypto.c
> new file mode 100644
> index 0000000000000..4bb6acea5112a
> --- /dev/null
> +++ b/net/quic/crypto.c
> @@ -0,0 +1,587 @@

[ ... ]

> +/* Derive and install reception (RX) or transmission (TX) packet protection
> + * keys for the current key phase.  This installs AEAD protection key, IV, and
> + * optionally header protection key.
> + */

[Severity: Low]
This isn't a bug, but this comment says the keys are installed "for the
current key phase". The function takes an explicit phase argument, and
quic_crypto_key_update() calls it with !phase, which is the next phase.

That is also the case where header protection key derivation is skipped:

	if (crypto->key_phase == phase)
		hp = quic_data(&hp_k, hp_key, keylen);

There is a similar mismatch in quic_crypto_set_cipher(). The comments say:

	/* Allocate AEAD and HP transform for each RX key phase. */
	...
	/* Allocate AEAD and HP transform for each TX key phase. */

But only one HP skcipher is allocated per direction (rx_hp_tfm and
tx_hp_tfm), and both phases share it. Only the AEAD tfms are per phase.

Could these comments be updated to match the code?

> +static int quic_crypto_keys_derive_and_install(struct quic_crypto *crypto,
> +					       bool rx, u8 phase)
> +{

[ ... ]

> +	if (rx) {
> +		crypto->rx_fails[phase] = 0;

[Severity: Medium]
Can resetting rx_fails[phase] here let the connection go past the AEAD
integrity limit?

RFC 9001 section 6.6 says endpoints must count packets that fail
authentication across all keys used in the connection. Once that total
goes over the integrity limit, the endpoint has to close the connection
with AEAD_LIMIT_REACHED.

Here the counter is split by key phase, and every RX key installation
zeroes it, including each key update:

quic_crypto_key_update()
    quic_crypto_keys_derive_and_install(crypto, true, !phase)
        crypto->rx_fails[phase] = 0;

crypto.h also describes rxlimit as "Max failures decrypted per key".

Later in the series, "quic: add crypto packet encryption and decryption"
checks the limit in quic_crypto_decrypt() like this:

	if (++crypto->rx_fails[cb->key_phase] >= crypto->cipher->rxlimit)
		err = -EKEYEXPIRED;

The later patches do not change how the counter works.

An off-path attacker who knows the connection ID could send forged
short-header packets until one phase counter is just below rxlimit. The
next legitimate key update would then reset that counter to zero.

With AES-CCM the limit is 2^21.5. A busy connection also has to update
its keys about every 2^21.5 sent packets, so the attacker's total
forgery budget grows with each key update. Splitting the count by phase
also allows about twice rxlimit per key epoch.

Should rx_fails be one connection-wide counter that key installation does
not reset?

[ ... ]

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