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
