Re: Question regarding TLS Key update for NVMe-TCP protocol

[Sabrina Dubroca <sd@queasysnail.net>]

2024-12-06, 13:09:23 +0100, Hannes Reinecke wrote:
> On 12/6/24 12:26, Sabrina Dubroca wrote:
> > 2024-12-03, 16:20:12 +0100, Hannes Reinecke wrote:
> > > On 12/3/24 15:33, Chuck Lever III wrote:
> > > > > On Dec 3, 2024, at 2:17 AM, Hannes Reinecke <hare@suse.de> wrote:
> > > > > But the current in-kernel TLS implementation doesn't allow to change
> > > > > the cipher ( cf net/tls/tls_main.c: do_tls_setsockopt_conf() ), so
> > > > > the only chance here is to close the connection and start from scratch.
> > > > 
> > > > Have we asked Boris and friends if it would be possible to
> > > > implement KeyUpdate via ktls? Just curious.
> > > > 
> > > The immediate problem with KeyUpdate is here:
> > > 
> > > net/tls/tls_main.c:tls_setsockopt_conf()
> > > 
> > >          /* Currently we don't support set crypto info more than one time */
> > >          if (TLS_CRYPTO_INFO_READY(crypto_info))
> > >                  return -EBUSY;
> > 
> > And my patches (mentioned in the first email of this thread, but
> > there's another version that I sent for review more recently [1])
> > address that.
> > 
> > [1] https://lore.kernel.org/netdev/cover.1731597571.git.sd@queasysnail.net/
> > 
> Ah-ha.
> So that worry is solved. Great work!
> 
> > > so we need to close the socket whenever we want to change the TLS key.
> > > And that results in a call to the socket callbacks, which informs the
> > > NVMe-TCP driver, and the connect is reset.
> > > 
> > > KeyUpdate is a single message in the packet stream, transferring
> > > the new key values encrypted with the original key.
> > 
> > nit: the KeyUpdate message doesn't contain the new key, it just says
> > that the new key will be used from that point on (otherwise we
> > wouldn't have to worry about keeping secrets around?).
> > 
> Yeah ... but that doesn't impact this issue, no?

At least on RX we wouldn't have to worry about recomputing a key, it
would be provided. On TX we'd still have to generate the new key
either way.

> > > But: The new key is derived from the application secret, which is _not_ the
> > > currently used Key/IV values used by the in-kernel TLS implementation (cf
> > > section 7.3 'Traffic Key Calculation' in RFC 8446).
> > > So we cannot compute the next set of application traffic secrets; the only
> > > one who might have had this is the tlshd session, but that is destroyed once
> > > the handshake is done (or, rather, I hope it is ...).
> > > 
> > > So the original handshake traffic secrets are gone, and we cannot
> > > derive the next set of traffic secrets from there. Plus I'm not sure
> > > if we should keep that around in tlshd; that definitely would be prone
> > > to leak secret material, inviting $RANDOM_HACKER to attack tlshd ...
> > > 
> > > And that's before even checking whether we need to modify gnutls
> > > for deriving updated traffic secrets ...
> > 
> > I would expect all TLS libraries keep those secrets around to be able
> > to implement KeyUpdate regardless of ktls (but I only work on the
> > kernel, not gnutls/openssl).
> > 
> They will keep secrets around as long as the program using these libraries
> is running. So yeah, technically tlshd would be alive, and gnutls should
> keep the secrets somehow.
> There is a resolved issue for gnutls ('kTLS key update support'), which
> seems to indicate that gnutls supports keyupdate.
> And looking closer it seems to be transparent to the caller.

It makes sense to me. Which key is being used is an implementation
detail that an application using TLS shouldn't have to care about.

> But how does userspace get the KeyUpdate message in the first place?
> I _think_ we should get something like TLS_ALERT from the ktls stack.
> How is that implemented?

(talking strictly about "pure" ktls, not tls-handshake/tlshd)

Data records are passed to userspace via a simple recv(). Non-data
records (for example a KeyUpdate) need cmsg: the record type is
stuffed into a cmsg, the payload goes in the usual buffer. If the next
available record is non-data and no cmsg is provided, the read will
fail. See Documentation/networking/tls.rst (or
https://docs.kernel.org/networking/tls.html) for more details on how
cmsg is used.

Part of my patchset is pausing decryption when the kernel parses a
KeyUpdate message, until userspace has provided a new key (because
trying to decrypt the next record after the KeyUpdate with the old key
will just fail).

Quoting from the doc update in that patchset (updated for v6, so a bit
different from what you'll find on netdev):

---------- 8< ----------

TLS 1.3 Key Updates
-------------------

In TLS 1.3, KeyUpdate handshake messages signal that the sender is
updating its TX key. Any message sent after a KeyUpdate will be
encrypted using the new key. The userspace library can pass the new
key to the kernel using the TLS_TX and TLS_RX socket options, as for
the initial keys. TLS version and cipher cannot be changed.

To prevent attempting to decrypt incoming records using the wrong key,
decryption will be paused when a KeyUpdate message is received by the
kernel, until the new key has been provided using the TLS_RX socket
option. Any read occurring after the KeyUpdate has been read and
before the new key is provided will fail with EKEYEXPIRED. poll() will
not report any read events from the socket until the new key is
provided. There is no pausing on the transmit side.

---------- 8< ----------

> Have you tested your implementation with gnutls?

Frantisek has (a while back, I keep getting sidetracked from finishing
this patchset :/)

-- 
Sabrina