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

[Apoorv Kothari <apoorvko@amazon.com>]

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

> 
> > > +		update = true;
> > > +		old_crypto_info = crypto_info;
> > > +		crypto_info = &tmp.info;
> > > +	}
> > >  
> > >  	rc = copy_from_sockptr(crypto_info, optval, sizeof(*crypto_info));
> > >  	if (rc) {
> > > @@ -704,6 +715,15 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> > >  		goto err_crypto_info;
> > >  	}
> > >  
> > > +	if (update) {
> > > +		/* Ensure that TLS version and ciphers are not modified */
> > > +		if (crypto_info->version != old_crypto_info->version ||
> > > +		    crypto_info->cipher_type != old_crypto_info->cipher_type) {
> > > +			rc = -EINVAL;
> > > +			goto err_crypto_info;
> > > +		}
> > > +	}
> > > +
> > >  	/* Ensure that TLS version and ciphers are same in both directions */
> > >  	if (TLS_CRYPTO_INFO_READY(alt_crypto_info)) {
> > 
> > We can change this to else-if.
> 
> Ok.
> 
> > >  		if (alt_crypto_info->version != crypto_info->version ||
> [...]
> > > @@ -2517,9 +2525,28 @@ int tls_set_sw_offload(struct sock *sk, int tx)
> > >  	u16 nonce_size, tag_size, iv_size, rec_seq_size, salt_size;
> > >  	struct crypto_tfm *tfm;
> > >  	char *iv, *rec_seq, *key, *salt, *cipher_name;
> > > -	size_t keysize;
> > > +	size_t keysize, crypto_info_size;
> > >  	int rc = 0;
> > >  
> > > +	if (new_crypto_info) {
> > > +		/* non-NULL new_crypto_info means rekey */
> > > +		src_crypto_info = new_crypto_info;
> > > +		if (tx) {
> > > +			sw_ctx_tx = ctx->priv_ctx_tx;
> > > +			crypto_info = &ctx->crypto_send.info;
> > > +			cctx = &ctx->tx;
> > > +			aead = &sw_ctx_tx->aead_send;
> > > +			sw_ctx_tx = NULL;
> > 
> > sw_ctx_tx is already initialised.
> 
> No, it was NULL at the beginning of the function, but then I used it
> to set aead on the previous line, so I need to clear it again. I could
> use a temp variable instead if you think it's better.
> 
> > > +		} else {
> > > +			sw_ctx_rx = ctx->priv_ctx_rx;
> > > +			crypto_info = &ctx->crypto_recv.info;
> > > +			cctx = &ctx->rx;
> > > +			aead = &sw_ctx_rx->aead_recv;
> > > +			sw_ctx_rx = NULL;
> > 
> > Same here.
> > 
> > 
> > > +		}
> > > +		goto skip_init;
> > > +	}
> > > +
> > >  	if (tx) {
> > >  		if (!ctx->priv_ctx_tx) {
> > >  			sw_ctx_tx = kzalloc(sizeof(*sw_ctx_tx), GFP_KERNEL);
> 
> Thanks for the comments.
> 
> -- 
> Sabrina

--
Apoorv