From 3c010f2c2182a12f28ad86c5e1ff984f1f2d880a Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 16 Mar 2023 16:56:12 -0700 Subject: [PATCH] net/textproto, mime/multipart: improve accounting of non-file data For requests containing large numbers of small parts, memory consumption of a parsed form could be about 250% over the estimated size. When considering the size of parsed forms, account for the size of FileHeader structs and increase the estimate of memory consumed by map entries. Thanks to Jakob Ackermann (@das7pad) for reporting this issue. For CVE-2023-24536 For #59153 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802454 Run-TryBot: Damien Neil Reviewed-by: Roland Shoemaker Reviewed-by: Julie Qiu Change-Id: I9620758495ed77c09ca6dc5db4b723c29f3baad8 Reviewed-on: https://go-review.googlesource.com/c/go/+/482076 TryBot-Result: Gopher Robot Auto-Submit: Michael Knyszek Run-TryBot: Michael Knyszek Reviewed-by: Matthew Dempsky --- src/mime/multipart/formdata.go | 9 +++-- src/mime/multipart/formdata_test.go | 55 ++++++++++++----------------- src/net/textproto/reader.go | 8 ++++- 3 files changed, 37 insertions(+), 35 deletions(-) diff --git a/src/mime/multipart/formdata.go b/src/mime/multipart/formdata.go index 902bb10e57..73431c3a84 100644 --- a/src/mime/multipart/formdata.go +++ b/src/mime/multipart/formdata.go @@ -107,8 +107,9 @@ func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) { // Multiple values for the same key (one map entry, longer slice) are cheaper // than the same number of values for different keys (many map entries), but // using a consistent per-value cost for overhead is simpler. + const mapEntryOverhead = 200 maxMemoryBytes -= int64(len(name)) - maxMemoryBytes -= 100 // map overhead + maxMemoryBytes -= mapEntryOverhead if maxMemoryBytes < 0 { // We can't actually take this path, since nextPart would already have // rejected the MIME headers for being too large. Check anyway. @@ -132,7 +133,10 @@ func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) { } // file, store in memory or on disk + const fileHeaderSize = 100 maxMemoryBytes -= mimeHeaderSize(p.Header) + maxMemoryBytes -= mapEntryOverhead + maxMemoryBytes -= fileHeaderSize if maxMemoryBytes < 0 { return nil, ErrMessageTooLarge } @@ -187,9 +191,10 @@ func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) { } func mimeHeaderSize(h textproto.MIMEHeader) (size int64) { + size = 400 for k, vs := range h { size += int64(len(k)) - size += 100 // map entry overhead + size += 200 // map entry overhead for _, v := range vs { size += int64(len(v)) } diff --git a/src/mime/multipart/formdata_test.go b/src/mime/multipart/formdata_test.go index e0ceb6f7aa..7c09cfd07a 100644 --- a/src/mime/multipart/formdata_test.go +++ b/src/mime/multipart/formdata_test.go @@ -223,10 +223,10 @@ func (r *failOnReadAfterErrorReader) Read(p []byte) (n int, err error) { // TestReadForm_NonFileMaxMemory asserts that the ReadForm maxMemory limit is applied // while processing non-file form data as well as file form data. func TestReadForm_NonFileMaxMemory(t *testing.T) { - n := 10<<20 + 25 if testing.Short() { - n = 10<<10 + 25 + t.Skip("skipping in -short mode") } + n := 10 << 20 largeTextValue := strings.Repeat("1", n) message := `--MyBoundary Content-Disposition: form-data; name="largetext" @@ -234,38 +234,29 @@ Content-Disposition: form-data; name="largetext" ` + largeTextValue + ` --MyBoundary-- ` - testBody := strings.ReplaceAll(message, "\n", "\r\n") - testCases := []struct { - name string - maxMemory int64 - err error - }{ - {"smaller", 50 + int64(len("largetext")) + 100, nil}, - {"exact-fit", 25 + int64(len("largetext")) + 100, nil}, - {"too-large", 0, ErrMessageTooLarge}, - } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - if tc.maxMemory == 0 && testing.Short() { - t.Skip("skipping in -short mode") - } - b := strings.NewReader(testBody) - r := NewReader(b, boundary) - f, err := r.ReadForm(tc.maxMemory) - if err == nil { - defer f.RemoveAll() - } - if tc.err != err { - t.Fatalf("ReadForm error - got: %v; expected: %v", err, tc.err) - } - if err == nil { - if g := f.Value["largetext"][0]; g != largeTextValue { - t.Errorf("largetext mismatch: got size: %v, expected size: %v", len(g), len(largeTextValue)) - } - } - }) + // Try parsing the form with increasing maxMemory values. + // Changes in how we account for non-file form data may cause the exact point + // where we change from rejecting the form as too large to accepting it to vary, + // but we should see both successes and failures. + const failWhenMaxMemoryLessThan = 128 + for maxMemory := int64(0); maxMemory < failWhenMaxMemoryLessThan*2; maxMemory += 16 { + b := strings.NewReader(testBody) + r := NewReader(b, boundary) + f, err := r.ReadForm(maxMemory) + if err != nil { + continue + } + if g := f.Value["largetext"][0]; g != largeTextValue { + t.Errorf("largetext mismatch: got size: %v, expected size: %v", len(g), len(largeTextValue)) + } + f.RemoveAll() + if maxMemory < failWhenMaxMemoryLessThan { + t.Errorf("ReadForm(%v): no error, expect to hit memory limit when maxMemory < %v", maxMemory, failWhenMaxMemoryLessThan) + } + return } + t.Errorf("ReadForm(x) failed for x < 1024, expect success") } // TestReadForm_MetadataTooLarge verifies that we account for the size of field names, diff --git a/src/net/textproto/reader.go b/src/net/textproto/reader.go index 48ae2946a6..af82b4b9ab 100644 --- a/src/net/textproto/reader.go +++ b/src/net/textproto/reader.go @@ -499,6 +499,12 @@ func readMIMEHeader(r *Reader, lim int64) (MIMEHeader, error) { m := make(MIMEHeader, hint) + // Account for 400 bytes of overhead for the MIMEHeader, plus 200 bytes per entry. + // Benchmarking map creation as of go1.20, a one-entry MIMEHeader is 416 bytes and large + // MIMEHeaders average about 200 bytes per entry. + lim -= 400 + const mapEntryOverhead = 200 + // The first line cannot start with a leading space. if buf, err := r.R.Peek(1); err == nil && (buf[0] == ' ' || buf[0] == '\t') { line, err := r.readLineSlice() @@ -542,7 +548,7 @@ func readMIMEHeader(r *Reader, lim int64) (MIMEHeader, error) { vv := m[key] if vv == nil { lim -= int64(len(key)) - lim -= 100 // map entry overhead + lim -= mapEntryOverhead } lim -= int64(len(value)) if lim < 0 { -- 2.50.0