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.

Sign up for GitHub

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/httputil: ReverseProxy forwards Connection headers if first one is empty #46313

Closed
FiloSottile opened this issue May 21, 2021 · 6 comments
Closed

net/http/httputil: ReverseProxy forwards Connection headers if first one is empty #46313

FiloSottile opened this issue May 21, 2021 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Security
Milestone
Go1.17

Comments

@FiloSottile
Copy link
Contributor

Per RFC 7230, Section 6.1, the Connection header needs to be removed by proxies, along with headers named in the Connection header field. Per Section 3.2.2, multiple Connection headers should be considered the same as a single Connection header with a comma-separated concatenation of their fields.

ReverseProxy fails to delete the Connection headers (as well as other legacy hop-by-hop headers, which however per RFC 7230 need to also be specified in Connection) if there are multiple ones and the first is empty, due to an incorrect Get(h) == "" check.

go/src/net/http/httputil/reverseproxy.go

Lines 250 to 265 in 2ebe77a

for _, h := range hopHeaders {
hv := outreq.Header.Get(h)
if hv == "" {
continue
}
if h == "Te" && hv == "trailers" {
// Issue 21096: tell backend applications that
// care about trailer support that we support
// trailers. (We do, but we don't go out of
// our way to advertise that unless the
// incoming client request thought it was
// worth mentioning)
continue
}
outreq.Header.Del(h)
}

This can lead to a security issue if the proxy is adding an important header, like X-Forwarded-For, and is sitting in front of another proxy which can be instructed by an attacker to drop that header as a hop-by-hop header. https://nathandavison.com/blog/abusing-http-hop-by-hop-request-headers

This is tracked as CVE-2021-33197.
@FiloSottile FiloSottile added Security NeedsFix The path to resolution is known, but the work has not been done. labels May 21, 2021
@FiloSottile FiloSottile added this to the Go1.17 milestone May 21, 2021
@dmitshur
Copy link
Contributor

dmitshur commented May 21, 2021 •
edited

If it's not okay to release Go 1.17 without a fix for this issue, please add the release-blocker label. If okay to fix this after beta 1, please also add okay-after-beta1.

(If the Security label implies a release blocker, we can update release tooling, but I'm not sure if all issues with a Security label in a given milestone must block that release.)

@FiloSottile
Copy link
Contributor Author

@gopherbot please open backport issues for this, according to the security policy.

@gopherbot
Copy link

Change https://golang.org/cl/321929 mentions this issue: net/http/httputil: always remove hop-by-hop headers

This was referenced May 21, 2021
Closed
Closed
@gopherbot
Copy link

Backport issue(s) opened: #46314 (for 1.15), #46315 (for 1.16).

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

@gopherbot
Copy link

Change https://golang.org/cl/323090 mentions this issue: [release-branch.go1.16] net/http/httputil: always remove hop-by-hop headers

@gopherbot
Copy link

Change https://golang.org/cl/323091 mentions this issue: [release-branch.go1.15] net/http/httputil: always remove hop-by-hop headers

gopherbot pushed a commit that referenced this issue May 28, 2021
@FiloSottile @katiehockman
[release-branch.go1.16] net/http/httputil: always remove hop-by-hop h…
0410005 
…eaders

Previously, we'd fail to remove the Connection header from a request
like this:

    Connection:
    Connection: x-header

Updates #46313
Fixes #46315
Fixes CVE-2021-33197

Change-Id: Ie3009e926ceecfa86dfa6bcc6fe14ff01086be7d
Reviewed-on: https://go-review.googlesource.com/c/go/+/321929
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Katie Hockman <katie@golang.org>
Trust: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
(cherry picked from commit 950fa11)
Reviewed-on: https://go-review.googlesource.com/c/go/+/323090
gopherbot pushed a commit that referenced this issue May 28, 2021
@FiloSottile @katiehockman
[release-branch.go1.15] net/http/httputil: always remove hop-by-hop h…
cbd1ca8 
…eaders

Previously, we'd fail to remove the Connection header from a request
like this:

    Connection:
    Connection: x-header

Updates #46313
Fixes #46314
Fixes CVE-2021-33197

Change-Id: Ie3009e926ceecfa86dfa6bcc6fe14ff01086be7d
Reviewed-on: https://go-review.googlesource.com/c/go/+/321929
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Katie Hockman <katie@golang.org>
Trust: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/323091
Run-TryBot: Katie Hockman <katie@golang.org>
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jun 4, 2021
@bsiegert
Update go115 to 1.15.13.
35127b1 
go1.15.13 (released 2021-06-03) includes security fixes to the archive/zip,
math/big, net, and net/http/httputil packages, as well as bug fixes to the
linker, the go command, and the math/big and net/http packages. See the Go
1.15.13 milestone on our issue tracker for details.

The SetString and UnmarshalText methods of math/big.Rat
<https://pkg.go.dev/math/big#Rat> may cause a panic or an unrecoverable
fatal error if passed inputs with very large exponents.
This is issue <golang/go#44910> and
CVE-2021-33198.

Thanks to the OSS-Fuzz project for discovering this issue and to Emmanuel
Odeke for reporting it.

ReverseProxy in net/http/httputil <https://pkg.go.dev/net/http/httputil> could
be made to forward certain hop-by-hop headers, including Connection. In
case the target of the ReverseProxy was itself a reverse proxy, this would
let an attacker drop arbitrary headers, including those set by the
ReverseProxy.Director.
This is issue <golang/go#46313> and
CVE-2021-33197.

Thanks to Mattias Grenfeldt (https://grenfeldt.dev) and Asta Olofsson for
reporting this issue.

The LookupCNAME, LookupSRV, LookupMX, LookupNS, and LookupAddr functions in
net <https://pkg.go.dev/net>, and their respective methods on the Resolver
<https://pkg.go.dev/net#Resolver> type may return arbitrary values
retrieved from DNS which do not follow the established RFC 1035
<https://datatracker.ietf.org/doc/html/rfc1035>rules for domain names. If
these names are used without further sanitization, for instance unsafely
included in HTML, they may allow for injection of unexpected content. Note
that LookupTXT may still return arbitrary values that could require
sanitization before further use.
This is issue <golang/go#46241> and
CVE-2021-33195.

Thanks to Philipp Jeitner and Haya Shulman from Fraunhofer SIT for
reporting this issue.

The NewReader and OpenReader functions in archive/zip
<https://pkg.go.dev/archive/zip> can cause a panic or an unrecoverable
fatal error when reading an archive that claims to contain a large number
of files, regardless of its actual size.
This is issue <https://github.com/golang/go/issues/46242>and
CVE-2021-33196.

Thanks to the OSS-Fuzz project for discovering this issue and to Emmanuel
Odeke for reporting it.
mholt added a commit to caddyserver/caddy that referenced this issue Jun 4, 2021
@mholt
reverseproxy: Always remove hop-by-hop headers
2a81094 
See golang/go#46313

Based on golang/go@950fa11
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jun 5, 2021
@bsiegert
Update go116 to 1.16.5.
f32db56 
go1.16.5 (released 2021-06-03) includes security fixes to the archive/zip, math
/big, net, and net/http/httputil packages, as well as bug fixes to the linker,
the go command, and the net/http package. See the Go 1.16.5 milestone on our
issue tracker for details.

The SetString and UnmarshalText methods of math/big.Rat
<https://pkg.go.dev/math/big#Rat> may cause a panic or an unrecoverable
fatal error if passed inputs with very large exponents.
This is issue <golang/go#44910> and
CVE-2021-33198.

Thanks to the OSS-Fuzz project for discovering this issue and to Emmanuel
Odeke for reporting it.

ReverseProxy in net/http/httputil <https://pkg.go.dev/net/http/httputil> could
be made to forward certain hop-by-hop headers, including Connection. In
case the target of the ReverseProxy was itself a reverse proxy, this would
let an attacker drop arbitrary headers, including those set by the
ReverseProxy.Director.
This is issue <golang/go#46313> and
CVE-2021-33197.

Thanks to Mattias Grenfeldt (https://grenfeldt.dev) and Asta Olofsson for
reporting this issue.

The LookupCNAME, LookupSRV, LookupMX, LookupNS, and LookupAddr functions in
net <https://pkg.go.dev/net>, and their respective methods on the Resolver
<https://pkg.go.dev/net#Resolver> type may return arbitrary values
retrieved from DNS which do not follow the established RFC 1035
<https://datatracker.ietf.org/doc/html/rfc1035>rules for domain names. If
these names are used without further sanitization, for instance unsafely
included in HTML, they may allow for injection of unexpected content. Note
that LookupTXT may still return arbitrary values that could require
sanitization before further use.
This is issue <golang/go#46241> and
CVE-2021-33195.

Thanks to Philipp Jeitner and Haya Shulman from Fraunhofer SIT for
reporting this issue.

The NewReader and OpenReader functions in archive/zip
<https://pkg.go.dev/archive/zip> can cause a panic or an unrecoverable
fatal error when reading an archive that claims to contain a large number
of files, regardless of its actual size.
This is issue <https://github.com/golang/go/issues/46242>and
CVE-2021-33196.

Thanks to the OSS-Fuzz project for discovering this issue and to Emmanuel
Odeke for reporting it.
@hiroakis hiroakis mentioned this issue Nov 13, 2021
Open
@golang golang locked and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Security
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
3 participants
@FiloSottile @dmitshur @gopherbot