From d6759e7a059f4208f07aa781402841d7ddaaef96 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 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 Upstream-Status: Backport [https://github.com/golang/go/commit/d6759e7a059f4208f07aa781402841d7ddaaef96] CVE: CVE-2023-24534 Signed-off-by: Vivek Kumbhar --- src/bytes/bytes.go | 13 +++++++ src/net/textproto/reader.go | 31 +++++++++++------ src/net/textproto/reader_test.go | 59 ++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 11 deletions(-) diff --git a/src/bytes/bytes.go b/src/bytes/bytes.go index e872cc2..1f0d760 100644 --- a/src/bytes/bytes.go +++ b/src/bytes/bytes.go @@ -1078,6 +1078,19 @@ func Index(s, sep []byte) int { return -1 } +// Cut slices s around the first instance of sep, +// returning the text before and after sep. +// The found result reports whether sep appears in s. +// If sep does not appear in s, cut returns s, nil, false. +// +// Cut returns slices of the original slice s, not copies. +func Cut(s, sep []byte) (before, after []byte, found bool) { + if i := Index(s, sep); i >= 0 { + return s[:i], s[i+len(sep):], true + } + return s, nil, false +} + func indexRabinKarp(s, sep []byte) int { // Rabin-Karp search hashsep, pow := hashStr(sep) diff --git a/src/net/textproto/reader.go b/src/net/textproto/reader.go index a505da9..8d547fe 100644 --- a/src/net/textproto/reader.go +++ b/src/net/textproto/reader.go @@ -486,8 +487,11 @@ func (r *Reader) ReadMIMEHeader() (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) } @@ -562,9 +566,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() @@ -572,17 +578,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 3124d43..3ae0de1 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.25.1