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
