Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mime/multipart, net/http: Request.ParseMultipartForm silently fails if you pass math.MaxInt64 which stems from multipart.Reader.readFrom not checking for overflows #40430

Closed
avivklas opened this issue Jul 27, 2020 · 11 comments
Assignees
Labels
Milestone

Comments

@avivklas
Copy link
Contributor

@avivklas avivklas commented Jul 27, 2020

What version of Go are you using (go version)?

go version go1.14.3 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/avivklas/Library/Caches/go-build"
GOENV="/Users/avivklas/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/avivklas/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.3/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.3/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/94/77vmj28n3xd3chzm7s3wml3c0000gn/T/go-build910100299=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have an http.HandlerFunc which is meant for file uploads. because my application handles request limits from outside of where the logic code lives, I've used r.ParseMultipartForm with math.MaxInt64.

func upload(_ http.ResponseWriter, r *http.Request) {
	err := r.ParseMultipartForm(math.MaxInt64)
	if err != nil {
		panic(err)
	}

	fmt.Println(r.MultipartForm.File["file"][0].Size)
}

What did you expect to see?

either panic with error or size larger than 0

What did you see instead?

0


I've drilled down to the following code in mime/multipart/formdata.go:

	// Reserve an additional 10 MB for non-file parts.
	maxValueBytes := maxMemory + int64(10<<20)

it adds to the value I've passed to ParseMultipartForm, but since I've passed the max int64 value, it causes the value to be negative. but a more severe issue, IMO, is that no error has been thrown.

I'm going to investigate why it doesn't return an error and how can I understand in my code that the parsing did not succeed, but additionally, I suggest to add a check if we can add another 10MB to the int64 value before actually adding it and set it to max int64 value otherwise

@avivklas avivklas changed the title http.Request's ParseMultipartForm silently fails if you pass math.MaxInt64 http.Request's ParseMultipartForm silently fails if you pass math.MaxInt64 Jul 27, 2020
@cagedmantis cagedmantis changed the title http.Request's ParseMultipartForm silently fails if you pass math.MaxInt64 net/http: http.Request's ParseMultipartForm silently fails if you pass math.MaxInt64 Jul 27, 2020
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jul 28, 2020

Thank you for reporting this issue @avivklas and welcome to the Go project! Thank you @cagedmantis for the triage!

@avivklas indeed, it is an integer overflow that's deep inside mime/multipart, but also 2 fold, nice that you dug in:
a) The code in mime/multipart can perhaps be changed to catch overflows so

diff --git a/src/mime/multipart/formdata.go b/src/mime/multipart/formdata.go
index 832d0ad693..4eb3101294 100644
--- a/src/mime/multipart/formdata.go
+++ b/src/mime/multipart/formdata.go
@@ -7,6 +7,7 @@ package multipart
 import (
 	"bytes"
 	"errors"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"net/textproto"
@@ -41,6 +42,9 @@ func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) {
 
 	// Reserve an additional 10 MB for non-file parts.
 	maxValueBytes := maxMemory + int64(10<<20)
+	if maxValueBytes <= 0 {
+		return nil, fmt.Errorf("multipart: integer overflow from maxMemory(%d) + 10MiB for non-file parts", maxMemory)
+	}
 	for {
 		p, err := r.NextPart()
 		if err == io.EOF {

b) The code for io.CopyN could also immediately check for n < 0 and return so

diff --git a/src/io/io.go b/src/io/io.go
index 3dea70b947..4f6f1b5800 100644
--- a/src/io/io.go
+++ b/src/io/io.go
@@ -341,6 +341,9 @@ func ReadFull(r Reader, buf []byte) (n int, err error) {
 // If dst implements the ReaderFrom interface,
 // the copy is implemented using it.
 func CopyN(dst Writer, src Reader, n int64) (written int64, err error) {
+	if n < 1 {
+		return 0, EOF
+	}
 	written, err = Copy(dst, LimitReader(src, n))
 	if written == n {
 		return n, nil

and here is a full standing test that we can use to catch regressions later on

package main

import (
	"bytes"
	"math"
	"mime/multipart"
	"net/http"
	"net/http/httptest"
	"testing"
)

func TestMaxInt64ForMultipartFormMaxMemory(t *testing.T) {
	cst := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
		if err := req.ParseMultipartForm(math.MaxInt64); err != nil {
			http.Error(rw, err.Error(), http.StatusBadRequest)
			return
		}
	}))
	defer cst.Close()

	fBuf := new(bytes.Buffer)
	mw := multipart.NewWriter(fBuf)
	mf, err := mw.CreateFormFile("file", "myfile.txt")
	if err != nil {
		t.Fatal(err)
	}
	if _, err := mf.Write(bytes.Repeat([]byte("abc"), 2<<20)); err != nil {
		t.Fatal(err)
	}
	if err := mw.Close(); err != nil {
		t.Fatal(err)
	}
	req, err := http.NewRequest("POST", cst.URL, fBuf)
	if err != nil {
		t.Fatal(err)
	}
	req.Header.Set("Content-Type", mw.FormDataContentType())
	res, err := cst.Client().Do(req)
	if err != nil {
		t.Fatal(err)
	}
	if g, w := res.StatusCode, http.StatusBadRequest; g != w {
		t.Fatalf("Status code mismatch: got %d, want %d", g, w)
	}
}

@avivklas given your enthusiasm and that you are already digging, it would be delightful for you to become a Go contributor when the tree opens. I'll gladly walk you through and you can submit a fix for this and I and others will review it; eventually you'll be reporting and fixing even bugs and it will be great to see your work more. The Go tree opens up pretty soon, and if you are up for it, the code I've posted up can serve as a seed and we can review this change for Go1.16. Thank you!

@odeke-em odeke-em changed the title net/http: http.Request's ParseMultipartForm silently fails if you pass math.MaxInt64 mime/multipart, net/http: Request.ParseMultipartForm silently fails if you pass math.MaxInt64 which stems from multipart.Reader.readFrom not checking for overflows Jul 28, 2020
@odeke-em odeke-em added this to the Go1.16 milestone Jul 28, 2020
@avivklas
Copy link
Contributor Author

@avivklas avivklas commented Jul 28, 2020

@odeke-em, thank you very much for your quick, descriptive, and welcoming response! It will be both honor and joy for me to become a Go contributor. I'd love to submit a fix for this and to be assigned to later work that needs to be done. I'll read and follow through https://golang.org/doc/contribute.html. Is there anything else I need in order to send the fix?

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jul 28, 2020

@avivklas awesome! For the fix, we'll also add tests in the respective packages. What you can do for the initial change is add the changes for mime/multipart and net/http. When you submit the change, I'll point you towards writing a test for mime/multipart, but the one for net/http can be done as I've pointed out above.

For mime/multipart, here is what we can do

diff --git a/src/mime/multipart/formdata.go b/src/mime/multipart/formdata.go
index 832d0ad693..9b49cd9cf9 100644
--- a/src/mime/multipart/formdata.go
+++ b/src/mime/multipart/formdata.go
@@ -7,6 +7,7 @@ package multipart
 import (
 	"bytes"
 	"errors"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"net/textproto"
@@ -31,6 +32,8 @@ func (r *Reader) ReadForm(maxMemory int64) (*Form, error) {
 	return r.readForm(maxMemory)
 }
 
+var errIntegerOverflow = errors.New("multipart: integer overflow")
+
 func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) {
 	form := &Form{make(map[string][]string), make(map[string][]*FileHeader)}
 	defer func() {
@@ -41,6 +44,9 @@ func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) {
 
 	// Reserve an additional 10 MB for non-file parts.
 	maxValueBytes := maxMemory + int64(10<<20)
+	if maxValueBytes <= 0 {
+		return nil, fmt.Errorf("%w from maxMemory(%d) + 10MiB for non-file parts", errIntegerOverflow, maxMemory)
+	}
 	for {
 		p, err := r.NextPart()
 		if err == io.EOF {
diff --git a/src/mime/multipart/formdata_test.go b/src/mime/multipart/formdata_test.go
index 7d756c8c24..ef509df942 100644
--- a/src/mime/multipart/formdata_test.go
+++ b/src/mime/multipart/formdata_test.go
@@ -6,7 +6,9 @@ package multipart
 
 import (
 	"bytes"
+	"errors"
 	"io"
+	"math"
 	"os"
 	"strings"
 	"testing"
@@ -52,6 +54,23 @@ func TestReadFormWithNamelessFile(t *testing.T) {
 	}
 }
 
+// Issue 40430: Ensure that we report integer overflows in additions of maxMemory,
+// instead of silently and subtly failing without indication.
+func TestReadFormMaxMemoryOverflow(t *testing.T) {
+	b := strings.NewReader(strings.ReplaceAll(messageWithTextContentType, "\n", "\r\n"))
+	r := NewReader(b, boundary)
+	f, err := r.ReadForm(math.MaxInt64)
+	if err == nil {
+		t.Fatal("Unexpected a non-nil error")
+	}
+	if f != nil {
+		t.Fatalf("Unexpected returned a non-nil form: %v\n", f)
+	}
+	if !errors.Is(err, errIntegerOverflow) {
+		t.Fatalf("Expected an integer overflow report, got %v", err)
+	}
+}
+
 func TestReadFormWithTextContentType(t *testing.T) {
 	// From https://github.com/golang/go/issues/24041
 	b := strings.NewReader(strings.ReplaceAll(messageWithTextContentType, "\n", "\r\n"))
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 7, 2020

Change https://golang.org/cl/247477 mentions this issue: mime/multipart, net/http: report of int overflow error

@avivklas
Copy link
Contributor Author

@avivklas avivklas commented Aug 7, 2020

@odeke-em, about CopyN: I followed the code in and I think that it's a bit misleading to return an EOF. Besides the common sense about this error, the documentation about it reads "Functions should return EOF only to signal a graceful end of input", and this is not the case.
Currently, CopyN uses LimitReader which returns no error in our case, so changing this behavior is a breaking change, isn't it?
If we do want to emphasize the logical redundancy of using this function with n <= 0, we may want a dedicated error, but again, this is a breaking change.

What do you think?

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Aug 13, 2020

Currently, CopyN uses LimitReader which returns no error in our case, so changing this behavior is a breaking change, isn't it?

That to me is a bug if you are asking a read of <0 e.g -2, and that showed itself as the current code let it fall through. Perhaps let’s meet in the middle and like I had alluded to earlier, you could mail that change separately.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Sep 12, 2020

Howdy @avivklas, I left some suggestions your change, please take a look and thank you!

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 15, 2020

Change https://golang.org/cl/254977 mentions this issue: net/http: test that ParseMultipartForm returns an error for int overflow

@avivklas
Copy link
Contributor Author

@avivklas avivklas commented Sep 15, 2020

Hey @odeke-em, I am truly sorry for stretching this. I have a physical condition in my arm that damages my productivity. I've just mailed the missing net/http test and applied your other suggestions.

I agree with you about CopyN and I'll mail another change later this week.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Sep 28, 2020

Thank you for this change @avivklas, and congrats on becoming a Go contributor! Looking forward to even more interactions with you on the Go project!

@gopherbot gopherbot closed this in 874b313 Sep 28, 2020
gopherbot pushed a commit that referenced this issue Oct 19, 2020
ParseMultipartForm has been changed to return an error if maxMemory
parameter + 10MB causes int overflows. This adds a test for the new
behaviour.

For #40430

Change-Id: I4f66ce8a9382940182011d22a84ee52b1d1364cf
Reviewed-on: https://go-review.googlesource.com/c/go/+/254977
Reviewed-by: Damien Neil <dneil@google.com>
Trust: Damien Neil <dneil@google.com>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 3, 2020

Change https://golang.org/cl/275112 mentions this issue: mime/multipart: handle ReadForm(math.MaxInt64) better

gopherbot pushed a commit that referenced this issue Dec 3, 2020
Returning an error about integer overflow is needlessly pedantic.
The meaning of ReadForm(MaxInt64) is easily understood
(accept a lot of data) and can be implemented.

Fixes #40430.

Change-Id: I8a522033dd9a2f9ad31dd2ad82cf08d553736ab9
Reviewed-on: https://go-review.googlesource.com/c/go/+/275112
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants