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

math/big: (*Rat).SetString with "1.770p02041010010011001001" crashes with "makeslice: len out of range" #45910

Closed
odeke-em opened this issue May 2, 2021 · 10 comments
Closed

math/big: (*Rat).SetString with "1.770p02041010010011001001" crashes with "makeslice: len out of range" #45910

odeke-em opened this issue May 2, 2021 · 10 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.17

Comments

@odeke-em
Copy link
Member

odeke-em commented May 2, 2021

What version of Go are you using (go version)?

Go since Go1.13 when we added support for hexadecimal literals

Does this issue reproduce with the latest release?

Yes!

What operating system and processor architecture are you using (go env)?

Not applicable, present on all versions

What did you do?

Found by oss-fuzz https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=33284, I ran the code in https://play.golang.org/p/pM4ROalIRvq or inlined below

package main

import "math/big"

func main() {
	r := new(big.Rat)
	r.SetString("1.770p02041010010011001001")
}

What did you expect to see?

No crash but the boolean value returned as false.

What did you see instead?

A crash resulting from us having passed in 31890781406421892 in make.

$ go run main.go 
panic: runtime error: makeslice: len out of range

goroutine 1 [running]:
math/big.nat.make(...)
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/math/big/nat.go:69
math/big.nat.shl({0xc000136008, 0x0, 0x0}, {0xc000136008, 0xc000140000, 0x0}, 0x113b270)
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/math/big/nat.go:1010 +0x213
math/big.(*Rat).SetString(0xc00012bf30, {0x10b527e, 0xc0000001a0})
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/math/big/ratconv.go:188 +0x756
main.main()
	/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/oss-fuzz/33284/main.go:7 +0x49
exit status 2

We should perhaps cap the value that we pass into make to avoid an OOM.
Kindly cc-ing @griesemer @findleyr @katiehockman @rolandshoemaker @FiloSottile
@griesemer griesemer self-assigned this May 2, 2021
@griesemer griesemer added this to the Go1.17 milestone May 2, 2021
@griesemer griesemer added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 2, 2021
@gopherbot
Copy link

Change https://golang.org/cl/316149 mentions this issue: math/big: check for excessive exponents in Rat.SetString

@odeke-em
Copy link
Member Author

odeke-em commented May 6, 2021

Should we perhaps backport this change?

@griesemer
Copy link
Contributor

cc: @FiloSottile @katiehockman re: backporting decision.

@odeke-em
Copy link
Member Author

odeke-em commented May 7, 2021

And another fuzz reproduction that caused an OOM "1p10000000000" with a 2.5GiB RAM allocation per https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=33286 which is fixed by the change that fixed this. Thank you @griesemer!

@ianlancetaylor
Copy link
Contributor

In general, I would vote against backporting fixes that are found by fuzzing if there are no security implications.

In this particular case I suppose there might potentially be a security implication if some server accepts user input and passes it to (*Rat).SetString. So perhaps we should backport it for the next minor releases.

@katiehockman
Copy link
Contributor

The @golang/security team is looking into this. If we determine that there are security implications (which likely there are), then I'll take care of getting this backported and into the next minor release.

@katiehockman
Copy link
Contributor

@gopherbot please consider this for backport to 1.15 and 1.16 as this is a security issue.

@gopherbot gopherbot mentioned this issue May 21, 2021
Closed
@gopherbot
Copy link

Backport issue(s) opened: #46305 (for 1.15), #46306 (for 1.16).

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

@gopherbot gopherbot mentioned this issue May 21, 2021
Closed
@gopherbot
Copy link

Change https://golang.org/cl/321831 mentions this issue: [release-branch.go1.15] math/big: check for excessive exponents in Rat.SetString

@gopherbot
Copy link

Change https://golang.org/cl/321832 mentions this issue: [release-branch.go1.16] math/big: check for excessive exponents in Rat.SetString

gopherbot pushed a commit that referenced this issue May 27, 2021
@griesemer @katiehockman
[release-branch.go1.15] math/big: check for excessive exponents in Ra…
df9ce19 
…t.SetString

Found by OSS-Fuzz https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=33284

Thanks to Emmanuel Odeke for reporting this issue.

Updates #45910
Fixes #46305
Fixes CVE-2021-33198

Change-Id: I61e7b04dbd80343420b57eede439e361c0f7b79c
Reviewed-on: https://go-review.googlesource.com/c/go/+/316149
Trust: Robert Griesemer <gri@golang.org>
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
(cherry picked from commit 6c591f7)
Reviewed-on: https://go-review.googlesource.com/c/go/+/321831
Run-TryBot: Katie Hockman <katie@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
gopherbot pushed a commit that referenced this issue May 27, 2021
@griesemer @katiehockman
[release-branch.go1.16] math/big: check for excessive exponents in Ra…
9210eaf 
…t.SetString

Found by OSS-Fuzz https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=33284

Thanks to Emmanuel Odeke for reporting this issue.

Updates #45910
Fixes #46306
Fixes CVE-2021-33198

Change-Id: I61e7b04dbd80343420b57eede439e361c0f7b79c
Reviewed-on: https://go-review.googlesource.com/c/go/+/316149
Trust: Robert Griesemer <gri@golang.org>
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
(cherry picked from commit 6c591f7)
Reviewed-on: https://go-review.googlesource.com/c/go/+/321832
Run-TryBot: Katie Hockman <katie@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
@golang golang locked and limited conversation to collaborators May 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
5 participants
@ianlancetaylor @odeke-em @griesemer @gopherbot @katiehockman