Re: [PATCH v2] crypto: testmgr - sync both RFC4106 IV copies

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Mar 04, 2020 at 03:48:47PM +0200, Gilad Ben-Yossef wrote:
> 
> >
> > > +     const unsigned int aad_tail_size = suite->skip_aad_iv ? aad_ivsize : 0;
> > >       const unsigned int authsize = vec->clen - vec->plen;
> > >
> > >       if (prandom_u32() % 2 == 0 && vec->alen > aad_tail_size) {
> > >                /* Mutate the AAD */
> > >               flip_random_bit((u8 *)vec->assoc, vec->alen - aad_tail_size);
> > > +             if (suite->auth_aad_iv)
> > > +                     memcpy((u8 *)vec->iv,
> > > +                            (vec->assoc + vec->alen - aad_ivsize),
> > > +                            aad_ivsize);
> >
> > Why sync the IV copies here?  When 'auth_aad_iv', we assume the copy of the IV
> > in the AAD (which was just corrupted) is authenticated.  So we already know that
> > decryption should fail, regardless of the other IV copy.
> 
> Nope. We know there needs to be a copy of the IV in the AAD and we know the IV
> should be included in calculating in the authentication tag. We don't know which
> copy of the IV will be used by the implementation.
> 
> Case in point - the ccree driver actually currently uses the copy of
> the IV passed via
> req->iv for calculating the IV contribution to the authentication tag,
> not the one in the AAD.
> 
> And what happens then if you don't do this copy than is that you get
> an unexpected
> decryption success where the test expects failure, because the driver
> fed the HW the
> none mutated copy of the IV from req->iv and not the mutated copy
> found in the AAD.

Okay, well in that case I don't see any difference between the two flags.  This
is because changing the IV *must* affect the authentication tag for *any* AEAD
algorithm, otherwise it's not actually authenticated encryption.

So why don't we just use a single flag 'aad_iv' that's set on rfc4106, rfc4309,
rfc4543, and rfc7539esp?  This flag would mean that the last bytes of the AAD
buffer must contain another copy of the IV for the behavior to be well-defined.

> > > @@ -2208,6 +2220,10 @@ static void generate_aead_message(struct aead_request *req,
> > >       /* Generate the AAD. */
> > >       generate_random_bytes((u8 *)vec->assoc, vec->alen);
> > >
> > > +     if (suite->auth_aad_iv && (vec->alen > ivsize))
> > > +             memcpy(((u8 *)vec->assoc + vec->alen - ivsize), vec->iv,
> > > +                    ivsize);
> >
> > Shouldn't this be >= ivsize, not > ivsize?
> Indeed.
> 
> > And doesn't the IV need to be synced
> > in both the skip_aad_iv and auth_aad_iv cases?
> 
> Nope, because in the skip_aad_iv case we never mutate the IV, so no
> point in copying.

But even if the IV isn't mutated, both copies still need to be the same, right?
We seem to have concluded that the behavior should be implementation-defined
when they're different, so that's the logical consequence...

- Eric