[Curdle] AD Review of draft-ietf-curdle-gss-keyex-sha2-05

[Eric Rescorla <[e-mail]>]

Rich version of this review at:
https://mozphab-ietf.devsvcdev.mozaws.net/D4544

This document has a huge amount of duplicated material which makes it
very hard to read. Please refactor so that the common material is in
one place.




COMMENTS
>      Due to security concerns with SHA-1 [RFC6194] and with MODP groups
>      with less than 2048 bits [NIST-SP-800-131Ar1]  we propose the use of
>      the SHA-2 based hashes with DH group14, group15, group16, group17 and
>      group18 [RFC3526].  Additionally we add support for key exchange
>      based on Elliptic Curve Diffie Hellman with NIST P-256, P-384 and
>      P-521 as well as X25519 and X448 curves.  Following the rationale of

"the X25519..."


>          +--------------------------+--------------------------------+
>          | Key Exchange Method Name | Implementation Recommendations |
>          +--------------------------+--------------------------------+
>          | gss-group14-sha256-*     | SHOULD/RECOMMENDED             |
>          | gss-group15-sha512-*     | MAY/OPTIONAL                   |
>          | gss-group16-sha512-*     | SHOULD/RECOMMENDED             |

Why are you only specifying SHA-512 with 4096-bit groups. SHA-256 is
still reasonable at that size?


>
>      Each of these methods specifies GSS-API-authenticated Diffie-Hellman
>      key exchange as described in Section 2.1 of [RFC4462]  with SHA-256
>      as HASH, and the group defined in Section 8.2 of [RFC4253] The method
>      name for each method is the concatenation of the string "gss-
>      group14-sha256-" with the Base64 encoding of the MD5 hash [RFC1321]

Why is this MD5? Is there some legacy reason for this? It's not
necessarily bad but it's odd to modern eyes.


>      Each of these methods specifies GSS-API-authenticated Diffie-Hellman
>      key exchange as described in Section 2.1 of [RFC4462]  with SHA-512
>      as HASH, and the group defined in Section 7 of [RFC3526] The method
>      name for each method is the concatenation of the string "gss-
>      group18-sha512-" with the Base64 encoding of the MD5 hash of the
>      ASN.1 DER encoding of the underlying GSS-API mechanism's OID.

These all seem to be boilerplate. is there a way to refactor into a
single paragraph with a table that describes the substitutions?


>      ASN.1 DER encoding of the underlying GSS-API mechanism's OID.
>
>   5.  New Elliptic Curve Diffie-Hellman Key Exchange methods
>
>      In [RFC5656] new SSH key exchange algorithms based on Elliptic Curve
>      Cryptography are introduced.  We reuse much of section 4 to implement

s/implement/define/


>      This section defers to [RFC7546] as the source of information on GSS-
>      API context establishment operations, Section 3 being the most
>      relevant.  All Security Considerations described in [RFC7546] apply
>      here too.
>
>      The Client:

This section should be refactored to put all the EC mechanics (which
are symmetrical) in one place.


>            and then y coordinate.  The coordinate coversion MUST preserve
>            leading zero octets.  Thus for nistp521 curve the encoded x
>            coordinate will always have a length of 66 octets while the Q_C
>            representation will be 133 octets long.  This is the
>            uncompressed representation specified in Section 4.3.6 of
>            [ANSI-X9-62-2005].

I have two questions about this:  1. Why are you specifying the
detailed computation of the public key? This seems like you could
defer it to another spec. 2. Why are you specifying uncompressed
representations for NIST curves? We did this in TLS because people
already supported them, but in general they are worse. Is there a
reason here?


>            by 31 zero octets for curve255519 and as an octect of value
>            0x05 followed by 55 zero octets.
>
>            Calculating Q_C as the result of the call to X25519 or X448
>            function, respectively for curve25519 and curve448 key
>            exchange, with parameters d_C and g.

This material all seems to be in RFC 7748 S 6.1 and 6.2.


>
>         For NIST curves, the server verifies that the q_C is not a point
>         at infinity, that both coordinates are in the interval [0, p - 1],
>         where p is the prime associated with the curve of the selected key
>         exchange and that the point lies on the curve (satisfies the curve
>         equation).

You should probably cite to the X9.62 or SP-800 for this procedure.


>         For curve25519, the server verifies that the the high-order bit of
>         the last octet is not set - this prevents distinguishing attacks
>         between implementations that use Montgomery ladder implementation
>         of the curve and ones that use generic elliptic-curve libraries.
>         If the bit is set, the key exchange SHOULD fail.  For curve448 any
>         bit can be set.

I'm not following what this is supposed to do. If you are worried
about this, why don't you just mask off the top bit.


>            For NIST curves, the peers perform point multiplication using
>            d_U and q_V to get point P.
>
>            For NIST curves, peers verify that P is not a point at
>            infinity.  If P is a point at infinity, the key exchange MUST
>            fail.

Why is this text here? It describes the client's behavior.


>            and q_V.  The result of the function is the shared secret.
>
>            For curve25519 and curve448, if all the octets of the shared
>            secret are zero octets, the key exchange MUST fail.
>
>         H = hash(V_C || V_S || I_C || I_S || K_S || Q_C || Q_S || K).

This kind of just comes out of nowhere. You probably want some
prefatory text.


>
>      7.  C verifies that the key Q_S is valid the same way it is done in
>      step 3.  If the key is not valid the key exchange MUST fail.
>
>      8.  C computes the shared secret K and H and verifies that it is
>      valid the same way it is done in step 5.  It then calls

This check only applies to CFRG curves.


>          string    server public host key and certificates (K_S)
>
>      Since this key exchange method does not require the host key to be
>      used for any encryption operations, this message is OPTIONAL.  If the
>      "null" host key algorithm described in Section 5 of [RFC4462] is
>      used, this message MUST NOT be sent.

I am assuming in this situation there is some other form of
authentication?


>          string    I_C, payload of the client's SSH_MSG_KEXINIT
>          string    I_S, payload of the server's SSH_MSG_KEXINIT
>          string    K_S, server's public host key
>          string    Q_C, client's ephemeral public key octet string
>          string    Q_S, server's ephemeral public key octet string
>          mpint     K,   shared secret

The actual equation is way up above this in the document, which is
presumably not great.


>      Each key exchange method is implicitly registered by this document.
>      The IESG is considered to be the owner of all these key exchange
>      methods; this does NOT imply that the IESG is considered to be the
>      owner of the underlying GSS-API mechanism.
>
>   5.2.1.  gss-nistp256-sha256-*

Again, can you refactor this section so it's not so duplicative.


>      the target the user intended.  Some mechanisms implementations (like
>      commonly used krb5 libraries) may use insecure DNS resolution to
>      canonicalize the target name; in these cases spoofing a DNS response
>      that points to an attacker-controlled machine may results in the user
>      silently delegating credentials to the attacker, who can then
>      impersonate the user at will.

Is this something new in this document?