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: request with a + in Content-Length is considered valid #39017

Closed
juliens opened this issue May 12, 2020 · 15 comments
Closed

net/http: request with a + in Content-Length is considered valid #39017

juliens opened this issue May 12, 2020 · 15 comments
Labels
Milestone

Comments

@juliens
Copy link
Contributor

@juliens juliens commented May 12, 2020

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

$ go version
go version go1.14.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/juliens/.cache/go-build"
GOENV="/home/juliens/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GOOS="linux"
GOPATH="/home/juliens/dev/go"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/home/juliens/go-current"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/juliens/go-current/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build262375258=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I launch a server using

http.ListenAndServe(":8080", http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
    fmt.Println(req.ContentLength)
}))

and after that, I make a call with a Content-Length: +3

echo -ne "POST / HTTP/1.1\r\nContent-Length: +3\r\nHost: 127.0.0.1\r\n\r\naaa\r\n" | nc 127.0.0.1 8083

What did you expect to see?

A bad request ( because of the RFC https://tools.ietf.org/html/rfc2616#section-14.13 )

What did you see instead?

A valid request, the Content-Length is parsed as just 3

More

For what I saw, it's because the Content-Length is parsed by using strconv.ParseInt and so +3 is valid and becomes 3

@tpaschalis
Copy link
Contributor

@tpaschalis tpaschalis commented May 21, 2020

I'd like to take a stab if there's no objection from the Go team!

For my first attempt, I simply used strconv.ParseUint (which disallows the prefixed sign), to replace the strconv.ParseInt calls (that allow it), and then cast back to int64. I'm using ParseUint(cl, 10, 63) to leave that extra sign bit, so it's safe to convert back to an int64.

From some manual testing, this seems to do the trick. I've added some unit test that reference this issue for posterity and have opened a CL.

With that change, I get

func main() {
    http.ListenAndServe(":8080", http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
        fmt.Println(req.ContentLength)
    }))
## Before
$ go run main.go
$ echo -ne "POST / HTTP/1.1\r\nContent-Length: 3\r\nHost: 127.0.0.1\r\n\r\naaa\r\n" | nc 127.0.0.1 8080
HTTP/1.1 200 OK
...

$ echo -ne "POST / HTTP/1.1\r\nContent-Length: +3\r\nHost: 127.0.0.1\r\n\r\naaa\r\n" | nc 127.0.0.1 8080
HTTP/1.1 200 OK
...

## After
$ $GODIR/bin/go run main.go
$ echo -ne "POST / HTTP/1.1\r\nContent-Length: 3\r\nHost: 127.0.0.1\r\n\r\naaa\r\n" | nc 127.0.0.1 8080
HTTP/1.1 200 OK
...

$ echo -ne "POST / HTTP/1.1\r\nContent-Length: +3\r\nHost: 127.0.0.1\r\n\r\naaa\r\n" | nc 127.0.0.1 8080
HTTP/1.1 400 Bad Request
...
@gopherbot
Copy link

@gopherbot gopherbot commented May 21, 2020

Change https://golang.org/cl/234817 mentions this issue: net/http: Disallow plus symbol in Content-Length

@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 23, 2020

Thank you for the report @juliens! Thank you for working on the fix @tpaschalis, awesome!
I've added some suggestions to your CL, please take a look. Our HTTP/2 stack purposefully skips checking for errors from strconv.ParseInt, because such errors don't necessarily affect our framing. Unfortunately this increases the surface area for what could change be broken within this release that we are already in.

@bradfitz would it be pragmatic for us to firstly accept only the fix for HTTP/1.1 for Go1.15? For Go1.16 make the follow-on update in x/net/http2 and subsequently to net/http/h2_bundle.go? We ignore the errors anyways in HTTP/2 so the change could perhaps survive one more release.

gopherbot pushed a commit that referenced this issue May 31, 2020
Enforces section 14.13 of RFC 2616 so that Content-Length header
values with a sign such as "+5" will be rejected.

Updates #39017

Change-Id: Icce9f00d03c8475fe704b33f9bed9089ff8802f0
Reviewed-on: https://go-review.googlesource.com/c/go/+/234817
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 3, 2020

Change https://golang.org/cl/236098 mentions this issue: x/net/http2: reject HTTP/2 Content-Length headers containing a sign

@tpaschalis
Copy link
Contributor

@tpaschalis tpaschalis commented Jun 8, 2020

@odeke-em sorry for pinging you! A few days ago, I opened a similar CL for HTTP/2.

I'm not sure if I should put you as a reviewer since we had some communication for the HTTP/1.1 part, or if you're busy with other things; in any case I'd be grateful if you could take a look! Thanks in advance.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 26, 2020

Too late for 1.15. Moving to Backlog.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.15, Backlog Jun 26, 2020
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 30, 2020

HTTP/2 Content-Lengths are indeed not security-relevant, so now that this is fixed in HTTP/1.1, it's not a security issue anymore. Even in HTTP/1.1, this was only an issue when paired with a misbehaving proxy that forwards invalid Content-Length headers that it ignored, when it's supposed to strip or reject them, so we consider this a security hardening fix, and it won't be backported.

We'd like to also thank Amit Klein, Safebreach for reporting this to security@golang.org before this issue was opened.

In the process of looking into this, we found another hardening fix I'll request a freeze exception for in a moment: leading zeroes are accepted the same way a leading + is. The HTTP/2 fix will probably want to fix that as well for Go 1.16.

@FiloSottile FiloSottile removed the Security label Jun 30, 2020
@FiloSottile FiloSottile removed their assignment Jun 30, 2020
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 30, 2020

In the process of looking into this, we found another hardening fix I'll request a freeze exception for in a moment: leading zeroes are accepted the same way a leading + is. The HTTP/2 fix will probably want to fix that as well for Go 1.16.

Sigh, scratch that. Turns out that leading zeroes are wholly unspecified. Someone tried to make an argument for a better spec in 2008 but was met with strong resistance.

This goes to show that

  1. making header parsing and semantics critical to the security of the protocol framing was a catastrophic design decision; and
  2. it's simply impossible for implementations to agree on the exact same set of valid headers, so the only countermeasure to request smuggling is for proxies not to forward headers they did not parse successfully.

(Request smuggling can still occur when two implementations both find a header valid, but disagree on its meaning, but in that case it's likely one of them is simply wrong.)

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jun 30, 2020

Shall we make backports for Go1.13 and Go1.14 for this issue for HTTP/1.1?

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 30, 2020

Shall we make backports for Go1.13 and Go1.14 for this issue for HTTP/1.1?

No need, we decided we'd land this as a hardening measure, not as a security fix.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jun 30, 2020

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 5, 2020

Change https://golang.org/cl/253238 mentions this issue: src/go.mod, net/http: update bundled and latest golang.org/x/net

gopherbot pushed a commit that referenced this issue Sep 5, 2020
Updates x/net/http2 to git rev 62affa334b73ec65ed44a326519ac12c421905e3

    x/net/http2: reject HTTP/2 Content-Length headers containing a sign
    https://go-review.googlesource.com/c/net/+/236098/ (fixes #39017)

also updates the vendored version of golang.org/x/net by running

go get golang.org/x/net@62affa334b73ec65ed44a326519ac12c421905e3
go mod tidy
go mod vendor
go generate -run bundle net/http

Change-Id: I7ecfdb7644574c44c3616e3b47664eefd4c926f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/253238
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 13, 2020

Change https://golang.org/cl/261919 mentions this issue: [release-branch.go1.15] src, net/http: update vendor, h2_bundle.go regeneration

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 13, 2020

Change https://golang.org/cl/258540 mentions this issue: [release-branch.go1.15] src, net/http: update vendor, regenerate h2_bundle.go

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 13, 2020

Change https://golang.org/cl/258538 mentions this issue: [release-branch.go1.14] src, net/http: update vendor, regenerate h2_bundle.go

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
7 participants
You can’t perform that action at this time.