netdev - Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a different type

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: <20230907100827.7c3553ec@kernel.org>
Date: Thu, 7 Sep 2023 10:08:27 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Herbert Xu <herbert@...dor.apana.org.au>, netdev@...r.kernel.org, Dave
 Watson <davejwatson@...com>, Vakul Garg <vakul.garg@....com>, Boris
 Pismenny <borisp@...dia.com>, John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a
 different type

On Thu, 7 Sep 2023 14:21:59 +0200 Sabrina Dubroca wrote:
> I wonder if the way we're using ctx->async_wait here is correct. I'm
> observing crypto_wait_req return 0 even though the decryption hasn't
> run yet (and it should return -EBADMSG, not 0). I guess
> tls_decrypt_done calls the completion (since we only had one
> decrypt_pending), and then crypto_wait_req thinks everything is
> already done.
> 
> Adding a fresh crypto_wait in tls_do_decryption (DECLARE_CRYPTO_WAIT)
> and using it in the !darg->async case also seems to fix the UAF (but
> makes the bad_cmsg test case fail in the same way as what I wrote in
> the cover letter for bad_in_large_read -- not decrypting the next
> message at all makes the selftest pass).
> 
> Herbert, WDYT? We're calling tls_do_decryption twice from the same
> tls_sw_recvmsg invocation, first with darg->async = true, then with
> darg->async = false. Is it ok to use ctx->async_wait for both, or do
> we need a fresh one as in this patch?

I think you're right, we need a fresh one. The "non-async" call to
tls_do_decryption() will see the completion that the "async" call
queued and think it can progress. Then at the end of recv we check
->decrypt_pending and think we're good to exit. But the "non-async"
call is still crypt'ing.

All makes sense.