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
Comments
I'd like to take a stab if there's no objection from the Go team! For my first attempt, I simply used 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
## 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
... |
Change https://golang.org/cl/234817 mentions this issue: |
Thank you for the report @juliens! Thank you for working on the fix @tpaschalis, awesome! @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. |
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>
Change https://golang.org/cl/236098 mentions this issue: |
@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. |
Too late for 1.15. Moving to Backlog. |
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 |
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
(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.) |
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. |
Cool, thanks!
…On Mon, Jun 29, 2020 at 6:16 PM Filippo Valsorda ***@***.***> wrote:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#39017 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFL3V3DH5BB4WOBWXRGAHTRZE4GXANCNFSM4M63G7DQ>
.
|
Change https://golang.org/cl/253238 mentions this issue: |
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>
Change https://golang.org/cl/261919 mentions this issue: |
Change https://golang.org/cl/258540 mentions this issue: |
Change https://golang.org/cl/258538 mentions this issue: |
Continuing the work of CL 234817, we enforce the same for HTTP/2 connections; that Content-Length header values with a sign (such as "+5") are rejected or ignored. In each place, the handling of such incorrect headers follows the approach already in place. Fixes golang/go#39017 Change-Id: Ie4854962dd0091795cb861d1cb3a78ce753fd3a8 Reviewed-on: https://go-review.googlesource.com/c/net/+/236098 Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
I launch a server using
and after that, I make a call with a
Content-Length: +3
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 usingstrconv.ParseInt
and so+3
is valid and becomes3
The text was updated successfully, but these errors were encountered: