Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 2
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] In keyupdate_setup_seq() (tools/testing/selftests/net/tls.c),
a missing AF_ALG hash interface makes the test FAIL instead of SKIP.
- [Low] No new selftest checks the next-generation traffic secret, key or
IV that the kernel derives in tls_keyupdate_rekey()…
- [Low] In keyupdate_manual_rekey() (tools/testing/selftests/net/tls.c),
the comment says "a key installed by the user removes the registered…
--
Reviewing commit cf973cdf44894385edb313912c677758928546ac in this reply.
selftests/tls: add KeyUpdate tests
This adds kselftests for the in-kernel TLS 1.3 KeyUpdate. Each end of a
bidirectional connection registers both traffic secrets. The tests cover
registration errors, KeyUpdates in both directions, update_requested,
splice, the record limit, manual rekeys, the rate limit and invalid
KeyUpdate messages. The generation-0 keys are derived in userspace with
HKDF-Expand-Label, using the kernel HMAC through AF_ALG.
quoted hunk ↗ jump to hunk
diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index c4203401c4bfc..ed3b510a5ef6f 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -2421,6 +2422,525 @@ TEST_F(tls, rekey_poll_delay)
[ ... ]
quoted hunk ↗ jump to hunk
+static bool hkdf_expand_label(const unsigned char *secret, int secret_len,
+ const char *label, unsigned char *out,
+ int out_len)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ tfm = socket(AF_ALG, SOCK_SEQPACKET, 0);
+ if (tfm < 0)
+ return false;
+ if (bind(tfm, (struct sockaddr *)&sa, sizeof(sa)) ||
+ setsockopt(tfm, SOL_ALG, ALG_SET_KEY, secret, secret_len))
+ goto out;
[ ... ]
quoted hunk ↗ jump to hunk
+static bool keyupdate_setup_seq(struct __test_metadata *_metadata,
+ FIXTURE_DATA(tls) *self,
+ const FIXTURE_VARIANT(tls) *variant,
+ uint64_t seq)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ memset(secret_a, 0xa5, sizeof(secret_a));
+ memset(secret_b, 0x5a, sizeof(secret_b));
+
+ /* the keys must be the ones that the secrets give */
+ ASSERT_TRUE(keyupdate_keys(variant->cipher_type, secret_a, len,
+ &keys_a));
+ ASSERT_TRUE(keyupdate_keys(variant->cipher_type, secret_b, len,
+ &keys_b));
[Severity: Medium]
If the AF_ALG hash interface is missing, should these tests skip rather
than fail?
hkdf_expand_label() returns false if any of these fail:
- socket(AF_ALG, ...)
- bind() to "hmac(sha256)" or "hmac(sha384)"
- ALG_SET_KEY
- accept()
That happens on a kernel without CONFIG_CRYPTO_USER_API_HASH,
CONFIG_CRYPTO_HMAC or the SHA-256/SHA-512 crypto API drivers. The
ASSERT_TRUE() above then aborts every in-kernel KeyUpdate test as a
failure, for each TLS 1.3 variant. keyupdate_manual_rekey_request() has
the same ASSERT_TRUE(keyupdate_keys(...)).
The kernel feature does not need any of these options. net/tls/Kconfig
only has:
select CRYPTO_LIB_SHA256
select CRYPTO_LIB_SHA512
The additions to selftests/net/config only help runs built from that
fragment. Elsewhere, a missing test-only dependency will look like a
KeyUpdate regression.
Could this return false so the caller takes the existing "no in-kernel
KeyUpdate for this variant" skip, or SKIP() with its own message?
+
+ /* rec_seq is the last member of each crypto_info */
[ ... ]
quoted hunk ↗ jump to hunk
+ EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX_TRAFFIC_SECRET,
+ secret_a, len), 0);
+ EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_RX_TRAFFIC_SECRET,
+ secret_b, len), 0);
+ EXPECT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_TX_TRAFFIC_SECRET,
+ secret_b, len), 0);
+ EXPECT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_RX_TRAFFIC_SECRET,
+ secret_a, len), 0);
[Severity: Low]
This isn't a bug, but does any test check the next-generation secret,
key and IV against an independent calculation?
Each test that uses keyupdate_setup() or keyupdate_setup_seq() registers
both secrets on both fd and cfd. The sender and the receiver therefore
run the same kernel code on the same secret:
sender: tls_keyupdate_rekey(sk, 1)
receiver: tls_keyupdate_rekey(sk, 0)
net/tls/tls_keyupdate.c:tls_keyupdate_rekey() {
...
rc = tls_hkdf_expand_label(secret->data, secret->len, "traffic upd",
next, secret->len);
...
memcpy(secret->data, next, secret->len);
...
}
Both ends would agree on the same wrong key after any of these mistakes,
and every test would still pass:
- a wrong label
- a wrong output length
- a wrong hash
- swapped salt and IV
- the secret never being replaced
keyupdate_keys() only derives the generation-0 key. The kernel already
checks that key at registration in tls_check_traffic_secret().
One option is to compute HKDF-Expand-Label(secret_a, "traffic upd", ...)
here, pass it to keyupdate_keys(), and compare the result with
getsockopt(TLS_TX) after a KeyUpdate. Another is to register only the TX
secret on fd and install the independently derived next key on cfd with
TLS_RX.
[ ... ]
quoted hunk ↗ jump to hunk
+TEST_F(tls, keyupdate_manual_rekey)
+{
+ struct tls_crypto_info_keys tls12;
+ int zero = 0;
+
+ if (!keyupdate_setup(_metadata, self, variant))
+ SKIP(return, "no in-kernel KeyUpdate for this variant");
+
+ /* a key installed by the user removes the registered secrets */
+ tls_crypto_info_init(variant->tls_version, variant->cipher_type,
+ &tls12, 2);
+ EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12, tls12.len), 0);
+
+ EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX_KEY_UPDATE,
+ &zero, sizeof(zero)), -1);
+ EXPECT_EQ(errno, EINVAL);
+}
[Severity: Low]
This isn't a bug, but the comment says "secrets", and the test only
shows that the TX secret is gone.
The EINVAL comes from the first check in tls_sw_key_update():
if (!ctx->tx_secret.len)
return -EINVAL;
On a TLS_TX update, do_tls_setsockopt_conf() also clears the RX secret
and tx_key_update_asked:
memzero_explicit(&ctx->rx_secret, sizeof(ctx->rx_secret));
/* the answer now goes to user space */
ctx->tx_key_update_asked = false;
Nothing here would catch a change that left rx_secret set. The kernel
would then keep consuming the peer's KeyUpdates silently, even though fd
can no longer answer them.
Could the test also check that a KeyUpdate sent from cfd now reaches
user space on fd?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105-keyupdate-rfc-v6-0-99b695f9eb4f%40gmail.com