netdev - Re: [net] net/tls: fix corrupted data in recvmsg

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]

Date:   Sun, 15 Nov 2020 02:26:30 +0000
From:   Vadim Fedorenko <vfedorenko@...ek.ru>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Boris Pismenny <borisp@...dia.com>,
        Aviad Yehezkel <aviadye@...dia.com>, netdev@...r.kernel.org
Subject: Re: [net] net/tls: fix corrupted data in recvmsg

No, I don't have any BPFs in test.
If we have Application Data in TCP queue then tls_sw_advance_skb
will change ctx->control from 0x16 to 0x17 (TLS_RECORD_TYPE_DATA)
and the loop will continue. The patched if will make zc = true and
data will be decrypted into msg->msg_iter.
After that the loop will break on:
                 if (!control)
                         control = tlm->control;
                 else if (control != tlm->control)
                         goto recv_end;

and the data will be lost.
Next call to recvmsg will find ctx->decrypted set to true and will
copy the unencrypted data from skb assuming that it has been decrypted
already.

The patch that I put into Fixes: changed the check you mentioned to
ctx->control, but originally it was checking the value of control that
was stored before calling to tls_sw_advance_skb.

On 15.11.2020 02:12, Jakub Kicinski wrote:
> On Sat, 14 Nov 2020 07:09:42 +0300 Vadim Fedorenko wrote:
>> If tcp socket has more data than Encrypted Handshake Message then
>> tls_sw_recvmsg will try to decrypt next record instead of returning
>> full control message to userspace as mentioned in comment. The next
>> message - usually Application Data - gets corrupted because it uses
>> zero copy for decryption that's why the data is not stored in skb
>> for next iteration. Disable zero copy for this case.
>>
>> Fixes: 692d7b5d1f91 ("tls: Fix recvmsg() to be able to peek across multiple records")
>> Signed-off-by: Vadim Fedorenko <vfedorenko@...ek.ru>
> Do you have some BPF in the mix?
>
> I don't see how we would try to go past a non-data record otherwise,
> since the loop ends like this:
>
> 		if (tls_sw_advance_skb(sk, skb, chunk)) {
> 			/* Return full control message to
> 			 * userspace before trying to parse
> 			 * another message type
> 			 */
> 			msg->msg_flags |= MSG_EOR;
> 			if (ctx->control != TLS_RECORD_TYPE_DATA)
> 				goto recv_end;
> 		} else {
> 			break;
> 		}