> -----Original Message----- > From: Eric Biggers <ebigg...@kernel.org> > Sent: Tuesday, July 30, 2019 6:26 AM > To: Pascal Van Leeuwen <pvanleeu...@verimatrix.com> > Cc: Herbert Xu <herb...@gondor.apana.org.au>; Pascal van Leeuwen > <pascalv...@gmail.com>; > linux-crypto@vger.kernel.org; da...@davemloft.net > Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for > AEAD fuzz > testing > > 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. > My guess is trying that first part will give you the second part for free :-) (i.e. if it's random, it almost certainly won't authenticate)
> > > > > > > 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. > Ok, fine > - Eric Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com
