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

[Simo Sorce <[e-mail]>]

Chiming in as one of the Authors, sorry for not responding earlier, I've bene
busy and I will follow up with more answers later on as I ge tmore time to
address them.

On Fri, 2018-04-06 at 16:24 -0700, Eric Rescorla wrote:
> 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.

In general we strived to maintain the same structure as the docuemnt we are
extending. This is why some sections seem repetitive. They are but they also
are consistent with the previous document. We think this is more beneficial
than trying to be more synthetic.

> 
> 
> 
> 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..."

Does this imply it should also be "the NIST P-256, ..." ?

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

I thik this has been sactisfactorily discussed later in the thread.
 
> 
> > 
> >      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.

Also discussed later, this is just an identifier, and we a re just being
consistent with previous naming which can be hardcoded, no need to implement
MD5.

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

See my explanantion above.

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

Is there a reason? We wanted to give implementers a clear linear explanation
without having to jump back and forth to reference what is happening at any
given point.

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

I think this is because we did not have any good normative reference, but I'll
let Hubert chime in.

> 2. Why are you specifying uncompressed
> representations for NIST curves?

I may be wrong but some compression algorithms have had IPR issues, so this may
derive from a willingness to avoid those, I'll let Hubert chime in here too.

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

I think we wanted it explicitly here, to avoid too much referencing and risk of
errors, but I guess we can reference if you feel strongly about this.
 
> > 
> >         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?

As explained elswhere, GSS key exchange assumes GSSAPI 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?

No, we just thought important to note, as it was missing in the original
document but it is a know factor to be aware of when dealing with Krb5 which is
de facto the only used GSSAPI mechanism for SHH GSS Key exchanges.

Simo.

-- 
Simo Sorce
Sr. Principal Software Engineer
Red Hat, Inc