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] [day] [month] [year] [list]

Message-ID: <5bc6ebac-f399-add3-f528-422840b988d3@novek.ru>
Date:   Sun, 15 Nov 2020 04:11:05 +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



On 15.11.2020 03:54, Jakub Kicinski wrote:
> Please don't top post.
>
> On Sun, 15 Nov 2020 02:26:30 +0000 Vadim Fedorenko wrote:
>> 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.
> Ah! Missed that, unpausing the parser will make it serve us another
> message and overwrite ctx.
>
>> 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.
> Is there a reason why we wouldn't just go back to checking the stored
> control? Or better - put your condition there (control != ctx->control)?
> Decrypting the next record seems unnecessary given we can't return it.
Going back to checking stored control is working in TLS 1.2 but I cannot
test it with TLS 1.3. But it looks like it will work too in that case.
Variable control should store correct value to pass it in control message
and it's not chaged after that.
Ok, will send v2 with check reverted, thanks!