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

[Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>]

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

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

And even if the implementation would read the contents, would it really
hurt that it's actually unitialized? They should still not be able to
influence the outcome here, so is there any other way that could cause
real-life problems? (assuming it's a legal to access buffer)

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

> - Eric

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com