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.

Sign up for GitHub

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

Jump to bottom

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

Closed
rolandshoemaker opened this issue Feb 22, 2023 · 8 comments
Closed

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

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

Comments

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Feb 22, 2023 •
edited by dmitshur
Loading

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
Contributor

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

@gopherbot
Copy link
Contributor

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

This was referenced Feb 24, 2023
Closed
Closed
@gopherbot
Copy link
Contributor

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.

@dmitshur dmitshur added this to the Go1.21 milestone Feb 24, 2023
@FiloSottile FiloSottile mentioned this issue Feb 24, 2023
Closed
@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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
@FiloSottile @gopherbot
[release-branch.go1.20] crypto/internal/nistec: reduce P-256 scalar
602eeaa 
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 <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Run-TryBot: Filippo Valsorda <[email protected]>
Auto-Submit: Filippo Valsorda <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
(cherry picked from commit 203e59a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/471695
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
gopherbot pushed a commit that referenced this issue Feb 27, 2023
@FiloSottile @gopherbot
[release-branch.go1.19] crypto/internal/nistec: reduce P-256 scalar
639b67e 
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 <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Run-TryBot: Filippo Valsorda <[email protected]>
Auto-Submit: Filippo Valsorda <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
(cherry picked from commit 203e59a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/471696
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Mar 3, 2023
@FiloSottile @romaindoumenc
[release-branch.go1.20] crypto/internal/nistec: reduce P-256 scalar
505b971 
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 <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Run-TryBot: Filippo Valsorda <[email protected]>
Auto-Submit: Filippo Valsorda <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
(cherry picked from commit 203e59a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/471695
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
@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
@Kausik-A Kausik-A mentioned this issue Mar 7, 2023
Merged
@kke kke mentioned this issue Mar 7, 2023
Merged
16 tasks
twz123 added a commit to twz123/k0s that referenced this issue Mar 8, 2023
@twz123
Bump go to v1.19.7
f30574e 
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 <[email protected]>
@twz123 twz123 mentioned this issue Mar 8, 2023
Merged
16 tasks
@Kausik-A Kausik-A mentioned this issue Mar 8, 2023
Merged
schmichael added a commit to hashicorp/nomad that referenced this issue Mar 10, 2023
@schmichael
build: update from go1.20.1 to go1.20.2
622f703 
Note that the CVE fixed in go1.20.2 does *not* impact Nomad.

golang/go#58647
@schmichael schmichael mentioned this issue Mar 10, 2023
Merged
schmichael added a commit to hashicorp/nomad that referenced this issue Mar 13, 2023
@schmichael
build: update from go1.20.1 to go1.20.2 (#16427)
5f37b2f 
* 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
This was referenced Mar 13, 2023
Merged
Merged
Merged
gopherbot pushed a commit that referenced this issue Mar 13, 2023
@FiloSottile @gopherbot
crypto/internal/nistec: refactor scalar multiplication
778627f 
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 <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Run-TryBot: Filippo Valsorda <[email protected]>
Auto-Submit: Filippo Valsorda <[email protected]>
Reasonable-Solutions added a commit to nais/teams-backend that referenced this issue Mar 14, 2023
@Reasonable-Solutions
bump go version to 1.20.2
925bdab 
as per golang/go#58647
Closed
@k0s-bot k0s-bot mentioned this issue Mar 16, 2023
Merged
k0s-bot pushed a commit to k0sproject/k0s that referenced this issue Mar 16, 2023
@twz123 @github-actions
Bump go to v1.19.7
38ab613 
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 <[email protected]>
(cherry picked from commit f30574e)
@zafs23 zafs23 mentioned this issue Mar 20, 2023
Closed
@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.

@kencochrane kencochrane mentioned this issue Mar 28, 2023
Closed
twz123 added a commit to twz123/k0s that referenced this issue Mar 29, 2023
@twz123
Bump go to v1.19.7
f9669a9 
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 <[email protected]>
(cherry picked from commit f30574e)
(cherry picked from commit 38ab613)
@guidovranken guidovranken mentioned this issue Apr 21, 2023
Closed
@guidovranken guidovranken mentioned this issue Jun 6, 2023
Merged
AdamKorcz pushed a commit to google/oss-fuzz that referenced this issue Jun 8, 2023
@guidovranken @oliverchang
[cryptofuzz] Use Golang development version (#10480)
11a6ac2 
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 <[email protected]>
@FiloSottile FiloSottile mentioned this issue Jun 10, 2023
Open
@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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Security
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
6 participants
@FiloSottile @prattmic @dmitshur @rolandshoemaker @gopherbot @thanm