netdev - Re: [PATCH net] tls: do not return error when the tls_bigint overflows in tls_advance_record

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: <ZPhcTQ3mFQYmTHet@hog>
Date: Wed, 6 Sep 2023 13:02:37 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Liu Jian <liujian56@...wei.com>
Cc: borisp@...dia.com, john.fastabend@...il.com, kuba@...nel.org,
	davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
	vfedorenko@...ek.ru, netdev@...r.kernel.org
Subject: Re: [PATCH net] tls: do not return error when the tls_bigint
 overflows in tls_advance_record_sn()

2023-09-06, 14:52:37 +0800, Liu Jian wrote:
> This is because the value of rec_seq of tls_crypto_info configured by the
> user program is too large, for example, 0xffffffffffffff. In addition, TLS
> is asynchronously accelerated. When tls_do_encryption() returns
> -EINPROGRESS and sk->sk_err is set to EBADMSG due to rec_seq overflow,
> skmsg is released before the asynchronous encryption process ends. As a
> result, the UAF problem occurs during the asynchronous processing of the
> encryption module.
> 
> I didn't see the rec_seq overflow causing other problems, so let's get rid
> of the overflow error here.
> 
> Fixes: 635d93981786 ("net/tls: free record only on encryption error")
> Signed-off-by: Liu Jian <liujian56@...wei.com>
> ---
>  net/tls/tls.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/tls/tls.h b/net/tls/tls.h
> index 28a8c0e80e3c..3f0e10df8053 100644
> --- a/net/tls/tls.h
> +++ b/net/tls/tls.h
> @@ -304,8 +304,7 @@ static inline void
>  tls_advance_record_sn(struct sock *sk, struct tls_prot_info *prot,
>  		      struct cipher_context *ctx)
>  {
> -	if (tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size))
> -		tls_err_abort(sk, -EBADMSG);
> +	tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size);

That seems wrong. We can't allow the record number to wrap, if breaks
the crypto. See for example:
https://datatracker.ietf.org/doc/html/rfc5288#section-6.1

The real fix would be to stop the caller from freeing the skmsg and
record if we go async. Once we go through async crypto, the record etc
don't belong to the caller anymore, they've been transfered to the
async callback. I'd say we need both tests in bpf_exec_tx_verdict:
-EINPROGRESS (from before 635d93981786) and EBADMSG (from
635d93981786).

Actually we need to check for both -EINPROGRESS and -EBUSY as I've
recently found out.

I've been running the selftests with async crypto and have collected a
few fixes that I was going to post this week (but not this one, since
we don't have a selftest for wrapping rec_seq). One of the patches
adds -EBUSY checks for all existing -EINPROGRESS, since the crypto API
can return -EBUSY as well if we're going through the backlog queue.

-- 
Sabrina