Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/elliptic: specific unreduced P-256 scalars produce incorrect results (CVE-2023-24532) #58647

Closed
rolandshoemaker opened this issue Feb 22, 2023 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone

Comments

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Feb 22, 2023

In particular (in psuedocode)

x = 48439561293906451759052585252797914202762949526041747995844080717082404635286
y = 36134250956749795798585127919587881956611106672985015071877198253568414405109

P256().ScalarMult(x, y, 30) != P256().ScalarMult(x, y, N + 30)

Thanks to Guido Vranken for reporting this issue via the Ethereum Foundation bug
bounty program.

This is CVE-2023-24532 and Go issue https://go.dev/issue/58647 (this one).

@thanm thanm added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 23, 2023
@FiloSottile
Copy link
Contributor

We agreed with @rolandshoemaker that this can be fixed as PUBLIC since it only affects niche configurations, namely very specific direct uses of crypto/elliptic. We found no real world protocol that could be attacked due to this.

@FiloSottile FiloSottile changed the title security: fix CVE-2023-24532 crypto/elliptic: specific unreduced P-256 scalars produce incorrect results Feb 24, 2023
@gopherbot
Copy link

Change https://go.dev/cl/471256 mentions this issue: crypto/internal/nistec: refactor scalar multiplication

@gopherbot
Copy link

Change https://go.dev/cl/471255 mentions this issue: crypto/internal/nistec: reduce P-256 scalar

@FiloSottile
Copy link
Contributor

@gopherbot please backport. CL 471255 is a security fix.

/cc @golang/security and @golang/release

@gopherbot
Copy link

Backport issue(s) opened: #58719 (for 1.19), #58720 (for 1.20).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link

Change https://go.dev/cl/471695 mentions this issue: [release-branch.go1.20] crypto/internal/nistec: reduce P-256 scalar

@gopherbot
Copy link

Change https://go.dev/cl/471696 mentions this issue: [release-branch.go1.19] crypto/internal/nistec: reduce P-256 scalar

gopherbot pushed a commit that referenced this issue Feb 27, 2023
Unlike the rest of nistec, the P-256 assembly doesn't use complete
addition formulas, meaning that p256PointAdd[Affine]Asm won't return the
correct value if the two inputs are equal.

This was (undocumentedly) ignored in the scalar multiplication loops
because as long as the input point is not the identity and the scalar is
lower than the order of the group, the addition inputs can't be the same.

As part of the math/big rewrite, we went however from always reducing
the scalar to only checking its length, under the incorrect assumption
that the scalar multiplication loop didn't require reduction.

Added a reduction, and while at it added it in P256OrdInverse, too, to
enforce a universal reduction invariant on p256OrdElement values.

Note that if the input point is the infinity, the code currently still
relies on undefined behavior, but that's easily tested to behave
acceptably, and will be addressed in a future CL.

Updates #58647
Fixes #58720
Fixes CVE-2023-24532

(Filed with the "safe APIs like complete addition formulas are good" dept.)

Change-Id: I7b2c75238440e6852be2710fad66ff1fdc4e2b24
Reviewed-on: https://go-review.googlesource.com/c/go/+/471255
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
(cherry picked from commit 203e59a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/471695
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
gopherbot pushed a commit that referenced this issue Feb 27, 2023
Unlike the rest of nistec, the P-256 assembly doesn't use complete
addition formulas, meaning that p256PointAdd[Affine]Asm won't return the
correct value if the two inputs are equal.

This was (undocumentedly) ignored in the scalar multiplication loops
because as long as the input point is not the identity and the scalar is
lower than the order of the group, the addition inputs can't be the same.

As part of the math/big rewrite, we went however from always reducing
the scalar to only checking its length, under the incorrect assumption
that the scalar multiplication loop didn't require reduction.

Added a reduction, and while at it added it in P256OrdInverse, too, to
enforce a universal reduction invariant on p256OrdElement values.

Note that if the input point is the infinity, the code currently still
relies on undefined behavior, but that's easily tested to behave
acceptably, and will be addressed in a future CL.

Updates #58647
Fixes #58719
Fixes CVE-2023-24532

(Filed with the "safe APIs like complete addition formulas are good" dept.)

Change-Id: I7b2c75238440e6852be2710fad66ff1fdc4e2b24
Reviewed-on: https://go-review.googlesource.com/c/go/+/471255
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
(cherry picked from commit 203e59a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/471696
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Mar 3, 2023
Unlike the rest of nistec, the P-256 assembly doesn't use complete
addition formulas, meaning that p256PointAdd[Affine]Asm won't return the
correct value if the two inputs are equal.

This was (undocumentedly) ignored in the scalar multiplication loops
because as long as the input point is not the identity and the scalar is
lower than the order of the group, the addition inputs can't be the same.

As part of the math/big rewrite, we went however from always reducing
the scalar to only checking its length, under the incorrect assumption
that the scalar multiplication loop didn't require reduction.

Added a reduction, and while at it added it in P256OrdInverse, too, to
enforce a universal reduction invariant on p256OrdElement values.

Note that if the input point is the infinity, the code currently still
relies on undefined behavior, but that's easily tested to behave
acceptably, and will be addressed in a future CL.

Updates golang#58647
Fixes golang#58720
Fixes CVE-2023-24532

(Filed with the "safe APIs like complete addition formulas are good" dept.)

Change-Id: I7b2c75238440e6852be2710fad66ff1fdc4e2b24
Reviewed-on: https://go-review.googlesource.com/c/go/+/471255
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
(cherry picked from commit 203e59a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/471695
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
@dmitshur dmitshur changed the title crypto/elliptic: specific unreduced P-256 scalars produce incorrect results crypto/elliptic: specific unreduced P-256 scalars produce incorrect results (CVE-2023-24532) Mar 3, 2023
twz123 added a commit to twz123/k0s that referenced this issue Mar 8, 2023
schmichael added a commit to hashicorp/nomad that referenced this issue Mar 10, 2023
Note that the CVE fixed in go1.20.2 does *not* impact Nomad.

golang/go#58647
schmichael added a commit to hashicorp/nomad that referenced this issue Mar 13, 2023
* build: update from go1.20.1 to go1.20.2

Note that the CVE fixed in go1.20.2 does *not* impact Nomad.

golang/go#58647
gopherbot pushed a commit that referenced this issue Mar 13, 2023
The assumptions of some of the assembly functions were still scarcely
documented and even disregarded: p256ScalarMult was relying on the fact
that the "undefined behavior" of p256PointAddAsm with regards to
infinity inputs was returning the infinity.

Aside from expanding comments, moving the bit window massaging into a
more easily understood p256OrdRsh function, and fixing the above, this
change folds the last iteration of p256ScalarMult into the loop to
reduce special cases and inverts the iteration order of p256BaseMult so
it matches p256ScalarMult for ease of comparison.

Updates #58647

Change-Id: Ie5712ea778aadbe5adcdb478d111c2527e83caa0
Reviewed-on: https://go-review.googlesource.com/c/go/+/471256
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reasonable-Solutions added a commit to nais/teams-backend that referenced this issue Mar 14, 2023
k0s-bot pushed a commit to k0sproject/k0s that referenced this issue Mar 16, 2023
@prattmic
Copy link
Member

@FiloSottile FYI https://go.dev/cl/471256 caused a minor regression in one benchmark: https://perf.golang.org/dashboard/?benchmark=GenerateKeyP256-8&unit=sec/op#commit778627f33187d874440ce1f353bb4d7bce55304a

Given this is minor, part of a CVE fix, and presumably in a crypto microbenchmark, I don't think this matters much, just raising it in case you care.

The benchmark is https://github.com/ethereum/go-ethereum/blob/v1.10.9/crypto/ecies/ecies_test.go#L166.

twz123 added a commit to twz123/k0s that referenced this issue Mar 29, 2023
https://groups.google.com/g/golang-announce/c/3-TpUx48iQY
https://go.dev/doc/devel/release#go1.19.7

Fixes:

* CVE-2023-24532 golang/go#58647

Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
(cherry picked from commit f30574e)
(cherry picked from commit 38ab613)
AdamKorcz pushed a commit to google/oss-fuzz that referenced this issue Jun 8, 2023
Switch Go from the one that the OSS-Fuzz image supplies via
`install_go.sh` to the latest development version cloned from Git.

The OSS-Fuzz Go is an older version which still has
[CVE-2023-24532](golang/go#58647) which keeps
getting found by the fuzzer. Additionaly by using the latest upstream
version, bugs in Go will be detected quickly after being introduced.

Additionally this PR fixes the 32 bit build. The 32 bit build is
currently not enabled on OSS-Fuzz because it leads to OOM bugs, but I
still like to build it myself for local testing.

---------

Co-authored-by: Oliver Chang <oliverchang@users.noreply.github.com>
@golang golang locked and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Security
Projects
None yet
Development

No branches or pull requests

6 participants