Subject: Re: [Curdle] Secdir last call review of draft-ietf-curdle-ssh-kex-sha2-14

From: Mališa Vučinić <[email-masked]>
Date: Mon, 01 Mar 2021 15:48:56 +0100

Thanks Ben, a couple of comments inline, enclosed within [MV] ... [/MV].

Mališa

On 28/02/2021 02:01, "Benjamin Kaduk" <[email-masked]> wrote:

    Hi Mališa,
    
    Thanks for the detailed review!
    
    I can only answer for some of the points, so hopefully Mark can chime in as
    needed.
    
    On Thu, Feb 25, 2021 at 06:14:18AM -0800, Mališa Vučinić via Datatracker wrote:
    > Reviewer: Mališa Vučinić
    > Review result: Has Issues
    > 
    > I 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
    > authors, document editors, and WG chairs should treat these comments just like
    > any other IETF Last Call comments.
    > 
    > I have found this document to have several issues related to the normative
    > language, imprecise wording, and inconsistent structure. Security
    > Considerations section is too generic and needs to detail the considerations of
    > using specific methods. My detailed comments are given below and I would
    > suggest a major editorial pass over the document.
    > 
    > Technical comments
    > ===================
    > 
    > Section 1.2.2.
    > “The minimum MODP group that MAY be used is the 2048-bit MODP group14.
    > Implementations SHOULD support a 3072-bit MODP group or larger.” - The use of
    > RFC2119 keywords here does not seem appropriate. These are the guiding
    > principles for specifying requirement levels of specific key exchange methods,
    > which are given in Section 3.
    > 
    > Section 1.2.3:
    > “The use of a SHA-2 Family hash with RSA 2048-bit keys has sufficient security.”
    > - Please define “sufficient” security
    > 
    > “The rsa1024-sha1 key exchange has less than 2048 bits and MUST NOT be
    > implemented.” - My view of Sections 1.2.1, 1.2.2 and 1.2.3  is that they give a
    > recap of public-key approaches with corresponding security levels when used
    > with different parameters. II am missing how a normative text on rsa1024-sha1
    > key exchange belongs here, and not to Section 3.4. Only in this section
    > (compared to 1.2.1 and 1.2.2) you reference specific SSH key exchange methods.
    > What does the security strength column in Table 3 refer to? The security level
    > of the RSA variant or of the hash function? Please make this consistent with
    > 1.2.1 and 1.2.2.
    
    It seems that an overarching consideration here is that we only need to
    make any given normative statement once in the document, and can refer to
    it or make other supporting statements as needed in other parts of the
    document.  I probably didn't pay as much care to this point in my review as
    I could have...

[MV] Agreed. [/MV]
    
    > Section 3.1:
    > “All of the key exchanges using the SHA-1 hashing algorithm should be
    > deprecated and phased out of use because SHA-1 has security concerns, as
    > documented in [RFC6194].” "It is reasonable to retain the
    > diffie-hellman-group14-sha1 exchange for interoperability with legacy
    > implementations.  The diffie-hellman-group14-sha1 key exchange MAY be
    > implemented.” - This text is conflicting with the statement just above that
    > SHA-1 should be deprecated, as you keep SHA1-based method as MAY for legacy
    > interop. Could you point me to the relevant discussion in the working group
    > where this decision has been made? Could you comment in the text on security
    > properties of the construct.
    
    The concern here is that diffie-hellman-group14-sha1 was one of only two
    "MUST implement" algorithms in the previous version of the spec, and that
    both (1) moving directly from "MUST" to "MUST NOT" is a huge leap that
    requires correspondingly strong justification; and (2) there was desire
    preserve the option to have backwards compatibility with non-updated
    implementations that, for example, only had the old MTI key-exchange
    method.
    
    There's some more extensive text discussing this that was produced in
    response to the genart review; the last consolidated version is at
    https://mailarchive.ietf.org/arch/msg/gen-art/GSb7vxgIDwu8ocOfiFwTRhTsoF0/
    but a few more edits were suggested in follow-up messages.
    
[MV] Thanks for the pointer and detailing the reasoning! [/MV]

    > “The SHA-2 Family of hashes [RFC6234] is the only one which is more secure than
    > SHA-1 and has been standardized for use with SSH key exchanges.” - “is the only
    > one which is more secure than SHA-1” seems too general of a statement, what
    > about SHA3?
    
    SHA3 is not standardized for use with SSH key exchange (yet).  The options
    are pretty limited when we limit it to just standardized ones.

[MV] 
This was really a subtle editorial comment: the statement read to me as (1) "SHA-2 is the only hash function which is more secure than SHA-1"; (2) "SHA-2 has in addition been standardized for use with SSH key exchanges". My comment was referring to (1). 

To me, one simple "that" would make a difference:

[OLD] The SHA-2 family of hashes [RFC6234] is the only one which is more secure than SHA-1 and has been standardized for use with SSH key exchanges. [/OLD]
[NEW] The SHA-2 family of hashes [RFC6234] is the only one which is more secure than SHA-1 and that has been standardized for use with SSH key exchanges. [/NEW]

I am not a native speaker so please feel free to disregard if this doesn't make any sense :)

[/MV]
    
    > Section 3.2.3:
    > - Title of the section got me confused. Sections 3.2.2 and 3.2.1 already
    > discuss ECDH-based key exchange methods. Could you make a clear differentiation
    > between the methods referenced in this section in terms of their cryptographic
    > core from those discussed in 3.2.2 and 3.2.1
    > 
    > “All of the NISTP curves named therein are mandatory to implement if any of
    > that RFC is implemented.” - Since this document does not update RFC 5656, I am
    > missing how it is appropriate to make this statement.
    
    My reading of this statement is that it is already a requirement of RFC
    5656 §10.1:
    
       Every SSH ECC implementation MUST support the named curves below.
       [table including nistp256, nistp384, and nistp521]

[MV] In the sense that the statement is just reiterating the requirement of RFC 5656 ? Sounds good. [/MV]
    
    > Section 3.3.1:
    > “This random selection from a set of pre-generated moduli for key exchange uses
    > SHA2-256 as defined in [RFC4419].” - RFC 4491 defines two key exchange methods,
    > diffie-hellman-group-exchange-sha1 using SHA1, and
    > diffie-hellman-group-exchange-sha256 using SHA-256. In this section you do not
    > mention the former. Phrasing “This key exchange MAY be used.” leaves ambiguity
    > which of the two methods you refer to.
    > 
    > Section 6:
    > - Security Considerations section is extremely generic and needs to be
    > improved. Referencing security considerations of the SSH protocol in RFC 4251
    > is appropriate, but since the point of this document is the update of the
    > requirement level of key exchange methods, the last paragraph should be
    > extended and more specific. While the discussion in Section 1.1 gives details
    > on the attacks on SHA-1, which can be referenced here, I would suggest
    > reiterating in the Security Considerations the practical consequences of using
    > the SHA-1 -based methods that are listed as SHOULD NOT, but also the
    > rsa1024-sha1 method that is listed as MUST NOT. Please stress the security
    > considerations of using diffie-hellman-group14-sha1 which is still a MAY.
    > 
    > Editorial comments and nits
    > ===================
    > 
    > - Please explicitly reference each table in the text. I would further suggest
    > adding a table in each section discussing new requirement levels of specific
    > key exchange methods, even if it would consist of a single entry, in order to
    > have a consistent structure.
    > 
    > Section 1:
    > “This document updates [RFC4250] [RFC4253] [RFC4432] [RFC4462] by changing the
    > requirement level ("MUST" moving to "SHOULD" or "MAY" or "SHOULD NOT", and
    > "MAY" moving to "MUST" or "SHOULD" or "SHOULD NOT" or "MUST NOT") of various
    > key-exchange mechanisms.” - The specific updates to these documents are spread
    > out throughout the text and pretty hard to grasp. It would be nice to see Table
    > 6 updated, by adding the reference RFC that is being updated, alongside the RFC
    > specifying the key exchange method, and maybe an old requirement level.
    > 
    > Section 1.1:
    > - introduce protocol parameters I_C and I_S
    
    These are standard protocol elements from RFC 4253; we should probably have
    a note at the top of the document that we freely use the terminology from
    that document.

[MV] Works for me, but adding "protocol parameters" in that sentence wouldn't hurt either. [/MV]
    
    -Ben
    
    > Section 3:
    > “and provides a suggested suitability for implementation of MUST, SHOULD,
    > SHOULD NOT, and MUST NOT.  Any method not explicitly listed MAY be
    > implemented.” - there are some explicit MAYs in the document, yet MAY is not
    > listed here.
    > 
    > Section 3.2.1:
    > “The use of SHA2-256 (also known as SHA-256 and sha256) as defined in [RFC6234]
    > for integrity is a reasonable one for this method” - “this” method not clear.
    > Be explicit and mention curve25519-sha256 key exchange method.
    > 
    > "it shares the same performance and security characteristics as curve25519-sha2”
    > - the key exchange method as standardized seems to be curve25519-sha256.
    > 
    > Section 3.2.2:
    > “It uses SHA2-512 (also known as SHA-512) defined in [RFC6234] for integrity.”
    > - “it” refers to the curve, not to the key exchange method. Rephrase to e.g.
    > corresponding key exchange method(s)
    > 
    > Section 3.2.1 and 3.2.2:
    > - Shuffle the content in these sections, first introduce the key exchange
    > methods, then talk about curves and hash function they use
    > 
    > Section 3.2.3:
    > - s/The are described in /These are described in
    > - s/Diffie Hellman/Diffie-Hellman
    > - s/elliptic curve Diffie Hellman/Elliptic-curve Diffie-Hellman
    > 
    > Section 3.3.1:
    > - You open a section with “This” which is confusing. Please rephrase.
    > - Add a table summarizing the requirement level per method, even if it consists
    > of a single method, in order to be consistent with other sections.
    > 
    > Section 3.3.2
    > - Please add some context and differentiation from Section 3.3.1
    > - Can you reference the RFC that specifies the methods discussed in this
    > section?
    > 
    > Section 3.5
    > “There are two key exchange methods, ext-info-c and ext-info-s, defined in
    > [RFC8308] which are not actually key exchanges.” - s/two key exchange
    > methods/two methods
    > 
    > 
    >

---

From: Mališa Vučinić <[email-masked]>
Date: Mon, 01 Mar 2021 15:59:06 +0100

Hi Mark,

Thanks, see a couple of comments inline, enclosed within [MV] ... [/MV].

Mališa

On 28/02/2021 18:26, "Mark D. Baushke" <[email-masked]> wrote:

    [CC- [email-masked]] This message really only addresses specific
    review comments...
    
    Hi Ben and Mališa and Rene,
    
    Benjamin Kaduk <[email-masked]> writes:
    
    > Hi 
    
    I will do what I can to address the points raised by everyone.
    
    I do not believe that I can introduce anything about SHA-3 hashing as
    that is not being used by Secure Shell key exchanges today and this is
    really only a survey of the existing key exchanges.

[MV] The comment on SHA-3 was really of an editorial nature as the sentence read too general, see the response to Ben for an easy resolution. [/MV]
    
    I will try to do a better job about the objective requirements.
    
    For example, the use of a 2048-bit MODP group with 112 bits of security
    is fine to protect a 3des-cbc 112 bits of security symmetric cipher.
    However, it is not sufficient to protect an aes128 bit key with 128 bits
    of security. Likewise, a SHA-1 hash with 80 bits or less of security is
    not really good to protect even the 3des-cbc cipher.
    
    The reason I recommended that a 3072-bit MODP group be used was to
    protect aes128 keys. I will try to make that point more explicit.
    
    Thank you for all of the comments. I have to find time to incorporate
    them.
    
    With regard to Rene's question about changing Table 6...
    
    Rene wrote:
    
    > Section 1: “This document updates [RFC4250] [RFC4253] [RFC4432]
    > [RFC4462] by changing the requirement level ("MUST" moving to "SHOULD"
    > or "MAY" or "SHOULD NOT", and "MAY" moving to "MUST" or "SHOULD" or
    > "SHOULD NOT" or "MUST NOT") of various key-exchange mechanisms.” - The
    > specific updates to these documents are spread out throughout the text
    > and pretty hard to grasp. It would be nice to see Table 6 updated, by
    > adding the reference RFC that is being updated, alongside the RFC
    
    Table 6 presently has the kex name, RFC reference, and the
    implementation guidance for the method. This is intended to be what
    becomes the IANA table for key exchange method names. I am not entirely
    sure I understand what is desired. Some methods do not have any language
    about MUST, SHOULD, or MAY implement. So, the previous history is not
    really defined. This is how the table might look if I list "none" for
    those that are not called out in their RFCs:

[MV] 
I believe this was a comment I made. The underlying issue here is that when I was going through the doc, I had a hard time understanding the specific updates to the referenced RFCs. The proposal above was a means to resolve that. If you find that too clumsy, feel free to propose any other way of making it explicit what exactly is being updated within those documents.
[/MV]
    
       +==========================+===========+================+===========+
       | Key Exchange Method      | Reference | Previous       | Implement |
       | Name                     |           | Recommendation |           |
       +==========================+===========+================+===========+
       | curve25519-sha256        | RFC8731   | none           | SHOULD    |
       +--------------------------+-----------+----------------+-----------+
       | curve448-sha512          | RFC8731   | none           | MAY       |
       +--------------------------+-----------+----------------+-----------+
       | diffie-hellman-group-    | RFC4419   | none           | SHOULD    |
       | exchange-sha1            | RFC8270   |                | NOT       |
       +--------------------------+-----------+----------------+-----------+
       | diffie-hellman-group-    | RFC4419   | none           | MAY       |
       | exchange-sha256          | RFC8720   |                |           |
       +--------------------------+-----------+----------------+-----------+
       | diffie-hellman-          | RFC4253   | MUST           | SHOULD    |
       | group1-sha1              |           |                | NOT       |
       +--------------------------+-----------+----------------+-----------+
       | diffie-hellman-          | RFC4253   | MUST           | MAY       |
       | group14-sha1             |           |                |           |
       +--------------------------+-----------+----------------+-----------+
       | diffie-hellman-          | RFC8268   | none           | MUST      |
       | group14-sha256           |           |                |           |
       +--------------------------+-----------+----------------+-----------+
       | diffie-hellman-          | RFC8268   | none           | MAY       |
       | group15-sha512           |           |                |           |
       +--------------------------+-----------+----------------+-----------+
       | diffie-hellman-          | RFC8268   | none           | SHOULD    |
       | group16-sha512           |           |                |           |
       +--------------------------+-----------+----------------+-----------+
       | diffie-hellman-          | RFC8268   | none           | MAY       |
       | group17-sha512           |           |                |           |
       +--------------------------+-----------+----------------+-----------+
       | diffie-hellman-          | RFC8268   | none           | MAY       |
       | group18-sha512           |           |                |           |
       +--------------------------+-----------+----------------+-----------+
       | ecdh-sha2-*              | RFC5656   | MAY            | MAY       |
       +--------------------------+-----------+----------------+-----------+
       | ecdh-sha2-nistp256       | RFC5656   | MUST           | SHOULD    |
       +--------------------------+-----------+----------------+-----------+
       | ecdh-sha2-nistp384       | RFC5656   | MUST           | SHOULD    |
       +--------------------------+-----------+----------------+-----------+
       | ecdh-sha2-nistp521       | RFC5656   | MUST           | SHOULD    |
       +--------------------------+-----------+----------------+-----------+
       | ecmqv-sha2               | RFC5656   | MAY            | MAY       |
       +--------------------------+-----------+----------------+-----------+
       | ext-info-c               | RFC8308   | SHOULD         | SHOULD    |
       +--------------------------+-----------+----------------+-----------+
       | ext-info-s               | RFC8308   | SHOULD         | SHOULD    |
       +--------------------------+-----------+----------------+-----------+
       | gss-*                    | RFC4462   | none           | MAY       |
       +--------------------------+-----------+----------------+-----------+
       | gss-                     | RFC8732   | SHOULD         | SHOULD    |
       | curve25519-sha256-*      |           |                |           |
       +--------------------------+-----------+----------------+-----------+
       | gss-curve448-sha512-*    | RFC8732   | MAY            | MAY       |
       +--------------------------+-----------+----------------+-----------+
       | gss-gex-sha1-*           | RFC4462   | none           | SHOULD    |
       |                          |           |                | NOT       |
       +--------------------------+-----------+----------------+-----------+
       | gss-group1-sha1-*        | RFC4462   | none           | SHOULD    |
       |                          |           |                | NOT       |
       +--------------------------+-----------+----------------+-----------+
       | gss-group14-sha256-*     | RFC8732   | none           | SHOULD    |
       +--------------------------+-----------+----------------+-----------+
       | gss-group15-sha512-*     | RFC8732   | none           | MAY       |
       +--------------------------+-----------+----------------+-----------+
       | gss-group16-sha512-*     | RFC8732   | none           | MAY       |
       +--------------------------+-----------+----------------+-----------+
       | gss-group17-sha512-*     | RFC8732   | none           | MAY       |
       +--------------------------+-----------+----------------+-----------+
       | gss-group18-sha512-*     | RFC8732   | none           | MAY       |
       +--------------------------+-----------+----------------+-----------+
       | gss-nistp256-sha256-*    | RFC8732   | none           | SHOULD    |
       +--------------------------+-----------+----------------+-----------+
       | gss-nistp384-sha384-*    | RFC8732   | none           | SHOULD    |
       +--------------------------+-----------+----------------+-----------+
       | gss-nistp521-sha512-*    | RFC8732   | none           | SHOULD    |
       +--------------------------+-----------+----------------+-----------+
       | rsa1024-sha1             | RFC4432   | none           | MUST NOT  |
       +--------------------------+-----------+----------------+-----------+
       | rsa2048-sha256           | RFC4432   | none           | MAY       |
       +--------------------------+-----------+----------------+-----------+
    
    I am not sure if this table is now too busy to be in the IANA table or
    not and ask for suggestions to make the IANA table more useful.
    
    My goal is to upload a -15 revision of the document on or soon after
    March 8th when submissions are open again.

[MV] Great, thanks! [/MV]
    
    	Be safe, stay healthy,
    	-- Mark