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

net/http: FileServer incorrectly parses Range header when a range starts with two minus signs #40940

Closed
disconnect3d opened this issue Aug 20, 2020 · 3 comments
Assignees
Labels
Milestone

Comments

@disconnect3d
Copy link

@disconnect3d disconnect3d commented Aug 20, 2020

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

$ go version
go version go1.14.7 darwin/amd64

Does this issue reproduce with the latest release?

Yes, it reproduces with go version go1.15 darwin/amd64.

What did you do?

If we launch the server below (and create a /tmp/index.html with a abc\n content) it incorrectly parses Range header such that contains a single value with two minus signs and responds with an unexpected 206 reply along with an incorrect Content-Range header value.

This can be tested by invoking curl -v -H 'Range: bytes=--2' localhost:3000/.

This bug seems to be not exploitable, at least not with a sane filesystem implementation.

For more details on what causes the bug see the "Investigation" section of this issue.

package main

import "net/http"

func main() {
  http.Handle("/", http.FileServer(http.Dir("/tmp")))
  http.ListenAndServe(":3000", nil)
}

What did you expect to see?

I expected to get the same reply as when an invalid range header value is sent, e.g.:

$ curl -v -H 'Range: bytes=a' localhost:3000/
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 3000 (#0)
> GET / HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.72.0
> Accept: */*
> Range: bytes=a
>
< HTTP/1.1 416 Requested Range Not Satisfiable
< Content-Type: text/plain; charset=utf-8
< Last-Modified: Thu, 20 Aug 2020 22:43:19 GMT
< X-Content-Type-Options: nosniff
< Date: Thu, 20 Aug 2020 22:43:36 GMT
< Content-Length: 14
<
invalid range
* Connection #0 to host localhost left intact
* Closing connection 0

What did you see instead?

I got a 206 reply with an incorrect Content-Range: bytes 6-3/4 header value:

$ curl -v -H 'Range: bytes=--2' localhost:3000/
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 3000 (#0)
> GET / HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.72.0
> Accept: */*
> Range: bytes=--2
>
< HTTP/1.1 206 Partial Content
< Accept-Ranges: bytes
< Content-Range: bytes 6-3/4
< Content-Type: text/html; charset=utf-8
< Last-Modified: Thu, 20 Aug 2020 22:43:19 GMT
< Date: Thu, 20 Aug 2020 22:45:24 GMT
< Transfer-Encoding: chunked
<
* Connection #0 to host localhost left intact
* Closing connection 0

Additionally, the server removed the Content-Length header due to its invalid value and printed out a warning about it:

2020/08/21 00:43:31 http: invalid Content-Length of "-2"

Investigation

The issue comes from the following code:

go/src/net/http/fs.go

Lines 749 to 783 in e94544c

// parseRange parses a Range header string as per RFC 7233.
// errNoOverlap is returned if none of the ranges overlap.
func parseRange(s string, size int64) ([]httpRange, error) {
if s == "" {
return nil, nil // header not present
}
const b = "bytes="
if !strings.HasPrefix(s, b) {
return nil, errors.New("invalid range")
}
var ranges []httpRange
noOverlap := false
for _, ra := range strings.Split(s[len(b):], ",") {
ra = textproto.TrimString(ra)
if ra == "" {
continue
}
i := strings.Index(ra, "-")
if i < 0 {
return nil, errors.New("invalid range")
}
start, end := textproto.TrimString(ra[:i]), textproto.TrimString(ra[i+1:])
var r httpRange
if start == "" {
// If no start is specified, end specifies the
// range start relative to the end of the file.
i, err := strconv.ParseInt(end, 10, 64)
if err != nil {
return nil, errors.New("invalid range")
}
if i > size {
i = size
}
r.start = size - i
r.length = size - r.start

The parseRange function takes the Range header, removes the bytes= prefix from it and splits it with the "," character. Later, for each split string or rather "range" it splits it further by "-". Then, it retrieves start and end values.

When there is only one of the values, the start is an empty string. The program then goes to the first branch and parses the end value with the ParseInt(end, 10, 64) call.

To give an example, if we pass bytes=--2 Range header we end up with start="", end="-2" strings and the end is then parsed with the ParseInt(end, 10, 64) call to -2 int64 value.

The program then fills in the r httpRange fields in:

			r.start = size - i
			r.length = size - r.start

Where, the size is just the total file size. With our example, we end up having r.start=size-(-2)=size+2 and r.length=size-(size+2)=-2. This result in what we see in the Content-Range header value.

Later, this httpRange value is used in the content.Seek call where the content's file pointer is moved past its end:

if _, err := content.Seek(ra.start, io.SeekStart); err != nil {

This operation does not fail (at least on my filesystem and for the bytes=--2 Range header) and a io.CopyN function is then called with a sendSize of -2 value here:

io.CopyN(w, sendContent, sendSize)

This ends up in copying no bytes from the content file pointer due to negative sendSize value and so returning no content in the request response body.

An additional observation

Additionally, the r.start = size - i calculation can overflow if we pass a very small int64 value after the bytes=-. However, this results in r.start having a negative value and making the content.Seek call fail. This can be observed in the following request/response:

$ curl -v -H 'Range: bytes=--9223372036854775808' localhost:3000/
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 3000 (#0)
> GET / HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.72.0
> Accept: */*
> Range: bytes=--9223372036854775808
>
< HTTP/1.1 416 Requested Range Not Satisfiable
< Content-Type: text/plain; charset=utf-8
< Last-Modified: Thu, 20 Aug 2020 22:43:19 GMT
< X-Content-Type-Options: nosniff
< Date: Thu, 20 Aug 2020 23:17:59 GMT
< Content-Length: 39
<
seek /tmp/index.html: invalid argument
* Connection #0 to host localhost left intact
* Closing connection 0

How can we fix this?

It seems like the easiest fix is to validate that the resulting i value from the i, err := strconv.ParseInt(end, 10, 64) call is not negative, and if that is, return an "invalid range" error.

On the other hand, maybe the function should also allow for all possible ranges for a given architecture and so use uints64 instead? This would be nice, however it would not be as easy to keep supporting the bytes=-2

@disconnect3d disconnect3d changed the title net/http: FileServer incorrectly parses Range header where a single bytes value starts with two minus signs net/http: FileServer incorrectly parses Range header when a range starts with two minus signs Aug 20, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Aug 21, 2020

Thank you for the report and the analysis.

/cc @bradfitz @rsc @bcmills

@dmitshur dmitshur added this to the Backlog milestone Aug 21, 2020
@odeke-em odeke-em self-assigned this Aug 22, 2020
@odeke-em
Copy link
Member

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

Thank you for this report @disconnect3d, welcome to the Go project! Thank you @dmitshur for the triage and welcome!

@disconnect3d by the Range header definition at:
Given the definition from https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Range
If we've got bytes=-N, we are now dealing with grammar definition bytes=-<SUFFIX_LENGTH>
Screen Shot 2020-09-02 at 12 31 25 AM
as well as RFC 7233 Section 2.1, "Byte-Ranges",
any suffix-length that is NOT positive should be reported back with a 416, Range not satisifable.

I shall send a CL shortly.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 2, 2020

Change https://golang.org/cl/252497 mentions this issue: net/http: reject negative suffix-length Range:bytes=--N with 416 status code

@gopherbot gopherbot closed this in ef20f76 Sep 2, 2020
@dmitshur dmitshur added NeedsFix and removed NeedsInvestigation labels Sep 2, 2020
@dmitshur dmitshur modified the milestones: Backlog, Go1.16 Sep 2, 2020
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
4 participants
You can’t perform that action at this time.