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

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Jul 29, 2019 at 10:49:38PM +0000, Pascal Van Leeuwen wrote:
> Herbert,
> 
> > -----Original Message-----
> > From: Herbert Xu <herbert@gondor.apana.org.au>
> > Sent: Tuesday, July 30, 2019 12:31 AM
> > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> > Cc: Eric Biggers <ebiggers@kernel.org>; Pascal van Leeuwen <pascalvanl@gmail.com>; linux-
> > crypto@vger.kernel.org; davem@davemloft.net
> > Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing
> > 
> > On Mon, Jul 29, 2019 at 10:16:48PM +0000, Pascal Van Leeuwen wrote:
> > >
> > > > EINVAL is for invalid lengths while EBADMSG is for inauthentic inputs.
> > > > Inauthentic test vectors aren't yet automatically generated (even after this
> > > > patch), so I don't think EBADMSG should be seen here.  Are you sure there isn't
> > > > a bug in your patch that's causing this?
> > > >
> > > As far as I understand it, the output of the encryption is fed back in to
> > > decrypt. However, if the encryption didn't work due to blocksize mismatch,
> > > there would not be any valid encrypted and authenticated data written out.
> > > So, if the (generic) driver checks that for decryption, it would result in
> > > -EINVAL. If it wouldn't check that, it would try to decrypt and authentica
> > > te the data, which would almost certainly result in a tag mismatch and
> > > thus an -EBADMSG error being reported.
> > >
> > > So actually, the witnessed issue can be perfectly explained from a missing
> > > block size check in aesni.
> > 
> > The same input can indeed fail for multiple reasons.  I think in
> > such cases it is unreasonable to expect all implementations to
> > return the same error value.
> > 
> Hmmm ... first off, testmgr expects error codes to match exactly. So if
> you're saying that's not always the case, it would need to be changed.
> (but then, what difference would still be acceptable?)
> But so far it seems to have worked pretty well, except for this now.
> 
> 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.

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