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

[odeke-em]

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

[gopherbot]

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

[odeke-em]

Should we perhaps backport this change?

[griesemer]

cc: @FiloSottile @katiehockman re: backporting decision.

[odeke-em]

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]

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]

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]

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

[gopherbot]

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]

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

[gopherbot]

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