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

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Jul 30, 2019 at 12:37:06AM +0000, Pascal Van Leeuwen wrote:
> > > You're the expert, but shouldn't there be some priority to the checks
> > > being performed? To me, it seems reasonable to do things like length
> > > checks prior to even *starting* decryption and authentication.
> > > Therefore, it makes more sense to get -EINVAL than -EBADMSG in this
> > > case. IMHO you should only get -EBADMSG if the message was properly
> > > formatted, but the tags eventually mismatched. From a security point
> > > of view it can be very important to have a very clear distinction
> > > between those 2 cases.
> > >
> > 
> > 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.

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

> > So for now we probably should just sidestep this
> > issue by skipping the decryption test if encryption failed, like this:
> > 
> > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > index 96e5923889b9c1..0413bdad9f6974 100644
> > --- a/crypto/testmgr.c
> > +++ b/crypto/testmgr.c
> > @@ -2330,10 +2330,12 @@ static int test_aead_vs_generic_impl(const char *driver,
> >  					req, tsgls);
> >  		if (err)
> >  			goto out;
> > -		err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name, cfg,
> > -					req, tsgls);
> > -		if (err)
> > -			goto out;
> > +		if (vec.crypt_error != 0) {
> > +			err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name,
> > +						cfg, req, tsgls);
> > +			if (err)
> > +				goto out;
> > +		}
> >  		cond_resched();
> >  	}
> >  	err = 0;
> > 
> > I'm planning to (at some point) update the AEAD tests to intentionally generate
> > some inauthentic inputs, but that will take some more work.
> > 
> > - Eric
> >
> I believe that's a rather essential part of verifying AEAD decryption(!)
> 

Agreed, which is why I am planning to work on it :-).  Actually a while ago I
started a patch for it, but there are some issues I haven't had time to address
quite yet:
https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/commit/?h=wip-crypto&id=687f4198ba09032c60143e0477b48f94c5714263

- Eric