Setting TLS_RX and TLS_TX crypto info more than once?

Setting TLS_RX and TLS_TX crypto info more than once?

[Marcel Holtmann]

Hi Ilya,

in commit 196c31b4b5447 you limited setsockopt for TLS_RX and TLS_TX
crypto info to just one time.

+       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.

Any thoughts?

Regards

Marcel

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

[Sabrina Dubroca]

Hi Marcel,

2023-01-24, 18:48:56 +0100, Marcel Holtmann wrote:
> Hi Ilya,
> 
> 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.

> 
> +       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

-- 
Sabrina

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

[Marcel Holtmann]

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

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

[Jakub Kicinski]

On Wed, 25 Jan 2023 11:24:18 +0100 Marcel Holtmann wrote:
> 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.

Could you say more about your use case?

What you're describing sound contrary to the common belief/design
direction of TLS which was to keep the kernel out of as much complexity
as possible, focus only on parts where we can add perf.

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

[Boris Pismenny]

On 25/01/2023 11:24, Marcel Holtmann wrote:
> 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.

That's quite cool!

>>>
>>> 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.

Well, we can always add a v2 if the use-case makes sense. There was no
TLS 1.3 when we upstreamed this, and I think that the above wouldn't
work for TLS1.2, right?
Having the kernel perform key derivation sounds nice, and it may help
the TLS_DEVICE rekey design I mentioned on another thread.

> 
> 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 agree about the problem. But, the part about HW is imprecise, the
kernel has all the state needed (except maybe in TLS_HW_RECORD).

> 
> 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.

We didn't do the rekey design when we coded this, back then the logic
was that rekeys are rare and connections are typically restarted when
a rekey is requested.

I think that having an end req_seq makes sense only if we do the rekey
in the kernel. On the TLS_DEVICE side, having the end_seq helps because
the driver will know: (1) when to switch to the next key on transmit;
(2) when to use the old key for retransmit; and (3) when to delete the
old key as all old key data has been transmitted. This can also work
with any number of keys/rekeys.

> 
> Regards
> 
> Marcel
>