Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Jul 30, 2019 at 01:26:17AM +0000, Pascal Van Leeuwen wrote:
> > > > Oh, I see.  Currently the fuzz tests assume that if encryption fails with an
> > > > error (such as EINVAL), then decryption fails with that same error.
> > > >
> > > Ah ok, oops. It should really log the error that was returned by the
> > > generic decryption instead. Which should just be a matter of annotating
> > > it back to vec.crypt_error?
> > >
> > 
> > It doesn't do the generic decryption yet though, only the generic encryption.
> > 
> I didn't look at the code in enough detail to pick that up, I was expecting
> it do do generic decryption and compare that to decryption with the algorithm
> being fuzzed. So what does it do then? Compare to the original input to the
> encryption? Ok, I guess that would save a generic decryption pass but, as we
> see here, it would not be able to capture all the details of the API.

Currently to generate an AEAD test vector the code just generates a "random"
plaintext and encrypts it with the generic implementation.

My plan is to extend the tests to also sometimes generate a "random" ciphertext
and try to decrypt it; and also sometimes try to decrypt a corrupted ciphertext.

> 
> > > > Regardless of what we think the correct decryption error is, running the
> > > > decryption test at all in this case is sort of broken, since the ciphertext
> > > > buffer was never initialized.
> > > >
> > > You could consider it broken or just some convenient way of getting
> > > vectors that don't authenticate without needing to spend any effort ...
> > >
> > 
> > It's not okay for it to be potentially using uninitialized memory though, even
> > if just in the fuzz tests.
> > 
> Well, in this particular case things should fail before you even hit the
> actual processing, so memory contents should be irrelevant really.
> (by that same reasoning you would not actually hit vectors that don't
> authenticate, by the way, there was an error in my thinking there)

But the problem is that that's not what's actually happening, right?  "authenc"
actually does the authentication (of uninitialized memory, in this case) before
it gets around to failing due to the cbc length restriction.

Anyway, I suggest sending the patch I suggested as 1 of 2 to avoid this case (so
your patch does not cause test failures), then this patch as 2 of 2.

- Eric