Re: [Cfrg] Reviews of AES-GCM-SIV (draft-irtf-cfrg-gcmsiv-04.txt)

[Adam Langley <[e-mail]>]

On Wed, Jul 5, 2017 at 12:43 AM, Paterson, Kenny
<[e-mail]> wrote:
> Please read these reviews carefully and take them into account in the next
> version of your draft. It would be helpful if you could respond on list as
> to how you have handled the various comments.

Now that the IETF meeting is complete and the submission window has
reopened, we've just published -06. Please see below for details on
the changes. We would like to thank the reviewers for their time and
attention.


All three reviewers requested that pseudo-code be included in addition
to the English descriptions. This has been done.

Several requested clarity about whether inputs that are not a whole
number of bytes are valid. We have clarified that they are not.
However, the lengths in the length block are still multiplied by eight
in keeping with AES-GCM. As noted in the appendix, POLYVAL and GHASH
have a simple relationship and it may be useful for implementations
that which to support both in minimal space to be able to exploit
this.

> One thing that may be problematic to an implementor was the nonces listed
> in the test vectors.  AES-GCM-SIV takes 12 byte nonces, the nonces listed
> are 16 bytes long.  While this is unlikely to be a major source of
> confusion for an implementator, I suggest that the nonces be trimmed down
> before publishing.

This was a mistake on our part and has been corrected.

> While the test vectors are quite good in general, they use only one nonce,
> and two keys (one for each key length).

Eight additional test vectors have been added (for each of the 128-
and 256-bit versions) that have random keys, nonces and inputs.

> The bytes in the test vector are listed LSB to MSB.  This rather assumes
> that the implementor is using little-endian byte ordering; I would suggest
> that this be changed to a more endian-neutral notation (possibly by just
> omitted the LSB and MSB labels).

The labels have been removed.

> Reviewer: Tibor Jager

> Section 1 "Introduction", 1st paragraph: I suggest to replace
>   "...that is easier for practitioners to use correctly."
> with
>   "that is easier to use correctly."

Agreed. Done.

> In Section 4, first paragraph, the text suggests that plaintexts and
> additional data of arbitrary length can be encrypted. However, the
> description of the decryption procedure in Section 5 rejects ciphertexts
> of size larger than 2^36+16 bytes, and Section 6 gives upper bounds on
> the plaintext and AD sizes P_MAX and A_MAX.

"arbitrary" has been replaced with "variable".

> In Section 4, last paragraph, the result of encryption is the "resulting
> ciphertext ... followed by the tag". Thus, in this notation, the tag is
> not part of the "ciphertext", but it is separate and sent along with the
> ciphertext.
> However, at the beginning of Section 5, decryption algorithm receives as
> input key, nonce, AD, and a ciphertext, and the ciphertext is split into
> the encrypted plaintext and the tag, thus the "ciphertext" contains the
> tag here. One could unify this, by always considering the tag as part of
> the ciphertext.

We agree that this is confusing and have changed the wording so that
the "ciphertext" that results from AES-GCM-SIV always refers to the
final result.

> Section 8, very very nitpicking: One could mention here that the
> plaintext are the bit strings corresponding to the *ASCII encoding* of
> "Hello world" and "example".

Done.

> Section 8, 5th paragraph, again very nitpicking: Some developers may
> have difficulties in understanding immediately which numbers are given
> in hexadecimal notation, and which in decimal notation. For clarity, one
> could write here something like:
> "example": 7 characters = 56 bits = 0x38 bits
> "Hello world": 11 characters = 88 bits = 0x58 bits

Done.

> Section 9, 7th paragraph: "Suzuki et al. [multibirthday]", the reference
> lists Kazuhiro as first author, so it seems this should be Kazuhiro et al.

The authors are likely Japanese and thus typically would put their
family name first. However, as best we can tell, the family name is
"Suzuki" and thus it seems that the authors may have written their
names in the western style for this paper. The BibTeX citation
provided by Springer supports this. So we /think/ that "Suzuki et al"
is correct here.

> Reviewer: Bjoern Tackmann

> In that description, I stumbled particularly over the “_length block_” -
> why the underscores?

The underscores are how xml2rfc was interpreting <spanx> in plain text
format. We agree that that's not the correct notation in that case and
have changed to quotation marks.

> Also, I think it would make sense to explicitly
> clarify that the 32-bit counter value for the CTR part is explicitly
> allowed to overflow.

Done. There are also now test vectors that depend on the overflow behaviour.

> One aspect that read a bit arbitrary to me was the domain separation for
> AES by setting the first bit of the last byte to 1/0, and in particular
> that seemed a bit wasteful since it is evaluated only once with the bit
> set to 0. Wouldn't it be simpler (and possibly even with better security,
> but at the cost of limiting the length to 2^32 - 1 blocks) to not fiddle
> with the bits but just use the first counter-block to compute the tag?
> Note that this would certainly require re-doing some part of the security
> analysis, and the gains seem moderate, so I’m fine with this comment being
> discarded. (Just wanted to make sure it is discarded consciously.)

There may be a small security gain to be had by using a more subtle
method for domain separation. But we believe that the benefit would be
minor and the current design has the advantage the the separation is
completely unambiguous. Our feeling is that the clarity here is
useful, so the gain from changing is unclear and not worth redoing the
proofs for.


Cheers

AGL