Skip to content

Commit

Permalink
[release-branch.go1.20] mime/multipart: limit memory/inode consumptio…
Browse files Browse the repository at this point in the history 
…n of ReadForm

Reader.ReadForm is documented as storing "up to maxMemory bytes + 10MB"
in memory. Parsed forms can consume substantially more memory than
this limit, since ReadForm does not account for map entry overhead
and MIME headers.

In addition, while the amount of disk memory consumed by ReadForm can
be constrained by limiting the size of the parsed input, ReadForm will
create one temporary file per form part stored on disk, potentially
consuming a large number of inodes.

Update ReadForm's memory accounting to include part names,
MIME headers, and map entry overhead.

Update ReadForm to store all on-disk file parts in a single
temporary file.

Files returned by FileHeader.Open are documented as having a concrete
type of *os.File when a file is stored on disk. The change to use a
single temporary file for all parts means that this is no longer the
case when a form contains more than a single file part stored on disk.

The previous behavior of storing each file part in a separate disk
file may be reenabled with GODEBUG=multipartfiles=distinct.

Update Reader.NextPart and Reader.NextRawPart to set a 10MiB cap
on the size of MIME headers.

Thanks to Jakob Ackermann (@das7pad) for reporting this issue.

Updates golang#58006
Fixes golang#58363
Fixes CVE-2022-41725

Change-Id: Ibd780a6c4c83ac8bcfd3cbe344f042e9940f2eab
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1714276
Reviewed-by: Julie Qiu <julieqiu@google.com>
TryBot-Result: Security TryBots <security-trybots@go-security-trybots.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
(cherry picked from commit 7d0da0029bfbe3228cc5216ced8c7b3184eb517d)
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1728950
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Roland Shoemaker <bracewell@google.com>
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/468120
Auto-Submit: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
  • Loading branch information
@neild @romaindoumenc
neild authored and romaindoumenc committed Mar 3, 2023
1 parent e09e9a4 commit 6edf4e7
Show file tree
Hide file tree
Showing 6 changed files with 297 additions and 38 deletions.
133 changes: 108 additions & 25 deletions src/mime/multipart/formdata.go
View file
Expand Up @@ -7,6 +7,7 @@ package multipart
import (
"bytes"
"errors"
"internal/godebug"
"io"
"math"
"net/textproto"
Expand All @@ -31,25 +32,61 @@ func (r *Reader) ReadForm(maxMemory int64) (*Form, error) {
return r.readForm(maxMemory)
}

var multipartFiles = godebug.New("multipartfiles")

func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) {
form := &Form{make(map[string][]string), make(map[string][]*FileHeader)}
var (
file *os.File
fileOff int64
)
numDiskFiles := 0
combineFiles := multipartFiles.Value() != "distinct"
defer func() {
if file != nil {
if cerr := file.Close(); err == nil {
err = cerr
}
}
if combineFiles && numDiskFiles > 1 {
for _, fhs := range form.File {
for _, fh := range fhs {
fh.tmpshared = true
}
}
}
if err != nil {
form.RemoveAll()
if file != nil {
os.Remove(file.Name())
}
}
}()

// Reserve an additional 10 MB for non-file parts.
maxValueBytes := maxMemory + int64(10<<20)
if maxValueBytes <= 0 {
// maxFileMemoryBytes is the maximum bytes of file data we will store in memory.
// Data past this limit is written to disk.
// This limit strictly applies to content, not metadata (filenames, MIME headers, etc.),
// since metadata is always stored in memory, not disk.
//
// maxMemoryBytes is the maximum bytes we will store in memory, including file content,
// non-file part values, metdata, and map entry overhead.
//
// We reserve an additional 10 MB in maxMemoryBytes for non-file data.
//
// The relationship between these parameters, as well as the overly-large and
// unconfigurable 10 MB added on to maxMemory, is unfortunate but difficult to change
// within the constraints of the API as documented.
maxFileMemoryBytes := maxMemory
maxMemoryBytes := maxMemory + int64(10<<20)
if maxMemoryBytes <= 0 {
if maxMemory < 0 {
maxValueBytes = 0
maxMemoryBytes = 0
} else {
maxValueBytes = math.MaxInt64
maxMemoryBytes = math.MaxInt64
}
}
for {
p, err := r.NextPart()
p, err := r.nextPart(false, maxMemoryBytes)
if err == io.EOF {
break
}
Expand All @@ -63,59 +100,91 @@ func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) {
}
filename := p.FileName()

// 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.
maxMemoryBytes -= int64(len(name))
maxMemoryBytes -= 100 // map overhead
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.
return nil, ErrMessageTooLarge
}

var b bytes.Buffer

if filename == "" {
// value, store as string in memory
n, err := io.CopyN(&b, p, maxValueBytes+1)
n, err := io.CopyN(&b, p, maxMemoryBytes+1)
if err != nil && err != io.EOF {
return nil, err
}
maxValueBytes -= n
if maxValueBytes < 0 {
maxMemoryBytes -= n
if maxMemoryBytes < 0 {
return nil, ErrMessageTooLarge
}
form.Value[name] = append(form.Value[name], b.String())
continue
}

// file, store in memory or on disk
maxMemoryBytes -= mimeHeaderSize(p.Header)
if maxMemoryBytes < 0 {
return nil, ErrMessageTooLarge
}
fh := &FileHeader{
Filename: filename,
Header: p.Header,
}
n, err := io.CopyN(&b, p, maxMemory+1)
n, err := io.CopyN(&b, p, maxFileMemoryBytes+1)
if err != nil && err != io.EOF {
return nil, err
}
if n > maxMemory {
// too big, write to disk and flush buffer
file, err := os.CreateTemp("", "multipart-")
if err != nil {
return nil, err
if n > maxFileMemoryBytes {
if file == nil {
file, err = os.CreateTemp(r.tempDir, "multipart-")
if err != nil {
return nil, err
}
}
numDiskFiles++
size, err := io.Copy(file, io.MultiReader(&b, p))
if cerr := file.Close(); err == nil {
err = cerr
}
if err != nil {
os.Remove(file.Name())
return nil, err
}
fh.tmpfile = file.Name()
fh.Size = size
fh.tmpoff = fileOff
fileOff += size
if !combineFiles {
if err := file.Close(); err != nil {
return nil, err
}
file = nil
}
} else {
fh.content = b.Bytes()
fh.Size = int64(len(fh.content))
maxMemory -= n
maxValueBytes -= n
maxFileMemoryBytes -= n
maxMemoryBytes -= n
}
form.File[name] = append(form.File[name], fh)
}

return form, nil
}

func mimeHeaderSize(h textproto.MIMEHeader) (size int64) {
for k, vs := range h {
size += int64(len(k))
size += 100 // map entry overhead
for _, v := range vs {
size += int64(len(v))
}
}
return size
}

// Form is a parsed multipart form.
// Its File parts are stored either in memory or on disk,
// and are accessible via the *FileHeader's Open method.
Expand All @@ -133,7 +202,7 @@ func (f *Form) RemoveAll() error {
for _, fh := range fhs {
if fh.tmpfile != "" {
e := os.Remove(fh.tmpfile)
if e != nil && err == nil {
if e != nil && !errors.Is(e, os.ErrNotExist) && err == nil {
err = e
}
}
Expand All @@ -148,15 +217,25 @@ type FileHeader struct {
Header textproto.MIMEHeader
Size int64

content []byte
tmpfile string
content []byte
tmpfile string
tmpoff int64
tmpshared bool
}

// Open opens and returns the FileHeader's associated File.
func (fh *FileHeader) Open() (File, error) {
if b := fh.content; b != nil {
r := io.NewSectionReader(bytes.NewReader(b), 0, int64(len(b)))
return sectionReadCloser{r}, nil
return sectionReadCloser{r, nil}, nil
}
if fh.tmpshared {
f, err := os.Open(fh.tmpfile)
if err != nil {
return nil, err
}
r := io.NewSectionReader(f, fh.tmpoff, fh.Size)
return sectionReadCloser{r, f}, nil
}
return os.Open(fh.tmpfile)
}
Expand All @@ -175,8 +254,12 @@ type File interface {

type sectionReadCloser struct {
*io.SectionReader
io.Closer
}

func (rc sectionReadCloser) Close() error {
if rc.Closer != nil {
return rc.Closer.Close()
}
return nil
}

0 comments on commit 6edf4e7

Please sign in to comment.