The previous code
if gex.g.Cmp(one) != 1 && gex.g.Cmp(pMinusOne) != -1 {
deobfuscates to the classic mistake
if g <= 1 && g >= p - 1 {
which is never true.
What the code actually intended to do is
if gex.g.Cmp(one) != 1 || gex.g.Cmp(pMinusOne) != -1 {
or more readably and consistently with the diffieHellman method
if gex.g.Cmp(one) <= 0 || gex.g.Cmp(pMinusOne) >= 0 {
Now, is this a security issue? The incorrect checks apply to g and k,
but not what we call Y and the spec calls f. RFC 4419 says:
Either side MUST NOT send or accept e or f values that are not in the
range [1, p-1]. If this condition is violated, the key exchange
fails. To prevent confinement attacks, they MUST accept the shared
secret K only if 1 < K < p - 1.
Note that RFC 8268, Section 4 updates the equivalent RFC 4253 statement
(although not the RFC 4419 one) about e and f, but we are already doing
the correct full check there.
DH Public Key values MUST be checked and both conditions:
1 < e < p-1
1 < f < p-1
MUST be true. Values not within these bounds MUST NOT be sent or
accepted by either side. If either one of these conditions is
violated, then the key exchange fails.
This simple check ensures that:
o The remote peer behaves properly.
o The local system is not forced into the two-element subgroup.
The check on K seems like a proxy for checking a number of ways to fix
the DH output (for example by manipulating g) to one of 0, 1, or -1.
This should not be meaningful to security for two reasons:
- all parameters end up in the "transcript" hash that will get signed
by the server's host key, and if the attacker controls the host
key's signature, they have the ability to MitM without resorting to
confinement attacks
- the client secret is ephemeral, so leaking bits of it by forcing it
into small sub-groups does not gain the attacker anything, as the
secret does not get reused
Indeed, this is the same explanation of why it's ok not to check that p
is indeed a (safe) prime, which even OpenSSH omits. Building an
equivalent attack by manipulating p instead of g is left as an exercise
to the reader.
For the future, this is a case study in why we should not add complexity
even when it looks easy enough to do. CL 174257 added the
diffie-hellman-group-exchange kex. That introduced a data race (arguably
a security issue), which was fixed in CL 222078. Then it was too slow,
which led to CL 252337 that removed the primalty check, which required a
full analysis of whether it's safe to skip it, and checking against
other implementations. Now we find there's a bug and we have to do
another security analysis that not even the RFC bothered to do in order
to decide if it's a security issue. My decision in
https://github.com/golang/go/issues/17230#issuecomment-489163656
does not look like the right one in hindsight.
While at it, clean up the code some
- drop useless bit size bounds logic in the server stub that get
ignored by the rest of the function
- make p and g local variables instead of method fields, since they
are not persistent state (this was originally a data race which was
fixed in CL 222078 by making Client not a pointer receiver)
Updates golang/go#17230
Change-Id: I4b1c68537109f627ccd75ec381dcfab57ce1768c
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/392015
Trust: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
RFC 9142 made diffie-hellman-group14-sha256 from RFC 8268 a MUST, and
it's strictly better than diffie-hellman-group14-sha1, which we already
have, and trivial to add.
> The method of key exchange used for the name "diffie-hellman-
> group14-sha256" is the same as that for "diffie-hellman-group14-sha1"
> except that the SHA256 hash algorithm is used.
Ignore the bigger groups which have a meaningful performance cost, and
don't share the same interoperability benefit.
Adapted from CL 387994.
Fixesgolang/go#31731
Co-authored-by: Nicola Murino <nicola.murino@gmail.com>
Change-Id: Id4ce345a2065840f193986739ea890f105a1e929
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/392014
Trust: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
The existing implementation validates that the prime returned by the server is, in fact, prime, which is extremely slow, especially for large key sizes.
As other implementations, including OpenSSH, do not verify the primality of the provided parameters, this change removes that check.
Fixesgolang/go#41151
Change-Id: I7539714c690f08b5792a0c540cbf46c3e81f13ba
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/252337
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Roland Shoemaker <roland@golang.org>
Add the diffie-hellman-group-exchange-sha256 defined in RFC 4419 to
the list of supported key exchange algorithms for ssh.
The server half is only a minimal implementation to satisfy the automated tests.
Fixesgolang/go#17230
Change-Id: I25880a564347fd9b4738dd2ed1e347cd5d2e21bb
GitHub-Last-Rev: 9f0b8d02c0c96e9baf00cdf1cf063ff834245443
GitHub-Pull-Request: golang/crypto#87
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/174257
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
None are "wrong" per se, but there are a lot of good suggestions and
in one case a docstring that was not present in godoc due to the
presence of an extra newline.
Changed "Id" in struct properties to "ID" in some non-exported
structs. Removed a trailing period from some error messages; I believe
the exact contents of error strings are not covered by the Go
compatibility promise.
Change-Id: I7c620582dc247396f72c52d38c909ccc0ec87b83
Reviewed-on: https://go-review.googlesource.com/80145
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Previously this code bounded the values to [1, p-1]. This protects
against invalid values that could take lots of CPU time to calculate
with. But the standard bounding is [2, p-2] so mirror that.
Since the DH exchange is signed anyway, this is not a security fix.
Change-Id: Ibef01805a596a433b0699d7a09c076344fa8c070
Reviewed-on: https://go-review.googlesource.com/30590
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This is done by running the key exchange and setting the session ID
under mutex. If the first exchange encounters an already set session
ID, then do nothing.
This fixes a race condition:
On setting up the connection, both sides sent a kexInit to initiate
the first (mandatory) key exchange. If one side was faster, the
faster side might have completed the key exchange, before the slow
side had a chance to send a kexInit. The slow side would send a
kexInit which would trigger a second key exchange. The resulting
confirmation message (msgNewKeys) would confuse the authentication
loop.
This fix removes sessionID from the transport struct.
This fix also deletes the unused interface rekeyingTransport.
Fixes#15066
Change-Id: I7f303bce5d3214c9bdd58f52d21178a185871d90
Reviewed-on: https://go-review.googlesource.com/21606
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
transport.
Sending the msgNewKeys packet and setting up the key material
now happen under a lock, preventing races with concurrent
writers.
R=kardianos, agl, jpsugar, hanwenn
CC=golang-dev
https://golang.org/cl/14476043
Adds readPacket() to conn, and renames conn to packetConn.
Key exchanges operate on packetConn, so they can be
unittested.
R=agl, jpsugar, dave
CC=golang-dev
https://golang.org/cl/13352055
Filippo Valsorda
Михаил ПатинandFilippo Valsorda
Daniel Cohen
Lucas Bremgartner
Lars Lehtonen
Kevin Burke
Dave Cheney
Adam Langley
Han-Wen Nienhuys
Carl Jackson
Jonathan Pittman