Message-ID: <CAOtvUMfsDfWskU8HYHfRhwqeN_DZcNZBo9g9MwXbjTcfe3-ocQ@mail.gmail.com>
Date:   Wed, 26 Feb 2020 17:09:57 +0200
From:   Gilad Ben-Yossef <gilad@...yossef.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Ofir Drang <ofir.drang@....com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] crypto: testmgr - sync both RFC4106 IV copies

On Tue, Feb 25, 2020 at 10:02 PM Eric Biggers <ebiggers@...nel.org> wrote:
>
> On Tue, Feb 25, 2020 at 05:48:34PM +0200, Gilad Ben-Yossef wrote:
> > RFC4106 AEAD ciphers the AAD is the concatenation of associated
> > authentication data || IV || plaintext or ciphertext but the
> > random AEAD message generation in testmgr extended tests did
> > not obey this requirements producing messages with undefined
> > behaviours. Fix it by syncing the copies if needed.
> >
> > Since this only relevant for developer only extended tests any
> > additional cycles/run time costs are negligible.
> >
> > This fixes extended AEAD test failures with the ccree driver
> > caused by illegal input.
> >
> > Signed-off-by: Gilad Ben-Yossef <gilad@...yossef.com>
> > Reported-by: Geert Uytterhoeven <geert+renesas@...der.be>
> > Cc: Eric Biggers <ebiggers@...nel.org>
> > ---
> >  crypto/testmgr.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > index cf565b063cdf..288f349a0cae 100644
> > --- a/crypto/testmgr.c
> > +++ b/crypto/testmgr.c
> > @@ -95,6 +95,11 @@ struct aead_test_suite {
> >       /*
> >        * Set if the algorithm intentionally ignores the last 8 bytes of the
> >        * AAD buffer during decryption.
> >        */
> >       unsigned int esp_aad : 1;
> > +
> > +     /*
> > +      * Set if the algorithm requires the IV to trail the AAD buffer.
> > +      */
> > +     unsigned int iv_aad : 1;
> >  };
>
> What's the difference between esp_aad and iv_aad?  Are you sure we need another
> flag and not just use the existing flag?

Yes, because we have 3 distinct states -
1. No IV in the AAD buffer - "normal"  AEAD.
2. There is a copy of the IV in the AAD buffer and it is NOT used for
ICV computation purposes - RFC4106 and RFC 4309.
3, There is a copy of the IV in the AAD buffer and it is used for ICV
computation purposes - RFC 4543.

3 states needs at least 2 bits.

> If they're both needed, please document them properly.  You're currently setting

I will add a remark explaining this.
I chose to keep the "esp_aad" name, since it was there before, but
possibly this is not a good choice in light of your comments so will
change that too.

> them both on some algorithms, which based on the current comments is a logical
> contradiction because esp_aad is documented to mean that the last 8 bytes are
> ignored while iv_aad is documented to mean that these bytes must be the IV.

I believe it isn't a contradiction after all. Consider -

RFC 4106 needs to have a copy of the IV in the AAD buffer which is
identical to the one being passed via the normal mechanism in the API.
If they are not identical, we have no way to know which copy of the IV
is being used by an implementation as part of the AES-GCM nonce and so
the generated message may be different from the one being used by the
generic implementation. which results in encryption test failing when
compared to the generic implementation  results, even though there is
nothing wrong with the tested implementation and indeed the previous
compassion against the precomputed test vectors passes.
This is what happened with the ccree driver. Note that this has
nothing to do with mutating the message - this is an encryption test
failing.
This is the iv_aad flag, which will need to be true in this case.

On the other hand when testing decryption and mutating the AEAD
message, we need to know not to mutate the IV copy in the AAD buffer,
since it is ignored.
This is the esp_aad flag, which will also needs to be true in this case.

Last but not least, RFC 4543 need a copy of the IV in the AAD buffer
However, this copy DOES get hashed as part of the ICV computation, so
-
We need iv_aad flag to be true to let us know we need to copy the IV.
However, we need esp_aad to be false since it's fine and even
desirable to mutate the IV copy in the AAD buffer.

You are correct though that I can make the 2nd copy of the IV post
mutation dependant on esp_aad not being set, though...


I hope this explains this mess better.
It certainly took me a while to figure out what is going on...


>
> >
> >  struct cipher_test_suite {
> > @@ -2207,6 +2212,10 @@ static void generate_aead_message(struct aead_request *req,
> >
> >       /* Generate the AAD. */
> >       generate_random_bytes((u8 *)vec->assoc, vec->alen);
> > +     /* For RFC4106 algs, a copy of the IV is part of the AAD */
> > +     if (suite->iv_aad)
> > +             memcpy(((u8 *)vec->assoc + vec->alen - ivsize), vec->iv,
> > +                    ivsize);
>
> What guarantees that vec->alen >= ivsize?

You are right, I need to check for that.
However, if it isn't this can't be a legal RFC4106 message and we will
fail encryption .

Thanks!
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

Powered by blists - more mailing lists