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: <20201116165454.5b5dd864@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 16 Nov 2020 16:54:54 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vadim Fedorenko <vfedorenko@...ek.ru>
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 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?

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