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 <kaduk@xxxxxxx> 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