From d99b717255be173b46cbb1f48a411a013242883c Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Fri, 10 Mar 2023 14:21:05 -0800 Subject: [PATCH] [release-branch.go1.19] net/textproto: avoid overpredicting the number of MIME header keys # AWS EKS Backported To: go-1.16.15-eks Backported On: Wed, 5 Apr 2023 Backported By: kodurub@amazon.com Backported From: release-branch.go1.19 Source Commit: https://github.com/golang/go/commit/d6759e7a059f4208f07aa781402841d7ddaaef96 This fix requires `Cut` method in `src/net/textproto/reader.go`. Backported `Cut` method as a separate patch. A parsed MIME header is a map[string][]string. In the common case, a header contains many one-element []string slices. To avoid allocating a separate slice for each key, ReadMIMEHeader looks ahead in the input to predict the number of keys that will be parsed, and allocates a single []string of that length. The individual slices are then allocated out of the larger one. The prediction of the number of header keys was done by counting newlines in the input buffer, which does not take into account header continuation lines (where a header key/value spans multiple lines) or the end of the header block and the start of the body. This could lead to a substantial amount of overallocation, for example when the body consists of nothing but a large block of newlines. Fix header key count prediction to take into account the end of the headers (indicated by a blank line) and continuation lines (starting with whitespace). Thanks to Jakob Ackermann (@das7pad) for reporting this issue. Fixes CVE-2023-24534 For #58975 Fixes #59267 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802452 Run-TryBot: Damien Neil Reviewed-by: Roland Shoemaker Reviewed-by: Julie Qiu (cherry picked from commit f739f080a72fd5b06d35c8e244165159645e2ed6) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802393 Reviewed-by: Damien Neil Run-TryBot: Roland Shoemaker Change-Id: I675451438d619a9130360c56daf529559004903f Reviewed-on: https://go-review.googlesource.com/c/go/+/481982 Run-TryBot: Michael Knyszek TryBot-Result: Gopher Robot Reviewed-by: Matthew Dempsky Auto-Submit: Michael Knyszek --- src/net/textproto/reader.go | 30 ++++++++++------ src/net/textproto/reader_test.go | 59 ++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 11 deletions(-) diff --git a/src/net/textproto/reader.go b/src/net/textproto/reader.go index c5b76c29d5..4299608d5a 100644 --- a/src/net/textproto/reader.go +++ b/src/net/textproto/reader.go @@ -493,8 +493,11 @@ func readMIMEHeader(r *Reader, maxMemory, maxHeaders int64) (MIMEHeader, error) // large one ahead of time which we'll cut up into smaller // slices. If this isn't big enough later, we allocate small ones. var strs []string - hint := r.upcomingHeaderNewlines() + hint := r.upcomingHeaderKeys() if hint > 0 { + if hint > 1000 { + hint = 1000 // set a cap to avoid overallocation + } strs = make([]string, hint) } @@ -590,9 +593,11 @@ func mustHaveFieldNameColon(line []byte) error { return nil } -// upcomingHeaderNewlines returns an approximation of the number of newlines +var nl = []byte("\n") + +// upcomingHeaderKeys returns an approximation of the number of keys // that will be in this header. If it gets confused, it returns 0. -func (r *Reader) upcomingHeaderNewlines() (n int) { +func (r *Reader) upcomingHeaderKeys() (n int) { // Try to determine the 'hint' size. r.R.Peek(1) // force a buffer load if empty s := r.R.Buffered() @@ -600,17 +605,20 @@ func (r *Reader) upcomingHeaderNewlines() (n int) { return } peek, _ := r.R.Peek(s) - for len(peek) > 0 { - i := bytes.IndexByte(peek, '\n') - if i < 3 { - // Not present (-1) or found within the next few bytes, - // implying we're at the end ("\r\n\r\n" or "\n\n") - return + for len(peek) > 0 && n < 1000 { + var line []byte + line, peek, _ = bytes.Cut(peek, nl) + if len(line) == 0 || (len(line) == 1 && line[0] == '\r') { + // Blank line separating headers from the body. + break + } + if line[0] == ' ' || line[0] == '\t' { + // Folded continuation of the previous line. + continue } n++ - peek = peek[i+1:] } - return + return n } // CanonicalMIMEHeaderKey returns the canonical format of the diff --git a/src/net/textproto/reader_test.go b/src/net/textproto/reader_test.go index 3124d438fa..3ae0de1353 100644 --- a/src/net/textproto/reader_test.go +++ b/src/net/textproto/reader_test.go @@ -9,6 +9,7 @@ import ( "bytes" "io" "reflect" + "runtime" "strings" "testing" ) @@ -127,6 +128,42 @@ func TestReadMIMEHeaderSingle(t *testing.T) { } } +// TestReaderUpcomingHeaderKeys is testing an internal function, but it's very +// difficult to test well via the external API. +func TestReaderUpcomingHeaderKeys(t *testing.T) { + for _, test := range []struct { + input string + want int + }{{ + input: "", + want: 0, + }, { + input: "A: v", + want: 1, + }, { + input: "A: v\r\nB: v\r\n", + want: 2, + }, { + input: "A: v\nB: v\n", + want: 2, + }, { + input: "A: v\r\n continued\r\n still continued\r\nB: v\r\n\r\n", + want: 2, + }, { + input: "A: v\r\n\r\nB: v\r\nC: v\r\n", + want: 1, + }, { + input: "A: v" + strings.Repeat("\n", 1000), + want: 1, + }} { + r := reader(test.input) + got := r.upcomingHeaderKeys() + if test.want != got { + t.Fatalf("upcomingHeaderKeys(%q): %v; want %v", test.input, got, test.want) + } + } +} + func TestReadMIMEHeaderNoKey(t *testing.T) { r := reader(": bar\ntest-1: 1\n\n") m, err := r.ReadMIMEHeader() @@ -223,6 +260,28 @@ func TestReadMIMEHeaderTrimContinued(t *testing.T) { } } +// Test that reading a header doesn't overallocate. Issue 58975. +func TestReadMIMEHeaderAllocations(t *testing.T) { + var totalAlloc uint64 + const count = 200 + for i := 0; i < count; i++ { + r := reader("A: b\r\n\r\n" + strings.Repeat("\n", 4096)) + var m1, m2 runtime.MemStats + runtime.ReadMemStats(&m1) + _, err := r.ReadMIMEHeader() + if err != nil { + t.Fatalf("ReadMIMEHeader: %v", err) + } + runtime.ReadMemStats(&m2) + totalAlloc += m2.TotalAlloc - m1.TotalAlloc + } + // 32k is large and we actually allocate substantially less, + // but prior to the fix for #58975 we allocated ~400k in this case. + if got, want := totalAlloc/count, uint64(32768); got > want { + t.Fatalf("ReadMIMEHeader allocated %v bytes, want < %v", got, want) + } +} + type readResponseTest struct { in string inCode int -- 2.39.1