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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
> Sent: Tuesday, July 30, 2019 6:26 AM
> To: Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx>
> Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>; Pascal van Leeuwen <pascalvanl@xxxxxxxxx>;
> linux-crypto@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx
> 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




[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux