]> Cypherpunks repositories - gostls13.git/commitdiff
[release-branch.go1.19] net/textproto, mime/multipart: improve accounting of non...
authorDamien Neil <dneil@google.com>
Thu, 16 Mar 2023 23:56:12 +0000 (16:56 -0700)
committerGopher Robot <gobot@golang.org>
Tue, 4 Apr 2023 16:47:46 +0000 (16:47 +0000)
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
For #59269

Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802454
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
Reviewed-by: Julie Qiu <julieqiu@google.com>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802396
Run-TryBot: Roland Shoemaker <bracewell@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Change-Id: I31bc50e9346b4eee6fbe51a18c3c57230cc066db
Reviewed-on: https://go-review.googlesource.com/c/go/+/481984
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Knyszek <mknyszek@google.com>

src/mime/multipart/formdata.go
src/mime/multipart/formdata_test.go
src/net/textproto/reader.go

index 975dcb6b26db47b55fa0551a663cfc8e20e9fb7b..3f6ff697ca608276541a4460fdf3130a7d650919 100644 (file)
@@ -103,8 +103,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.
@@ -128,7 +129,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
                }
@@ -183,9 +187,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))
                }
index f5b56083b2377324da9735d659f28f5b71c821d6..8ed26e0c340814475f3bf582d3ca6901bc6ff43f 100644 (file)
@@ -192,10 +192,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"
@@ -203,38 +203,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,
index 9a21777df8be02dd5b0727619afea31a010deba8..c1284fde25eb7ffdc8b2fd1a7d581518085d25ed 100644 (file)
@@ -503,6 +503,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()
@@ -538,7 +544,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 {