[secdir] Secdir last call review of draft-ietf-curdle-ssh-kex-sha2-14

[Mališa Vučinić via Datatracker <[e-mail]>]

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.

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 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?

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.

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

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