netdev - Re: [PATCH net-next v4 1/6] tls: block decryption when a rekey is pending

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: <20241210153310.6535e6b3@kernel.org>
Date: Tue, 10 Dec 2024 15:33:10 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Vadim Fedorenko <vfedorenko@...ek.ru>, Frantisek
 Krenzelok <fkrenzel@...hat.com>, Kuniyuki Iwashima <kuniyu@...zon.com>,
 Apoorv Kothari <apoorvko@...zon.com>, Boris Pismenny <borisp@...dia.com>,
 John Fastabend <john.fastabend@...il.com>, Shuah Khan <shuah@...nel.org>,
 linux-kselftest@...r.kernel.org, Gal Pressman <gal@...dia.com>, Marcel
 Holtmann <marcel@...tmann.org>, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH net-next v4 1/6] tls: block decryption when a rekey is
 pending

On Tue, 10 Dec 2024 17:16:09 +0100 Sabrina Dubroca wrote:
> > The only reason the check_pending_rekey() can fail is if the message is
> > mis-formatted, I wonder if we are better off ignoring mis-formatted
> > rekeys? User space will see them and break the connection, anyway.
> > Alternatively - we could add a selftest for this.  
> 
> Going back to tls_check_pending_rekey():
> 
> > > +	if (rxm->full_len < 1)
> > > +		return -EINVAL;  
> 
> There's no real reason to fail here, we should probably just ignore
> it. It's not a rekey, and it's not a valid handshake message, but one
> could say that's not the kernel's problem. I'll make that return 0
> unless you want to keep -EINVAL.

returning 0 SGTM

> Hard to write a selftest for because we'd have to do a sendmsg with
> len=0, or do the crypto in the selftest.
> 
> > > +	err = skb_copy_bits(skb, rxm->offset, &hs_type, 1);
> > > +	if (err < 0)
> > > +		return err;  
> 
> This probably means that the skb we got from the parser was broken. If
> we can't read 1B with full_len >= 1, something's wrong. Maybe worth a
> DEBUG_NET_WARN_ON_ONCE?

Also SG!

> > > +	if (hs_type == TLS_HANDSHAKE_KEYUPDATE) {  
> 
> Here I don't actually check if it's a correct KeyUpdate message [1],
> we pause decryption and let userspace decide what to do (probably
> break the connection as you said).