Re: [secdir] [TLS] secdir review of draft-ietf-tls-ecdhe-psk-aead-03

[Benjamin Kaduk <[e-mail]>]

On Fri, May 19, 2017 at 11:55:35AM -0400, Daniel Migault wrote:
> Hi Benjamin,
> 
> Thank you for the review. Please find my comments inline and let me know if
> you agree with the proposed text. I believe the only point not addressed
> concerns the addition of CCM-256 which has been remove after discussion
> during the WGLC.
> 
> Thanks you for the review,
> 
> Yours,
> Daniel
> 
> On Fri, May 19, 2017 at 12:38 AM, Benjamin Kaduk <[e-mail]> wrote:
> 
> > 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.
> >
> > MGLT: TLS_ECDHE_PSK_WITH_AES_256_CCM_8_SHA256 and
> TLS_ECDHE_PSK_WITH_AES_256_CCM_SHA384  were mentioned in the 01 and removed
> after  the WGLC. [0] The issue was whether or not introducing new ciphers
> not supported by TLS1.3. In 01, we though of adding the code point for
> TLS1.2 and then specify that TLS1.3 was only implementing a **subset** of
> the cipher suites proposed.In case of needs these could still be supported
> later by TLS1.3. The consensus seems to not introduce ciphers that would
> not be handled by TLS1.3. If the WG decides otherwise, these could still be
> added.
> 
> [0]
> https://mailarchive.ietf.org/arch/msg/tls/M442CwmUMxrYJR8FjCh3h-a69o4/?qid=6e4713be1d71bae6718c6e6e6c4b8007

That seems like a reasonable position for the WG to take, given how
close TLS 1.3 is to publication.  (It would also be reasonable to
define the full cross-product for TLS 1.2 only and have TLS 1.3 just
use a subset, but I have no real preference.)

> 
> > 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.)
> >
> 
> MGLT: I agree we coudl be more explicit, here are the changes I propose. I
> believe it address your purpose.
> 
> OLD:
> 
> Messages and pre-master secret construction in this
> document are based on [RFC4279 <https://tools.ietf.org/html/rfc4279>].
> 
> NEW:
> 
> Messages and pre-master secret construction in this
> document are defined in [RFC5489]. The ServerKeyExchange
> and ClientKeyExchange messages and premaster secret is
> computed as for  the ECDHE_PSK key exchange.

That basically works for me, though I'd tweak the phrasing slightly
for clarity/grammar:

NEW2:

Messages and pre-master secret construction in this
document are defined in [RFC5489]. The ServerKeyExchange
and ClientKeyExchange messages are used and the premaster secret is
computed as for the ECDHE_PSK key exchange.


> 
> > 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).
> >
> 
> MGLT: I believe the following text address your concern, however I am
> wondering if we are not going beyond the scope of expected considerations.

It perhaps is and perhaps is not worth mentioning, yes.

> In addition, it is worth noting that TLS1.0 <xref target="RFC2246"/> and
> TL1.2 <xref target="RFC4346"/> splits the premaster in two parts. The PRF
> results of mixing the two pseudorandom streams with distinct hash functions

"results of" is an unusual phrasing; "results from" might be more
familiar.

> (MD5 and SHA-1) by exclusive-ORing them together. In the case of ECDHE_PSK
> authentication, the PSK and pre-master are treated by distinct hash
> function with distinct properties. This may introduce vulnerabilities over
> the expected security provided by the constructed pre-master. As such
> TLS1.0 and TLS1.1 SHOULD NOT be used with ECDHE_PSK.

The general tenor of this text addresses my concern, yes, but feel
free to use editorial discretion and not bother putting it in, if
you don't think it's useful.  (BTW, I think we are still TLS1.0 and
TLS1.1 MUST NOT be used with these ciphersuites, so maybe the SHOULD
NOT in the last sentence is not quite what is intended.)

> 
> > 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.
> >
> 
> MGLT: I believe the following text address your comments.
> 
>  <t>GCM or CCM encryption - even of different clear text - re-using a
> nonce with a same key undermines the security of GCM and CCM. As a
> result, GCM and CCM MUST only be used with system guaranteeing nonce
> uniqueness <xref target="RFC5116"/>.</t>

Sure, sounds good.  (Grammar nit: "a system".)

> 
> >
> > Some other editorial nits follow.
> >
> 
> > 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.
> >
> 
> MGLT: I believe the following text address your concern:
> 
> OLD text:
> <t>AEAD algorithms that combine encryption and integrity protection are
> strongly recommended for <xref target="RFC7525" /> and non-AEAD algorithms
> are forbidden to use in TLS 1.3 <xref target="I-D.ietf-tls-tls13"/>.
> 
> NEW text:
> <t>AEAD algorithms that combine encryption and integrity protection are
> strongly recommended for (D)TLS <xref target="RFC7525" /> and non-AEAD
> algorithms
> are forbidden to use in TLS 1.3 <xref target="I-D.ietf-tls-tls13"/>.

Yes, thanks.

> > 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.
> >
> > MGLT: I believe the following text address your comment:
> 
> OLD:
> these cipher suites MUST NOT be negotiated in TLS  versions prior to 1.2.
> 
> NEW:
> the cipher suites defined in this document MUST NOT be negotiated in TLS
> versions prior to 1.2.
> 
>  OLD:
> Clients MUST NOT offer these cipher suites
> 
> NEW:
> Clients MUST NOT offer these cipher suites defined in this document

I think I only intended to comment about the first one, but there is
no harm in changing both.  Thanks!


-Ben