net/http/httputil: NewSingleHostReverseProxy - omit X-Forwarded-For not working #53423

firefart · 2022-06-17T07:52:28Z

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

$ go version
go version go1.18 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=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/firefart/.cache/go-build"
GOENV="/home/firefart/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/firefart/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/firefart/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/firefart/code/zwiebelproxy/go.mod"
GOWORK=""
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-build534953705=/tmp/go-build -gno-record-gcc-switches"

What did you do?

In #38079 a check was added to not leak the IP if the X-Forwarded-For header is nil.

Doing this does not seem to work as it always return an empty string slice

r.Header["X-Forwarded-For"] = nil
fmt.Printf("%T", r.Header["X-Forwarded-For"])

Output:

[]string

What did you expect to see?

The header is not added

What did you see instead?

The IP header is always added as it seems impossible to set the value to nil

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

Line 301 in 9068c68

    
           omit := ok && prior == nil // Issue 38079: nil now means don't populate the header

The text was updated successfully, but these errors were encountered:

firefart · 2022-06-17T08:22:31Z

The actual issue lies in the Header.Clone method.

ServeHTTP calls req.Clone(ctx) over here:

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

Line 246 in 9068c68

outreq := req.Clone(ctx)

Clone then calls the Clone method on the Headers map:

go/src/net/http/request.go

Line 380 in 9068c68

r2.Header = r.Header.Clone()

I added a debug output before and after the r.Header.Clone() call and this is the output:

http.Header{"X-Forwarded-For":[]string(nil)}
-- Clone()
http.Header{"X-Forwarded-For":[]string{}}

So it looks like the Header.Clone() method does not preserve nil values in headers which makes the check for nil invalid and thus the client ip is always added.

This can be a privacy issue because currently golang is leaking the original client ip to the end targets even if the header is set to nil to prevent that

seankhliao · 2022-06-17T09:06:15Z

Seems to be working? https://go.dev/play/p/ME0dFW6gT1g

firefart · 2022-06-17T09:16:57Z

Nope. Here is a short play to demonstrate the underlying issue:
https://go.dev/play/p/c8Oc19Te76u

http.Header{"X-Forwarded-For":[]string(nil)}
http.Header{"X-Forwarded-For":[]string{}}

The Clone converts the nil value to an empty slice

Here is also some pseudocode for an http handler that forwards the request using httputil.NewSingleHostReverseProxy.

func (app *application) proxyHandler(w http.ResponseWriter, r *http.Request) {
	host, port, err := net.SplitHostPort(r.Host)
	if err != nil {
		// no port present
		host = r.Host
		port = r.URL.Port()
	}

	ctx, cancel := context.WithTimeout(r.Context(), app.timeout)
	defer cancel()
	r = r.WithContext(ctx)

	if port != "" && port != "80" && port != "443" {
		host = net.JoinHostPort(host, port)
	}
	r.URL.Host = host
	r.Host = host

	if r.URL.Scheme == "" {
		switch port {
		case "":
			r.URL.Scheme = "http"
		case "80":
			r.URL.Scheme = "http"
		case "443":
			r.URL.Scheme = "https"
		default:
			r.URL.Scheme = "http"
		}
	}

	// needed so the ip will not be leaked
	r.Header["X-Forwarded-For"] = nil

	app.logger.Debugf("port: %+v", port)
	app.logger.Debugf("r.URL: %+v", r.URL)
	app.logger.Debugf("r.RequestURI: %+v", r.RequestURI)
	app.logger.Debugf("r.Host: %+v", r.Host)
	app.logger.Debugf("r.Header: %+v", r.Header)

	proxy := httputil.NewSingleHostReverseProxy(r.URL)
	proxy.FlushInterval = -1
	proxy.ModifyResponse = app.modifyResponse
	proxy.Transport = app.httpClient.tr.Clone()

	app.logger.Debugf("sending request %+v", r)

	proxy.ServeHTTP(w, r)
}

If you add the debug statements on the clone method mentioned in the comment, you can see that the header value is not nil after cloning

seankhliao · 2022-06-17T09:28:15Z

I understood #38079 to mean allowing control over X-Forwarded-For from within the proxy, ie using your own Director, not from the outside.

cc @neild

firefart · 2022-06-17T09:55:48Z

Maybe, but the comment does not say this exactly, it only mentions the header value needs to be nil

// If an X-Forwarded-For header already exists, the client IP is
// appended to the existing values. As a special case, if the header
// exists in the Request.Header map but has a nil value (such as when
// set by the Director func), the X-Forwarded-For header is
// not modified.

I also think the Clone() function not handling nil values is a bit inconsistent as it modifies the original request.

neild · 2022-06-17T17:05:12Z

Given that ReverseProxy assigns meaning to nil header values, Header.Clone should probably preserve nil-ness.

gopherbot · 2022-06-17T17:12:36Z

Change https://go.dev/cl/412857 mentions this issue: net/http: preserve nil values in Header.Clone

neild · 2022-06-29T22:31:15Z

This has been assigned CVE-2022-32148, since it can lead to unintended leakage of private information (the client IP address).

neild · 2022-06-29T22:32:02Z

@gopherbot please open backport issues.

gopherbot · 2022-06-29T22:32:25Z

Backport issue(s) opened: #53620 (for 1.17), #53621 (for 1.18).

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

gopherbot · 2022-06-29T22:33:53Z

Change https://go.dev/cl/415221 mentions this issue: [release-branch.go1.17] net/http: preserve nil values in Header.Clone

gopherbot · 2022-06-29T22:34:25Z

Change https://go.dev/cl/415222 mentions this issue: [release-branch.go1.18] net/http: preserve nil values in Header.Clone

ReverseProxy makes a distinction between nil and zero-length header values. Avoid losing nil-ness when cloning a request. Thanks to Christian Mehlmauer for discovering this. For #53423 For CVE-2022-32148 Fixes #53620 Change-Id: Ice369cdb4712e2d62e25bb881b080847aa4801f5 Reviewed-on: https://go-review.googlesource.com/c/go/+/412857 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> (cherry picked from commit b2cc0fe) Reviewed-on: https://go-review.googlesource.com/c/go/+/415221 Reviewed-by: Heschi Kreinick <heschi@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Michael Knyszek <mknyszek@google.com> Run-TryBot: Heschi Kreinick <heschi@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>

ReverseProxy makes a distinction between nil and zero-length header values. Avoid losing nil-ness when cloning a request. Thanks to Christian Mehlmauer for discovering this. For #53423 For CVE-2022-32148 Fixes #53621 Change-Id: Ice369cdb4712e2d62e25bb881b080847aa4801f5 Reviewed-on: https://go-review.googlesource.com/c/go/+/412857 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> (cherry picked from commit b2cc0fe) Reviewed-on: https://go-review.googlesource.com/c/go/+/415222 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Michael Knyszek <mknyszek@google.com> Reviewed-by: Heschi Kreinick <heschi@google.com> Run-TryBot: Heschi Kreinick <heschi@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>

ReverseProxy makes a distinction between nil and zero-length header values. Avoid losing nil-ness when cloning a request. Thanks to Christian Mehlmauer for discovering this. For golang#53423 For CVE-2022-32148 Fixes golang#53621 Change-Id: Ice369cdb4712e2d62e25bb881b080847aa4801f5 Reviewed-on: https://go-review.googlesource.com/c/go/+/412857 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> (cherry picked from commit b2cc0fe) Reviewed-on: https://go-review.googlesource.com/c/go/+/415222 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Michael Knyszek <mknyszek@google.com> Reviewed-by: Heschi Kreinick <heschi@google.com> Run-TryBot: Heschi Kreinick <heschi@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>

ReverseProxy makes a distinction between nil and zero-length header values. Avoid losing nil-ness when cloning a request. Thanks to Christian Mehlmauer for discovering this. Fixes golang#53423 Fixes CVE-2022-32148 Change-Id: Ice369cdb4712e2d62e25bb881b080847aa4801f5 Reviewed-on: https://go-review.googlesource.com/c/go/+/412857 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

ReverseProxy makes a distinction between nil and zero-length header values. Avoid losing nil-ness when cloning a request. Thanks to Christian Mehlmauer for discovering this. For golang#53423 For CVE-2022-32148 Fixes golang#53620 Change-Id: Ice369cdb4712e2d62e25bb881b080847aa4801f5 Reviewed-on: https://go-review.googlesource.com/c/go/+/412857 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> (cherry picked from commit b2cc0fe) Reviewed-on: https://go-review.googlesource.com/c/go/+/415221 Reviewed-by: Heschi Kreinick <heschi@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Michael Knyszek <mknyszek@google.com> Run-TryBot: Heschi Kreinick <heschi@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>

# AWS EKS Backported To: go-1.15.15-eks Backported On: Thu, 22 Sept 2022 Backported By: budris@amazon.com Backported From: release-branch.go1.17 EKS Patch Source Commit: danbudris@a966897 Upstream Source Commit: golang@ed2f33e # Original Information ReverseProxy makes a distinction between nil and zero-length header values. Avoid losing nil-ness when cloning a request. Thanks to Christian Mehlmauer for discovering this. For golang#53423 For CVE-2022-32148 Fixes golang#53620 Change-Id: Ice369cdb4712e2d62e25bb881b080847aa4801f5 Reviewed-on: https://go-review.googlesource.com/c/go/+/412857 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> (cherry picked from commit b2cc0fe) Reviewed-on: https://go-review.googlesource.com/c/go/+/415221 Reviewed-by: Heschi Kreinick <heschi@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Michael Knyszek <mknyszek@google.com> Run-TryBot: Heschi Kreinick <heschi@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>

# AWS EKS Backported To: go-1.16.15-eks Backported On: Tue, 04 Oct 2022 Backported By: budris@amazon.com Backported From: release-branch.go1.17 EKS Patch Source Commit: danbudris@0cf92aa Upstream Source Commit: golang@ed2f33e # Original Information ReverseProxy makes a distinction between nil and zero-length header values. Avoid losing nil-ness when cloning a request. Thanks to Christian Mehlmauer for discovering this. For golang#53423 For CVE-2022-32148 Fixes golang#53620 Change-Id: Ice369cdb4712e2d62e25bb881b080847aa4801f5 Reviewed-on: https://go-review.googlesource.com/c/go/+/412857 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> (cherry picked from commit b2cc0fe) Reviewed-on: https://go-review.googlesource.com/c/go/+/415221 Reviewed-by: Heschi Kreinick <heschi@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Michael Knyszek <mknyszek@google.com> Run-TryBot: Heschi Kreinick <heschi@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>

firefart changed the title ~~net/http/httputil: omit X-Forwarded-For not working~~ net/http/httputil: NewSingleHostReverseProxy - omit X-Forwarded-For not working Jun 17, 2022

seankhliao added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 17, 2022

seankhliao added NeedsFix The path to resolution is known, but the work has not been done. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jun 18, 2022

seankhliao added this to the Go1.20 milestone Jun 18, 2022

gopherbot closed this as completed in b2cc0fe Jun 29, 2022

This was referenced Jun 29, 2022

net/http/httputil: NewSingleHostReverseProxy - omit X-Forwarded-For not working [1.17 backport] #53620

Closed

net/http/httputil: NewSingleHostReverseProxy - omit X-Forwarded-For not working [1.18 backport] #53621

Closed

dmitshur modified the milestones: Go1.20, Go1.19 Jul 6, 2022

neild added the Security label Jul 6, 2022

tatianab added the release-blocker label Jul 8, 2022

tatianab mentioned this issue Jul 14, 2022

x/vulndb: potential Go vuln in std: CVE-2022-32148 golang/vulndb#520

Closed

golang locked and limited conversation to collaborators Jul 8, 2023

gopherbot added the FrozenDueToAge label Jul 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http/httputil: NewSingleHostReverseProxy - omit X-Forwarded-For not working #53423

net/http/httputil: NewSingleHostReverseProxy - omit X-Forwarded-For not working #53423

firefart commented Jun 17, 2022 •

edited

firefart commented Jun 17, 2022 •

edited

seankhliao commented Jun 17, 2022

firefart commented Jun 17, 2022 •

edited

seankhliao commented Jun 17, 2022

firefart commented Jun 17, 2022

neild commented Jun 17, 2022

gopherbot commented Jun 17, 2022

neild commented Jun 29, 2022

neild commented Jun 29, 2022

gopherbot commented Jun 29, 2022

gopherbot commented Jun 29, 2022

gopherbot commented Jun 29, 2022

net/http/httputil: NewSingleHostReverseProxy - omit X-Forwarded-For not working #53423

net/http/httputil: NewSingleHostReverseProxy - omit X-Forwarded-For not working #53423

Comments

firefart commented Jun 17, 2022 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

firefart commented Jun 17, 2022 • edited

seankhliao commented Jun 17, 2022

firefart commented Jun 17, 2022 • edited

seankhliao commented Jun 17, 2022

firefart commented Jun 17, 2022

neild commented Jun 17, 2022

gopherbot commented Jun 17, 2022

neild commented Jun 29, 2022

neild commented Jun 29, 2022

gopherbot commented Jun 29, 2022

gopherbot commented Jun 29, 2022

gopherbot commented Jun 29, 2022

firefart commented Jun 17, 2022 •

edited

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

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

firefart commented Jun 17, 2022 •

edited

firefart commented Jun 17, 2022 •

edited