Re: [PATCH v2] lib/crypto: chacha20poly1305: zeroize state in __chacha20poly1305_decrypt

[Mohamad Raizudeen]

On Sun, Oct 04, 2026 at 07:21:55PM +0200, Eric Biggers wrote:
> On Sun, Oct 04, 2026 at 10:04:13AM +0530, Mohamad Raizudeen wrote:
> > The `__chacha20poly1305_decrypt` function does not zeroize the chacha
> > state, unlike its encrypt counterpart. The regular
> > `chacha20poly1305_decrypt` function handles this by manually calling
> > chacha_zeroize_state(). However, `xchacha20poly1305_decrypt` returns
> > the result directly without clearing the state, leaving the derived
> > chacha20 subkey on the stack.
> > 
> > Fix this by moving the chacha_zeroize_state() call into
> > __chacha20poly1305_decrypt() itself, so the helper cleans up after
> > itself just like __chacha20poly1305_encrypt does. This ensures all
> > callers are secure without needing manual cleanup.
> > 
> > Fixes: ed20078b7e333 ("crypto: chacha20poly1305 - import construction and selftest from Zinc")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Suggested-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > Signed-off-by: Mohamad Raizudeen <raizudeen.kerneldev@xxxxxxxxx>
> 
> Sorry, to nitpick this a bit more:
> 
> Can you reword this to clarify that this is an ABI robustness
> improvement rather than a fix, since currently the single caller of
> xchacha20poly1305_decrypt() in wg_cookie_message_consume() doesn't
> require forward secrecy, as mentioned by Jason.  And maybe remove Fixes
> and 'Cc stable'.  Otherwise this commit will unnecessarily trigger all
> the stable backport and CVE spam.
> 
> > diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c
> > index ea42a28f4ff7..80904321458b 100644
> > --- a/lib/crypto/chacha20poly1305.c
> > +++ b/lib/crypto/chacha20poly1305.c
> > @@ -137,8 +137,10 @@ __chacha20poly1305_decrypt(u8 *dst, const u8 *src, const size_t src_len,
> >  		__le64 lens[2];
> >  	} b;
> >  
> > -	if (unlikely(src_len < POLY1305_DIGEST_SIZE))
> > +	if (unlikely(src_len < POLY1305_DIGEST_SIZE)) {
> > +		chacha_zeroize_state(chacha_state);
> >  		return false;
> > +	}
> 
> How about we move this length check into the two callers before they
> write anything to the state at all?  Then the state would not need to be
> zeroized if the length check fails.  Note that
> chacha20poly1305_decrypt_sg_inplace() already does it this way.
> 
> >  	memzero_explicit(&b, sizeof(b));
> >  
> > +	chacha_zeroize_state(chacha_state);
> >  	return !ret;
> 
> Nit: Use the same order and whitespace as
> chacha20poly1305_crypt_sg_inplace():
> 
> 	chacha_zeroize_state(chacha_state);
> 	memzero_explicit(&b, sizeof(b));
> 
> 	return !ret;
> 
> - Eric

Sure, that makes sense. I will drop the fixes and stable tags and reword
the commit message to focus on ABI robustness.

Moving the length check to the callers is a much cleaner approach. I
will implement that and fix the whitespace ordering for v3.

Thanks,
Mohamad Raizudeen