netdev - Re: Setting TLS_RX and TLS_TX crypto info more than once?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <77DB4DFF-0950-47D4-A6A1-56F6D7142B19@holtmann.org>
Date:   Wed, 25 Jan 2023 11:24:18 +0100
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Sabrina Dubroca <sd@...asysnail.net>
Cc:     Dave Watson <davejwatson@...com>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>
Subject: Re: Setting TLS_RX and TLS_TX crypto info more than once?

Hi Sabrina,

>> in commit 196c31b4b5447 you limited setsockopt for TLS_RX and TLS_TX
>> crypto info to just one time.
> 
> Looking at commit 196c31b4b5447, that check was already there, it only
> got moved.

I still think that check is not needed. We should get rid of it for
TLS 1.2 and TLS 1.3.

(and Ilya seems to have left Mellanox/nVidia anyway)

>> +       crypto_info = &ctx->crypto_send;
>> +       /* Currently we don't support set crypto info more than one time */
>> +       if (TLS_CRYPTO_INFO_READY(crypto_info))
>> +               goto out;
>> 
>> This is a bit unfortunate for TLS 1.3 where the majority of the TLS
>> handshake is actually encrypted with handshake traffic secrets and
>> only after a successful handshake, the application traffic secrets
>> are applied.
>> 
>> I am hitting this issue since I am just sending ClientHello and only
>> reading ServerHello and then switching on TLS_RX right away to receive
>> the rest of the handshake via TLS_GET_RECORD_TYPE. This works pretty
>> nicely in my code.
>> 
>> Since this limitation wasn’t there in the first place, can we get it
>> removed again and allow setting the crypto info more than once? At
>> least updating the key material (the cipher obviously has to match).
>> 
>> I think this is also needed when having to do any re-keying since I
>> have seen patches for that, but it seems they never got applied.
> 
> The patches are still under discussion (I only posted them a week ago
> so "never got applied" seems a bit harsh):
> https://lore.kernel.org/all/cover.1673952268.git.sd@queasysnail.net/T/#u

My bad, I kinda remembered they are from end of 2020. Anyway, following
that thread, I see you fixed my problem already.

The encrypted handshake portion is actually simple since it defines
really clear boundaries for the handshake traffic secret. The deployed
servers are a bit optimistic since they send you an unencrypted
ServerHello followed right away by the rest of the handshake messages
fully encrypted. I was surprised I can MSG_PEEK at the TLS record
header and then just read n bytes of just the ServerHello leaving
everything else in the receive buffer to be automatically decrypted
once I set the keys. This allows for just having the TLS handshake
implemented in userspace.

It is a little bit unfortunate that with the support for TLS 1.3, the
old tls12_ structures for providing the crypto info have been used. I
would have argued for providing the traffic_secret into the kernel and
then the kernel could have easily derived key+iv by itself. And with
that it could have done the KeyUpdate itself as well.

The other problem is actually that userspace needs to know when we are
close to the limits of AES-GCM encryptions or when the sequence number
is about to wrap. We need to feed back the status of the rec_seq back
to userspace (and with that also from the HW offload).

I would argue that it might have made sense not just specifying the
starting req_seq, but also either an ending or some trigger when
to tell userspace that it is a good time to re-key.

Regards

Marcel