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

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: <Y8fMLtYmlIw2wfMM@hog>
Date:   Wed, 18 Jan 2023 11:38:38 +0100
From:   Sabrina Dubroca <sd@...asysnail.net>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>
Cc:     fkrenzel@...hat.com, netdev@...r.kernel.org, apoorvko@...zon.com
Subject: Re: [PATCH net-next 3/5] tls: implement rekey for TLS1.3

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?

[...]
> > 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".

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