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 [1.19 backport] #56154

Closed
gopherbot opened this issue Oct 11, 2022 · 12 comments
Closed

net/http: bad handling of HEAD requests with a body [1.19 backport] #56154

gopherbot opened this issue Oct 11, 2022 · 12 comments
Assignees
@neild
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.19.6

Comments

@gopherbot
Copy link

@bobby-stripe requested issue #53960 to be considered for backport to the next 1.19 minor release.

@gopherbot please consider this for backport to 1.19, it's a serious problems with no workaround.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Oct 11, 2022
@gopherbot gopherbot mentioned this issue Oct 11, 2022
Closed
@gopherbot gopherbot added this to the Go1.19.3 milestone Oct 11, 2022
@bobby-stripe
Copy link

We have a Go service that results in user-facing 503s when it gets a HEAD request with a body, and as this happens so low in the net/http stack we don't have a good way to distinguish these 5xxs from more actionable errors. I believe this meets the criteria for a backport from MinorReleases, but happy to discuss more!

@neild
Copy link
Contributor

neild commented Oct 12, 2022

I don't have a strong opinion on whether this should be backported.

The impact is that we treat a HEAD request with a body as a protocol error. HEAD requests with a body are uncommon enough that (AFAIK) the first report of the problem was found via fuzzing.

This is not a security issue. It has no workaround, however, so the question for whether it meets the backport criteria turns on whether it's serious or not.

@dr2chase dr2chase added the CherryPickApproved Used during the release process for point releases label Oct 19, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Oct 19, 2022
@dr2chase dr2chase mentioned this issue Oct 19, 2022
Closed
@dr2chase
Copy link
Contributor

See also #56323.

@gopherbot gopherbot modified the milestones: Go1.19.3, Go1.19.4 Nov 1, 2022
@mknyszek
Copy link
Contributor

mknyszek commented Nov 8, 2022

Ping for updates here to make sure this approved cherry-pick is moves along. It's been approved for about 20 days now.

@gopherbot gopherbot modified the milestones: Go1.19.4, Go1.19.5 Dec 6, 2022
@bobby-stripe
Copy link

another ping here -- @dr2chase is this something you can help with?

@neild neild self-assigned this Dec 7, 2022
@dr2chase
Copy link
Contributor

Sorry for late reply, net/http is not my area and there's a high risk I'd make a mistake. I did the paperwork for the backport because I was on release-rotation that week. @neild (who assigned himself to the bug 3 days ago) is the right person.

@gopherbot
Copy link
Author

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

@gopherbot
Copy link
Author

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.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
Author

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
Copy link
Author

Closed by merging 7540675 to release-branch.go1.19.

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>
@neild
Copy link
Contributor

neild commented Jan 26, 2023

The backport for the HTTP/1 fix was never merged, thanks to none of us noticing that Gopherbot had prematurely closed this issue (#29599). The HTTP/2 fix was merged, however.

Reopening to merge the HTTP/1 fix (https://go.dev/cl/457438) for 1.19.6.

@neild neild reopened this Jan 26, 2023
@neild neild modified the milestones: Go1.19.5, Go1.19.6 Jan 26, 2023
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>
@mknyszek
Copy link
Contributor

CL 457438 was merged, but lacked a "Fixes" line (I think it was just a typo). Closing.

@golang golang locked and limited conversation to collaborators Jan 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@neild neild

Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.19.6
Development

No branches or pull requests
5 participants
@neild @mknyszek @dr2chase @gopherbot @bobby-stripe