net/http: reject negative suffix-length Range:bytes=--N with 416 status code

Fixes the file server to reject requests of the form:
    "Range": "bytes=--N"
where "-N" is a negative suffix-length as designated by the
grammar in RFC 7233 Section 2.1, "Byte-Ranges", which specifies
that suffix-length MUST be of the form 1*DIGIT aka a non-negative digit.

Thus requests such as:
    "Range": "bytes=--2"
will be rejected with a "416 Range Not Satisfiable" response.

Fixes #40940

Change-Id: I3e89f8326c14af30d8bdb126998a50e02ba002d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/252497
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>

2 files changed, 66 insertions, 2 deletions

diff --git a/src/net/http/fs.go b/src/net/http/fs.go
index 922706ada1..d718fffba0 100644
--- a/src/net/http/fs.go
+++ b/src/net/http/fs.go
@@ -771,9 +771,15 @@ func parseRange(s string, size int64) ([]httpRange, error) {
 		var r httpRange
 		if start == "" {
 			// If no start is specified, end specifies the
-			// range start relative to the end of the file.
+			// range start relative to the end of the file,
+			// and we are dealing with <suffix-length>
+			// which has to be a non-negative integer as per
+			// RFC 7233 Section 2.1 "Byte-Ranges".
+			if end == "" || end[0] == '-' {
+				return nil, errors.New("invalid range")
+			}
 			i, err := strconv.ParseInt(end, 10, 64)
-			if err != nil {
+			if i < 0 || err != nil {
 				return nil, errors.New("invalid range")
 			}
 			if i > size {
diff --git a/src/net/http/fs_test.go b/src/net/http/fs_test.go
index 245d9ce65c..4ac73b728f 100644
--- a/src/net/http/fs_test.go
+++ b/src/net/http/fs_test.go
@@ -1316,3 +1316,61 @@ func Test_scanETag(t *testing.T) {
 		}
 	}
 }
+
+// Issue 40940: Ensure that we only accept non-negative suffix-lengths
+// in "Range": "bytes=-N", and should reject "bytes=--2".
+func TestServeFileRejectsInvalidSuffixLengths_h1(t *testing.T) {
+	testServeFileRejectsInvalidSuffixLengths(t, h1Mode)
+}
+func TestServeFileRejectsInvalidSuffixLengths_h2(t *testing.T) {
+	testServeFileRejectsInvalidSuffixLengths(t, h2Mode)
+}
+
+func testServeFileRejectsInvalidSuffixLengths(t *testing.T, h2 bool) {
+	defer afterTest(t)
+	cst := httptest.NewUnstartedServer(FileServer(Dir("testdata")))
+	cst.EnableHTTP2 = h2
+	cst.StartTLS()
+	defer cst.Close()
+
+	tests := []struct {
+		r        string
+		wantCode int
+		wantBody string
+	}{
+		{"bytes=--6", 416, "invalid range\n"},
+		{"bytes=--0", 416, "invalid range\n"},
+		{"bytes=---0", 416, "invalid range\n"},
+		{"bytes=-6", 206, "hello\n"},
+		{"bytes=6-", 206, "html says hello\n"},
+		{"bytes=-6-", 416, "invalid range\n"},
+		{"bytes=-0", 206, ""},
+		{"bytes=", 200, "index.html says hello\n"},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.r, func(t *testing.T) {
+			req, err := NewRequest("GET", cst.URL+"/index.html", nil)
+			if err != nil {
+				t.Fatal(err)
+			}
+			req.Header.Set("Range", tt.r)
+			res, err := cst.Client().Do(req)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if g, w := res.StatusCode, tt.wantCode; g != w {
+				t.Errorf("StatusCode mismatch: got %d want %d", g, w)
+			}
+			slurp, err := ioutil.ReadAll(res.Body)
+			res.Body.Close()
+			if err != nil {
+				t.Fatal(err)
+			}
+			if g, w := string(slurp), tt.wantBody; g != w {
+				t.Fatalf("Content mismatch:\nGot:  %q\nWant: %q", g, w)
+			}
+		})
+	}
+}