net/http/httputil: ReverseProxy should not forward unparseable query parameters [go1.16 backport] - CVE-2022-2880

[markapruett]

From Golang security announcement:

Requests forwarded by ReverseProxy included the raw query parameters from the inbound request, including unparseable parameters rejected by net/http. This could permit query parameter smuggling when a Go proxy forwards a parameter with an unparseable value.

ReverseProxy will now sanitize the query parameters in the forwarded query when the outbound request's Form field is set after the ReverseProxy.Director function returns, indicating that the proxy has parsed the query parameters. Proxies which do not parse query parameters continue to forward the original query parameters unchanged.

Thanks to Gal Goldstein (Security Researcher, Oxeye) and Daniel Abeles (Head of Research, Oxeye) for reporting this issue.

This is CVE-2022-2880 and Go issue https://go.dev/issue/54663.

[rcrozean]

Status: Patch is able to apply cleanly, but the added test fails. When trying to iterate through with Goland I get the following error: package std/net/http/httputil httputil.go:11:2: use of internal package net/http/internal not allowed and am unable to use goland to iterate through. I tried renaming the internal folder and imports to get around the internal package error, but to no success.

Looking through the blame for reverseproxy.go I wondered if a previous smuggling commit would be required, but it only appeared to break net/http and not fix the test.

[danbudris]

Digging into this, I ran through the net/http tests in the std lib with the goland debugger, and was able to see that the query parsing in net/url was treating the semicolon (;) as a valid query parameter separator. However, semicolons have since been deprecated as valid query parameter separators, and as of Go 1.17 the net/url package was updated to reflect this. So we'll need to take https://github.com/golang/go/commit/e6dda19888180c5159460486d30c0412e4980748 in order to ensure that we're doing the same in EKS Go 1.16.

[danbudris]

we can see in https://url.spec.whatwg.org/#urlencoded-parsing that now only supported separator is &

[danbudris]

ok, cut a new PR (⬆️ ) which uses the original patch for https://github.com/advisories/GHSA-m3hq-grv6-h853, but first patches https://github.com/golang/go/commit/e6dda19888180c5159460486d30c0412e4980748 so that we're not treating semicolons as acceptable path separators in URL query parameters. Compilation and tests all passing locally, running in pre-submit now.

[rcrozean]

@danbudris is this good to close with PR #563

[markapruett]

Top level tracking issue: https://github.com/aws/eks-distro-build-tooling/issues/620