Re: [PATCH net-next 3/5] tls: implement rekey for TLS1.3

[Sabrina Dubroca <sd@queasysnail.net>]

2023-01-18, 17:25:46 -0800, Apoorv Kothari wrote:
> > 2023-01-17, 15:16:33 -0800, Kuniyuki Iwashima wrote:
> > > Hi,
> > > 
> > > Thanks for posting this series!
> > > We were working on the same feature.
> > > CC Apoorv from s2n team.
> > 
> > Ah, cool. Does the behavior in those patches match what your
> > implementation?
> 
> Thanks for submitting this, it looks great! We are working on testing this now.
> 
> > 
> > [...]
> > > > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> > > > index fb1da1780f50..9be82aecd13e 100644
> > > > --- a/net/tls/tls_main.c
> > > > +++ b/net/tls/tls_main.c
> > > > @@ -669,9 +669,12 @@ static int tls_getsockopt(struct sock *sk, int level, int optname,
> > > >  static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> > > >  				  unsigned int optlen, int tx)
> > > >  {
> > > > +	union tls_crypto_context tmp = {};
> > > > +	struct tls_crypto_info *old_crypto_info = NULL;
> > > >  	struct tls_crypto_info *crypto_info;
> > > >  	struct tls_crypto_info *alt_crypto_info;
> > > >  	struct tls_context *ctx = tls_get_ctx(sk);
> > > > +	bool update = false;
> > > >  	size_t optsize;
> > > >  	int rc = 0;
> > > >  	int conf;
> > > > @@ -687,9 +690,17 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> > > >  		alt_crypto_info = &ctx->crypto_send.info;
> > > >  	}
> > > >  
> > > > -	/* Currently we don't support set crypto info more than one time */
> > > > -	if (TLS_CRYPTO_INFO_READY(crypto_info))
> > > > -		return -EBUSY;
> > > > +	if (TLS_CRYPTO_INFO_READY(crypto_info)) {
> > > > +		/* Currently we only support setting crypto info more
> > > > +		 * than one time for TLS 1.3
> > > > +		 */
> > > > +		if (crypto_info->version != TLS_1_3_VERSION)
> > > > +			return -EBUSY;
> > > > +
> > > 
> > > Should we check this ?
> > > 
> > >                 if (!tx && !key_update_pending)
> > >                         return -EBUSY;
> > > 
> > > Otherwise we can set a new RX key even if the other end has not sent
> > > KeyUpdateRequest.
> > 
> > Maybe. My thinking was "let userspace shoot itself in the foot if it
> > wants".
> 
> I feel avoiding foot-guns is probably the correct thing to do. The RFC also has
> a requirement that re-key(process messages with new key) should only happen after
> a KeyUpdate is received so it would be nice if the kTLS implemention can help
> enforce this.
> 
> Based on the RFC https://www.rfc-editor.org/rfc/rfc8446#section-4.6.3:
>    Additionally, both sides MUST enforce that a KeyUpdate
>    with the old key is received before accepting any messages encrypted
>    with the new key.  Failure to do so may allow message truncation
>    attacks.

Ok. I'll add that in v2, unless someone is strongly against it.

Thanks.

-- 
Sabrina