net/http: reject negative suffix-length Range:bytes=--N with 416 stat…

…us 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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Bryan C. Mills <[email protected]>

doc/go1.16.html

@@ -167,3 +167,9 @@ <h3 id="minor_library_changes">Minor changes to the library</h3>
  handler serves a 404 instead of its previous behavior of invoking the
  underlying handler with a mismatched <code>Path</code>/<code>RawPath</code> pair.
 </p>
+
+<p>
+ The <a href="/pkg/net/http/"><code>net/http</code></a> package now rejects HTTP range requests
+ of the form <code>"Range": "bytes=--N"</code> where <code>"-N"</code> is a negative suffix length, for
+ example <code>"Range": "bytes=--2"</code>. It now replies with a <code>416 "Range Not Satisfiable"</code> response.
+</p>

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 {

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)
+ }
+ })
+ }
+}