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

Jump to bottom

net/http: bad handling of HEAD requests with a body #53960

Closed
neild opened this issue Jul 19, 2022 · 14 comments
Closed

net/http: bad handling of HEAD requests with a body #53960

neild opened this issue Jul 19, 2022 · 14 comments
Assignees
@neild
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.20

Comments

@neild
Copy link
Contributor

neild commented Jul 19, 2022

HEAD requests may have a body, although RFC 7231 states that "some existing implementations" may reject a HEAD request which contains one.

The net/http package handles HEAD requests with a body in different ways:

  • HTTP/1, non-chunked: return a 400 error, close the connection.
  • HTTP/1, Content-Encoding: chunked: ignore the chunked body (trying to parse it as the next request on the connection). Clearly buggy. Not a request smuggling mechanism, since the chunked body data can never be a valid HTTP request.
  • HTTP/2: close the stream with an error.

We should either support HEAD requests with a body in all circumstances, or fix the HTTP/1 chunked case and add a test for the HTTP/1 identity case. I think support, but I could be argued into always-reject on the grounds that nobody ever actually sends a body in a HEAD request.

Thanks to Bahruz Jabiyev, Tommaso Innocenti, Anthony Gavazzi, Steven Sprecher, and Kaan Onarlioglu for reporting this issue.
@neild neild self-assigned this Jul 19, 2022
@gopherbot
Copy link

Change https://go.dev/cl/418614 mentions this issue: net/http: accept HEAD requests with a body

@gopherbot
Copy link

Change https://go.dev/cl/418634 mentions this issue: http2: accept HEAD requests with a body

@nightlyone
Copy link
Contributor

@neild What can be gained in supporting such requests?

I would suggest to continue dropping the body and fix case 2 (trying to parse it as a next request) .

Supporting undefined behaviour seems like a road to incompatibilities.

Are you aware of a use case that relies on that is undefined behaviour or some IETF draft requiring this to work, so that the undefined behaviour becomes standardized soon?

@neild
Copy link
Contributor Author

neild commented Jul 20, 2022

There's no undefined behavior here.

The semantics of HEAD requests with a body are undefined--there's no RFC that I know of that defines what that body means--but the mechanics of sending such a request are well-defined. HEAD responses are defined as having no body, but HEAD requests may have one.

The argument I see against supporting HEAD requests with a body is that nobody sends these in practice, so receiving one probably indicates a mistake or malicious behavior of some kind.

The argument for supporting them is that they're valid requests, and that it's more work to reject them than it is to just handle them.

The actual state of support for HEAD-with-a-body in net/http is a bit of a mess as described above, so we need to do something. (The one case I forgot to mention is client behavior: net/http handles sending HEAD-with-a-body fine in all cases I've tested, including HTTP/1, HTTP/2, identity encoding, and chunked encoding.)

@seankhliao seankhliao added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 1, 2022
@seankhliao seankhliao added this to the Go1.20 milestone Aug 20, 2022
gopherbot pushed a commit to golang/net that referenced this issue Sep 19, 2022
@neild
http2: accept HEAD requests with a body
f8f703f 
RFC 7231 permits HEAD requests to contain a body, although it does
state there are no defined semantics for payloads of HEAD requests
and that some servers may reject HEAD requests with a payload.

Accept HEAD requests with a body.

Test is in net/http CL 418614.

For golang/go#53960.

Change-Id: I946d3ec796054c3908beb8a69cc78aa51c04c972
Reviewed-on: https://go-review.googlesource.com/c/net/+/418634
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/432197 mentions this issue: all: update vendored golang.org/x/net

gopherbot pushed a commit that referenced this issue Sep 21, 2022
@neild
all: update vendored golang.org/x/net
493ebc3 
Pull in HTTP/2 fix needed for net/http test case.

	f8f703f979 http2: accept HEAD requests with a body

For #53960

Change-Id: I59bbd83977daced5ed21ec5345af8cdb607e532e
Reviewed-on: https://go-review.googlesource.com/c/go/+/432197
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Sep 21, 2022
@neild
net/http: accept HEAD requests with a body
e246cf6 
RFC 7231 permits HEAD requests to contain a body, although it does
state there are no defined semantics for payloads of HEAD requests
and that some servers may reject HEAD requests with a payload.

Accept HEAD requests with a body.

Fix a bug where a HEAD request with a chunked body would interpret
the body as the headers for the next request on the connection.

For #53960.

Change-Id: I83f7112fdedabd6d6291cd956151d718ee6942cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/418614
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@neild neild closed this as completed Sep 22, 2022
@bobby-stripe
Copy link

@gopherbot please consider this for backport to 1.19, it's a serious problems with no workaround.

@gopherbot gopherbot mentioned this issue Oct 11, 2022
Closed
@gopherbot
Copy link

Backport issue(s) opened: #56154 (for 1.19).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@dr2chase dr2chase mentioned this issue Oct 19, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/450515 mentions this issue: doc/go1.20: add release notes for net/http and net/http/httputil

gopherbot pushed a commit that referenced this issue Nov 15, 2022
@neild @gopherbot
doc/go1.20: add release notes for net/http and net/http/httputil
8cca427 
For #41773
For #41773
For #50465
For #51914
For #53002
For #53896
For #53960
For #54136
For #54299

Change-Id: I729d5eafc1940d5706f980882a08ece1f69bb42c
Reviewed-on: https://go-review.googlesource.com/c/go/+/450515
Auto-Submit: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @neild
http2: accept HEAD requests with a body
b0b28d5 
RFC 7231 permits HEAD requests to contain a body, although it does
state there are no defined semantics for payloads of HEAD requests
and that some servers may reject HEAD requests with a payload.

Accept HEAD requests with a body.

Test is in net/http CL 418614.

For golang/go#53960.

Change-Id: I946d3ec796054c3908beb8a69cc78aa51c04c972
Reviewed-on: https://go-review.googlesource.com/c/net/+/418634
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/457275 mentions this issue: [release-branch.go1.18] net/http: accept HEAD requests with a body

@gopherbot
Copy link

Change https://go.dev/cl/457438 mentions this issue: [release-branch.go1.19] net/http: accept HEAD requests with a body

@gopherbot
Copy link

Change https://go.dev/cl/457416 mentions this issue: [internal-branch.go1.18-vendor] http2: accept HEAD requests with a body

@gopherbot
Copy link

Change https://go.dev/cl/457357 mentions this issue: [internal-branch.go1.19-vendor] http2: accept HEAD requests with a body

gopherbot pushed a commit to golang/net that referenced this issue Dec 14, 2022
@neild @thanm
[internal-branch.go1.18-vendor] http2: accept HEAD requests with a body
6143a13 
RFC 7231 permits HEAD requests to contain a body, although it does
state there are no defined semantics for payloads of HEAD requests
and that some servers may reject HEAD requests with a payload.

Accept HEAD requests with a body.

Test is in net/http CL 418614.

For golang/go#53960.
For golang/go#56323.

Change-Id: I946d3ec796054c3908beb8a69cc78aa51c04c972
Reviewed-on: https://go-review.googlesource.com/c/net/+/418634
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
(cherry picked from commit f8f703f)
Reviewed-on: https://go-review.googlesource.com/c/net/+/457416
Reviewed-by: Than McIntosh <thanm@google.com>
gopherbot pushed a commit to golang/net that referenced this issue Dec 14, 2022
@neild @thanm
[internal-branch.go1.19-vendor] http2: accept HEAD requests with a body
183621a 
RFC 7231 permits HEAD requests to contain a body, although it does
state there are no defined semantics for payloads of HEAD requests
and that some servers may reject HEAD requests with a payload.

Accept HEAD requests with a body.

Test is in net/http CL 418614.

For golang/go#53960.
For golang/go#56154.

Change-Id: I946d3ec796054c3908beb8a69cc78aa51c04c972
Reviewed-on: https://go-review.googlesource.com/c/net/+/418634
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
(cherry picked from commit f8f703f)
Reviewed-on: https://go-review.googlesource.com/c/net/+/457357
Reviewed-by: Than McIntosh <thanm@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/457596 mentions this issue: [release-branch.go1.18] all: upgrade golang.org/x/net to v0.0.0-20221214163811-6143a133e5c9

@gopherbot
Copy link

Change https://go.dev/cl/457556 mentions this issue: [release-branch.go1.19] all: upgrade golang.org/x/net to v0.0.0-20221214163817-183621ab9c4e

gopherbot pushed a commit that referenced this issue Dec 19, 2022
@neild @dr2chase
[release-branch.go1.19] all: upgrade golang.org/x/net to v0.0.0-20221…
7540675 
…214163817-183621ab9c4e

Update x/net to include the fix for #53960.

For #53960
For #56154

Change-Id: Ib3e0d66e4125601e20f1b2e3040f29e7ebd4b080
Reviewed-on: https://go-review.googlesource.com/c/go/+/457556
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
gopherbot pushed a commit that referenced this issue Dec 19, 2022
@neild @dr2chase
[release-branch.go1.18] all: upgrade golang.org/x/net to v0.0.0-20221…
d17cf56 
…214163811-6143a133e5c9

Update x/net to include the fix for #53960.

For #53960
For #56323

Change-Id: I825212ecdf7bf2f52c2fda1faf1739b593063653
Reviewed-on: https://go-review.googlesource.com/c/go/+/457596
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
gopherbot pushed a commit that referenced this issue Jan 30, 2023
@neild @mknyszek
[release-branch.go1.19] net/http: accept HEAD requests with a body
01a5a83 
RFC 7231 permits HEAD requests to contain a body, although it does
state there are no defined semantics for payloads of HEAD requests
and that some servers may reject HEAD requests with a payload.

Accept HEAD requests with a body.

Fix a bug where a HEAD request with a chunked body would interpret
the body as the headers for the next request on the connection.

For #53960.
For #56154.

Change-Id: I83f7112fdedabd6d6291cd956151d718ee6942cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/418614
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/457438
Reviewed-by: Than McIntosh <thanm@google.com>
@golang golang locked and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@neild neild

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
5 participants
@neild @nightlyone @gopherbot @seankhliao @bobby-stripe