[go] net/http/fcgi: avoid integer overflow when checking params lengths
Rajiv Singh (Gerrit)
Rajiv Singh has uploaded the change for review![]( "Open in Gerrit")
Commit message
net/http/fcgi: avoid integer overflow when checking params lengths
parseParams reads a key length and a value length, each a uint32 capped
at 0x7fffffff by readSize, and checks their sum against the remaining
input. The sum was computed as int, which is 32 bits wide on 32-bit
platforms, so two large lengths wrap to a negative value and the check
passes. parseParams then slices the remaining text by the oversized key
length and panics with a slice bounds error.
Compute the sum as uint64 so it cannot overflow on any platform.
Fixes #81293
Change-Id: Idc38539185a315781fad5411b0a861b588dda792
Change diff
diff --git a/src/net/http/fcgi/child.go b/src/net/http/fcgi/child.go
index 7665e7d..5e858ab 100644
--- a/src/net/http/fcgi/child.go
+++ b/src/net/http/fcgi/child.go
@@ -59,7 +59,11 @@
return
}
text = text[n:]
- if int(keyLen)+int(valLen) > len(text) {
+ // keyLen and valLen are uint32, so their sum can exceed the range of
+ // int on 32-bit platforms. Add them as uint64 to avoid overflowing,
+ // which would let an oversized length slip past this check and panic
+ // in the slicing below.
+ if uint64(keyLen)+uint64(valLen) > uint64(len(text)) {
return
}
key := readString(text, keyLen)
diff --git a/src/net/http/fcgi/fcgi_test.go b/src/net/http/fcgi/fcgi_test.go
index 03c4224..9c35b88 100644
--- a/src/net/http/fcgi/fcgi_test.go
+++ b/src/net/http/fcgi/fcgi_test.go
@@ -9,6 +9,7 @@
"errors"
"io"
"net/http"
+ "slices"
"strings"
"testing"
"time"
@@ -451,3 +452,64 @@
<-rc.closed
t.Log("FastCGI child closed connection")
}
+
+// TestParseParamsOversizedLengths checks that a params record declaring key and
+// value lengths whose sum overflows int is rejected rather than panicking.
+// readSize caps each length at 0x7fffffff, so on platforms where int is 32 bits
+// the sum of two such lengths wraps negative, which used to let the length
+// check in parseParams pass and then panic when slicing. See go.dev/issue/81293.
+func TestParseParamsOversizedLengths(t *testing.T) {
+ maxSize := []byte{0xff, 0xff, 0xff, 0xff} // decodes to 0x7fffffff
+ smallSize := []byte{0x01} // decodes to 1
+
+ for _, tt := range []struct {
+ name string
+ rawParams []byte
+ }{
+ {
+ name: "both lengths oversized",
+ rawParams: slices.Concat(maxSize, maxSize),
+ },
+ {
+ name: "key length oversized",
+ rawParams: slices.Concat(maxSize, smallSize, []byte("v")),
+ },
+ {
+ name: "value length oversized",
+ rawParams: slices.Concat(smallSize, maxSize, []byte("k")),
+ },
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ r := newRequest(1, 0)
+ r.rawParams = tt.rawParams
+
+ r.parseParams() // must not panic on any platform
+
+ if len(r.params) != 0 {
+ t.Errorf("parseParams recorded %d params, want 0", len(r.params))
+ }
+ })
+ }
+}
+
+// TestParseParamsWellFormed checks that ordinary params are still parsed, so
+// that the oversized-length check does not reject valid input.
+func TestParseParamsWellFormed(t *testing.T) {
+ r := newRequest(1, 0)
+ r.rawParams = []byte{
+ 3, 5, 'k', 'e', 'y', 'v', 'a', 'l', 'u', 'e',
+ 1, 1, 'a', 'b',
+ }
+
+ r.parseParams()
+
+ want := map[string]string{"key": "value", "a": "b"}
+ if len(r.params) != len(want) {
+ t.Fatalf("parseParams got %v, want %v", r.params, want)
+ }
+ for k, v := range want {
+ if got := r.params[k]; got != v {
+ t.Errorf("params[%q] = %q, want %q", k, got, v)
+ }
+ }
+}
Change information
- M src/net/http/fcgi/child.go
- M src/net/http/fcgi/fcgi_test.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rajiv Singh (Gerrit)
Message from Rajiv Singh![]( "Open in Gerrit")
Friendly ping, this has been up for about a month without a review. I have just rebased onto master as patch set 2.
One thing worth stating explicitly, since it affects how the test reads: this only reproduces where int is 32 bits. On 64-bit platforms int(keyLen)+int(valLen) is 4294967294, which correctly fails the check and returns early, so TestParseParamsOversizedLengths passes with or without the fix on amd64 and arm64. It only has teeth on linux-386 and the other 32-bit builders, where the sum wraps negative, the guard does not fire, and parseParams falls through to text[keyLen:].
Worth noting too that readString is already self-guarding, returning "" when size > uint32(len(s)), so the panic site is the text = text[keyLen:] slice rather than readString itself.
I verified the arithmetic at all three widths for each of the three test cases. At int32 the guard evaluates false in every case, so execution reaches the slice, while at int64 and at the uint64 of this patch it evaluates true and returns early. The package also cross-compiles cleanly for linux/386, linux/arm and linux/mips.
Happy to restructure the test if you would prefer it shaped differently.
Related details
- Damien Neil
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |