From ef41a4e2face45e580c5836eaebd51629fc23f15 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 16 Mar 2023 14:18:04 -0700 Subject: [PATCH] [release-branch.go1.19] mime/multipart: avoid excessive copy buffer allocations in ReadForm When copying form data to disk with io.Copy, allocate only one copy buffer and reuse it rather than creating two buffers per file (one from io.multiReader.WriteTo, and a second one from os.File.ReadFrom). 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/+/1802453 Run-TryBot: Damien Neil Reviewed-by: Julie Qiu Reviewed-by: Roland Shoemaker Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802395 Run-TryBot: Roland Shoemaker Reviewed-by: Damien Neil Change-Id: Ie405470c92abffed3356913b37d813e982c96c8b Reviewed-on: https://go-review.googlesource.com/c/go/+/481983 Run-TryBot: Michael Knyszek TryBot-Result: Gopher Robot Auto-Submit: Michael Knyszek Reviewed-by: Matthew Dempsky Upstream-Status: Backport [https://github.com/golang/go/commit/ef41a4e2face45e580c5836eaebd51629fc23f15] CVE: CVE-2023-24536 Signed-off-by: Vijay Anusuri --- src/mime/multipart/formdata.go | 15 +++++++-- src/mime/multipart/formdata_test.go | 49 +++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/src/mime/multipart/formdata.go b/src/mime/multipart/formdata.go index a7d4ca97f0484..975dcb6b26db4 100644 --- a/src/mime/multipart/formdata.go +++ b/src/mime/multipart/formdata.go @@ -84,6 +84,7 @@ func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) { maxMemoryBytes = math.MaxInt64 } } + var copyBuf []byte for { p, err := r.nextPart(false, maxMemoryBytes) if err == io.EOF { @@ -147,14 +148,22 @@ func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) { } } numDiskFiles++ - size, err := io.Copy(file, io.MultiReader(&b, p)) + if _, err := file.Write(b.Bytes()); err != nil { + return nil, err + } + if copyBuf == nil { + copyBuf = make([]byte, 32*1024) // same buffer size as io.Copy uses + } + // os.File.ReadFrom will allocate its own copy buffer if we let io.Copy use it. + type writerOnly struct{ io.Writer } + remainingSize, err := io.CopyBuffer(writerOnly{file}, p, copyBuf) if err != nil { return nil, err } fh.tmpfile = file.Name() - fh.Size = size + fh.Size = int64(b.Len()) + remainingSize fh.tmpoff = fileOff - fileOff += size + fileOff += fh.Size if !combineFiles { if err := file.Close(); err != nil { return nil, err diff --git a/src/mime/multipart/formdata_test.go b/src/mime/multipart/formdata_test.go index 5cded7170c6b8..f5b56083b2377 100644 --- a/src/mime/multipart/formdata_test.go +++ b/src/mime/multipart/formdata_test.go @@ -368,3 +368,52 @@ func testReadFormManyFiles(t *testing.T, distinct bool) { t.Fatalf("temp dir contains %v files; want 0", len(names)) } } + +func BenchmarkReadForm(b *testing.B) { + for _, test := range []struct { + name string + form func(fw *Writer, count int) + }{{ + name: "fields", + form: func(fw *Writer, count int) { + for i := 0; i < count; i++ { + w, _ := fw.CreateFormField(fmt.Sprintf("field%v", i)) + fmt.Fprintf(w, "value %v", i) + } + }, + }, { + name: "files", + form: func(fw *Writer, count int) { + for i := 0; i < count; i++ { + w, _ := fw.CreateFormFile(fmt.Sprintf("field%v", i), fmt.Sprintf("file%v", i)) + fmt.Fprintf(w, "value %v", i) + } + }, + }} { + b.Run(test.name, func(b *testing.B) { + for _, maxMemory := range []int64{ + 0, + 1 << 20, + } { + var buf bytes.Buffer + fw := NewWriter(&buf) + test.form(fw, 10) + if err := fw.Close(); err != nil { + b.Fatal(err) + } + b.Run(fmt.Sprintf("maxMemory=%v", maxMemory), func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + fr := NewReader(bytes.NewReader(buf.Bytes()), fw.Boundary()) + form, err := fr.ReadForm(maxMemory) + if err != nil { + b.Fatal(err) + } + form.RemoveAll() + } + + }) + } + }) + } +}