Last Call Review of draft-ietf-tls-ecdhe-psk-aead-03
review-ietf-tls-ecdhe-psk-aead-03-secdir-lc-kaduk-2017-05-18-00

Team Security Area Directorate (secdir)
Title Last Call Review of draft-ietf-tls-ecdhe-psk-aead-03
Request Last Call - requested 2017-05-04
Reviewer Benjamin Kaduk
Review result Has Nits
Posted at https://mailarchive.ietf.org/arch/msg/secdir/CTTrcgsiBaJIpW-iAkqbRs5wUa8
Last updated 2017-05-18

Review
review-ietf-tls-ecdhe-psk-aead-03-secdir-lc-kaduk-2017-05-18

Hi all,

I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the
IESG.  These comments were written primarily for the benefit of the
security area directors.  Document editors and WG chairs should
treat these comments just like any other last call comments.

This document is ready with nits.

Essentially, we are filling in a gap in the TLS (< 1.3) ciphersuite
space, thought of as a cross product of key exchange and
cipher+mac/AEAD -- we have some of the combinations (PSK with ECDHE
but no AES-[GC]CM, PSK with AES-GCM but only non-EC DH, etc.) but
not quite this one.

That said, it seems a little silly to only partially fill the gap
(by omitting the AES_256_CCM* cipher suites), even though there is
not currently demand for them.

This document is just assembling pieces that were already specified
elsewhere, so it need not contain much detail itself, which is fine.
That said, I think section 3 should probably state explicitly which
pieces it uses, instead of a vague reference of being "based on RFC
4279".  So, "The ServerKeyExchange and ClientKeyExchange messages
from RFC 5489 for ECDHE_PSK are used, and the premaster secret is
computed in the same manner as for ECDHE_PSK key exchange in RFC
5489."  (I am not sure why RFC 4279 is cited in the current text; it
does not cover ECDHE_PSK.)

The premaster_secret structure so used basically ends up putting the
ECDH output first followed by the static PSK; with the pre-TLS 1.2
PRF, that would give the ECDH shared secret to md5 and the PSK to
sha1, which is perhaps another reason to not use these with pre-1.2
worth mentioning (in addition to the AEAD availability).

The security considerations largely refer to those of the other
documents providing the pieces that are combined together here;
those referenced security considerations sections are more than
adequate here, as this document itself does not really do anything
particularly novel.  That said, if we are going to reiterate the
entropy requirement for PSKs inline, we probably ought to also
reiterate the nonce-reuse considerations for GCM and CCM.  The
relevant constructions help, but there are still ways to mess up and
reuse a nonce when doing crypto in parallel, if I remember the
GCM/TLS document's security considerations correctly.


Some other editorial nits follow.

I see we're already discussing "perfect forward secrecy" vs.
"forward secrecy"; my preference is for just "forward secrecy" (and
I may have been one of the more active voices in causing TLS 1.3 to
switch), but in the grand scheme of things it doesn't really matter.

In section 2, the RFC 7525 reference that "AEAD algorithms [...] are
strongly recommended" is scoped to just (D)TLS, so the text here
should probably also list that qualification.

In section 4, "... make use of the authenticated encryption with
additional data (AEAD) defined in TLS 1.2" seems to make AEAD into
something of a unique object, as opposed to a class of things with
multiple possible instantiations.  I would consider something like
"... (AEAD) concept".

In section 4, "these cipher suites MUST NOT be negotiated in TLS
versions prior to 1.2" should probably clarify that "these" cipher
suites are the new ones specified by this document.

s/Servers,\nwhich/Servers\nthat/

Does the last paragraph of section 5 want a note "RFC Editor please
remove this paragraph"?

Section 6, third paragraph, s/by using short PSK/by using a short
PSK/.  Also, s/by a human and thus/by a human which thus/, maybe?

Last paragraph page 4, comma after "i.e.".


-Ben