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

[Eric Biggers]

On Wed, Mar 04, 2020 at 11:58:53AM -0800, Eric Biggers wrote:
> 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...
> 

I sent out new patches -- can you review and test them?
https://lkml.kernel.org/r/20200304224405.152829-1-ebiggers@xxxxxxxxxx

Thanks,

- Eric