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

[Hubert Kario <[e-mail]>]

On Friday, 13 April 2018 15:41:37 CEST Eric Rescorla wrote:
> On Thu, Apr 12, 2018 at 11:29 AM, Hubert Kario <[e-mail]> wrote:
> > 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

> > >      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)
> 
> Well, I'm a pretty experienced programmer, and I find it pretty hard to
> follow.
> It's exactly this kind of boilerplate that leads to confusion.

done

> > > >      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...
> 
> Yes, That's why it's confusing to describe their operations in order rather
> than
> the behavior that a DH peer does and then just the points where they are
> inserted in the protocol. Compare, for instance, the TLS 1.3 specification,
> where both KeyShare (https://tools.ietf.org/html/d
> raft-ietf-tls-tls13-28#page-53) and
> the DH computations (https://tools.ietf.org/html/d
> raft-ietf-tls-tls13-28#section-7.4) are
> described in an endpoint agnostic manner. DH is inherently symmetrical.

the actions performed in context of GSSAPI-infused key exchange aren't
 
> > > >            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).
> 
> I think you're misunderstanding me. My point is that there are already
> documents
> which describe how to generate the private and public keys for EC. You
> should
> be referring to them, not recapitulating their contents here.

proposed in https://github.com/simo5/ietf/pull/24
 
> > > 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
> 
> Well, in TLS 1.3 this was an unfortunate concession to backward
> compatibility
> with a very widely deployed installed base. I'm trying to determine if
> that's true
> here. As for negotiation, this can be fixed by creating a new code point.

or can be fixed by mandating the most-widely supported format, and if somebody 
is interested in supporting compressed, he or she can propose that extension
 
> > > >            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.
> 
> It's also an opportunity for new mistakes to be made as these documents are
> less thoroughly reviewed than RFC 7748, as well as having two normative
> specifications for ostensible the same algorithm, which we try not to do.
> Please
> defer the algorithms to the original sources.

https://github.com/simo5/ietf/pull/24

> > > >         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
> 
> This is explicitly justified as "preventing distinguishing attacks". How
> does it do so?

removed

> > > >            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
> 
> Yes, that's why it's very confusing to have it in the middle of the
> server's operations.

the client part is referencing the server part
 
> > 
> > > >      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
> 
> Hmm... TLS 1.3 does not specify that one must validate the output of the DH
> computation. So, the IETF should be consistent on this point. If you think
> that
> TLS 1.3 is wrong, please explain why.
> 
> Second, the specific check you are requiring for the CFRG curves is the
> one applicable if you do the recommended DH computations. Here's the
> relevant text for TLS 13.
> 
>    For X25519 and X448, implementations SHOULD use the approach
>    specified in [RFC7748 <https://tools.ietf.org/html/rfc7748>] to
> calculate the Diffie-Hellman shared secret.
>    Implementations MUST check whether the computed Diffie-Hellman shared
>    secret is the all-zero value and abort if so, as described in
>    Section 6 of [RFC7748]
> <https://tools.ietf.org/html/rfc7748#section-6>.  If implementors use
> an alternative
>    implementation of these elliptic curves, they SHOULD perform the
>    additional checks specified in Section 7 of [RFC7748]
> <https://tools.ietf.org/html/rfc7748#section-7>.

Section 4.2.8.1:

   Peers MUST validate each other's public key Y by ensuring that 1 < Y
   < p-1.  This check ensures that the remote peer is properly behaved
   and isn't forcing the local system into a small subgroup.

and section 4.2.8.2 of draft-28:

   For the curves secp256r1, secp384r1 and secp521r1, peers MUST
   validate each other's public value Q by ensuring that the point is a
   valid point on the elliptic curve.  The appropriate validation
   procedures are defined in Section 4.3.7 of [X962] and alternatively
   in Section 5.6.2.3 of [KEYAGREEMENT].  This process consists of three
   steps: (1) verify that Q is not the point at infinity (O), (2) verify
   that for Q = (x, y) both integers x and y are in the correct
   interval, (3) ensure that (x, y) is a correct solution to the
   elliptic curve equation.  For these curves, implementers do not need
   to verify membership in the correct subgroup.

seem to me to be more relevant and quite detailed.

RFC 7748 says that the verification is optional, I don't see why making it 
mandatory is incorrect for SSH. For nist curves, the verification is about 
public key, not shared secret, it's just performed in the same step.


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