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

[Hubert Kario <[e-mail]>]

Sorry for the delay in replying was swamped with other work, I should have no 
problem replying quickly now.

On Saturday, 7 April 2018 01:24:27 CEST 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.
> 
> 
> 
> 
> COMMENTS
> >          +--------------------------+--------------------------------+
> >          
> >          | 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 think that was already discussed at length - short version: mirroring of the 
regular (non-gss) version of those KEX methods
 
> >      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.

it has no impact on security, it's used only to limit length of the method 
name and because MD5 was always used for that

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

while valid point, it follows the style established in RFC 4462

for a programmer familiar with the old gss KEX methods and general IETF 
terminology, the names of the new algorithms alone are sufficient to implement 
them

for a programmer just learning it, it's sufficiently detailed to hand-hold the 
implementation process (and resolve disputes in case of minor differences)

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

I don't think I understand what changes you'd like to see

both FFDH and ECDH are symmetrical... both client and server need to perform 
the same operations...

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

because this allows to verify the sanity of the share when it is being parsed, 
before the cryptographic routines are processing it, limiting the attack area
(and the implementation may want to handle a clearly malformed key share
and a invalid-value malformed key share differently).

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

same reason – the uncompressed is the only format that is guaranteed to be 
supported and there is no mechanism in SSH to negotiate the supported formats

e.g. the implementation shipped in Red Hat Enterprise Linux does not support 
compressed representations

since it's also the same (and only) format used in TLS 1.3, it allows better 
cryptographic code reuse

(as Simo pointed out, because the standards allow for compressed 
representation, neither is explicit in expected sizes of serialised point 
representation so they are listed here, also as a simple sanity check to 
perform)

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

we do need local nomenclature for the inputs and outputs though

also, having all the necessary checks in a single document allows for easier 
code review and verification if they are performed.

I opted for verbosity in the name of security. RFC 7748 is already referenced 
in the definitions of the methods.

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

+1

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

because debugging mismatch in key calculation is harder to do than rejection 
of a malformed message

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

both client and server need to perform that operation
 
> >            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.

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

no, for CFRG curves the invalid value is a point at infinity, for X25519 
invalid value is an all-zero string

so the check if the shared secret is valid must be performed irrespective of 
curve used
 
> >          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?

yes, Kerberos is not the sole implementation of GSS API
 
> >          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.

 1. those are equivalent
 2. this approach is inherited from RFC 4462; intention being to quickly show 
    no difference to the underlying inputs (for FFDHE only the hash, prime 
    and generators are changed)
 
> >      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.

same replay as with FFDHE methods

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

Yes, those are additional security considerations, from discussions with Ben 
on Curve-WG ML.

-- 
Regards,
Hubert Kario
Senior Quality Engineer, QE BaseOS Security team
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkyňova 115, 612 00  Brno, Czech Republic