netdev - Re: [net v2] 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]

Message-ID: <f88588ce-03c7-74e0-1c43-0213d9133abd@novek.ru>
Date:   Tue, 17 Nov 2020 00:59:54 +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 v2] net/tls: fix corrupted data in recvmsg


On 17.11.2020 00:54, Jakub Kicinski wrote:
> On Tue, 17 Nov 2020 00:45:11 +0000 Vadim Fedorenko wrote:
>> On 17.11.2020 00:26, Jakub Kicinski wrote:
>>> On Sun, 15 Nov 2020 07:16:00 +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. Revert check to not decrypt next record if
>>>> current is not Application Data.
>>>>
>>>> Fixes: 692d7b5d1f91 ("tls: Fix recvmsg() to be able to peek across multiple records")
>>>> Signed-off-by: Vadim Fedorenko <vfedorenko@...ek.ru>
>>>> ---
>>>>    net/tls/tls_sw.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
>>>> index 95ab5545..2fe9e2c 100644
>>>> --- a/net/tls/tls_sw.c
>>>> +++ b/net/tls/tls_sw.c
>>>> @@ -1913,7 +1913,7 @@ int tls_sw_recvmsg(struct sock *sk,
>>>>    			 * another message type
>>>>    			 */
>>>>    			msg->msg_flags |= MSG_EOR;
>>>> -			if (ctx->control != TLS_RECORD_TYPE_DATA)
>>>> +			if (control != TLS_RECORD_TYPE_DATA)
>>> Sorry I wasn't clear enough, should this be:
>>>
>>> 	if (ctx->control != control)
>>>
>>> ? Otherwise if we get a control record first and then data record
>>> the code will collapse them, which isn't correct, right?
>>>   
>>>>    				goto recv_end;
>>>>    		} else {
>>>>    			break;
>> I think you mean when ctx->control is control record and control is
>> data record.
> Yup.
>
>> In this case control message will be decrypted without
>> zero copy and will be stored in skb for the next recvmsg, but will
>> not be returned together with data message.
> Could you point me to a line which breaks the loop in that case?
>
Sure!

		if (!control)
			control = tlm->control;
		else if (control != tlm->control)
			goto recv_end;


In that case control != tlm->control
Variable control is set only once and never changes again.

>> This behavior is the same
>> as for TLSv1.3 when record type is known only after decrypting.
>> But if we want completely different flow for TLSv1.2 and TLSv1.3
>> then changing to check difference in message types makes sense.