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: <a6dec380-1ebc-d495-da67-7bd61525d4a8@huawei.com>
Date: Thu, 7 Sep 2023 20:59:51 +0800
From: "liujian (CE)" <liujian56@...wei.com>
To: Sabrina Dubroca <sd@...asysnail.net>
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()



On 2023/9/6 19:02, Sabrina Dubroca wrote:
> 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).

Thanks for your review~

By the way, does the return of EBADMSG mean that the tls link needs to 
renegotiate the encryption information or re-establish the link?

And is this okay?
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 1ed4a611631f..d1fc295b83b5 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -817,7 +817,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, 
struct sock *sk,
         psock = sk_psock_get(sk);
         if (!psock || !policy) {
                 err = tls_push_record(sk, flags, record_type);
-               if (err && sk->sk_err == EBADMSG) {
+               if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
                         *copied -= sk_msg_free(sk, msg);
                         tls_free_open_rec(sk);
                         err = -sk->sk_err;
@@ -846,7 +846,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, 
struct sock *sk,
         switch (psock->eval) {
         case __SK_PASS:
                 err = tls_push_record(sk, flags, record_type);
-               if (err && sk->sk_err == EBADMSG) {
+               if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
                         *copied -= sk_msg_free(sk, msg);
                         tls_free_open_rec(sk);
                         err = -sk->sk_err;

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