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

[denis bider <[e-mail]>]

For the record, I think most of Eric's recommendations here are very
sensible.

On Fri, Apr 13, 2018 at 8:41 AM, Eric Rescorla <[e-mail]> 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
>> > >          +--------------------------+--------------------------------
>> +
>> > >
>> > >          | 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
>>
>
> Yes, OK.
>
>
>
>> > >      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
>>
>
> OK.
>
>
> > >      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.
>
>
>> > >      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.
>
>
>> > >            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.
>
>
>> > 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.
>
>
>
>> > >            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.
>
>
>
>> > >         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?
>
>
>> > >            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.
>
>
>
>> > >            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
>>
>
> 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>.
>
>
>
> > >          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)
>>
>
> I'll take a look at your revision and see what I think.
>
>
>> > >      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
>>
>
> Yes. and I have the same objection.
>
> -Ekr
>
>
>
> _______________________________________________
> Curdle mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/curdle
>
>