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

[FiloSottile]

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.

[dmitshur]

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]

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

[gopherbot]

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

[gopherbot]

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]

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

[gopherbot]

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