Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
ssh: fix diffie-hellman-group-exchange g and K bounds checks
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 golang/go#17230 (comment) 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 <[email protected]> Run-TryBot: Filippo Valsorda <[email protected]> Reviewed-by: Roland Shoemaker <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
- Loading branch information