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

[Eric Rescorla <[e-mail]>]

On Wed, Apr 25, 2018 at 11:53 AM, Hubert Kario <[e-mail]> wrote:

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

Neither are they in TLS, but we managed to put the DH part in one place.

> 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


I will review this.


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

Yes, but then you have both.


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



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

Yes, that's why I said you should put all the DH operations in one place.


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

I think we're talking past each other. The TLS spec requires that:

- You do public key validation for FFDHE and NIST curves
- You do output validation for CFRG curves.

Unless I am misreading your text, you are requiring that you also do output
validation for the NIST curves. Is that correct? If so, can you provide a
source for why?

-Ekr