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

net/http/httputil: ReverseProxy should not forward unparseable query parameters #54663

Closed
neild opened this issue Aug 24, 2022 · 11 comments
Closed
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone

Comments

@neild
Copy link
Contributor

neild commented Aug 24, 2022

We generally treat malformed value pairs in URL queries as a soft error, ignoring the invalid pair but accepting others. For example, the following (playground link) accepts the value for the key b even as it rejects the invalid one for a:

u, _ := url.Parse("http://go.dev/?a=%x&b=ok")
v, err := url.ParseQuery(u.RawQuery)
fmt.Println(v, err)
// map[b:[ok]] invalid URL escape "%x"

ReverseProxy should not include unparseable query parameters when forwarding a request, since this is a vector for parameter smuggling. In Go 1.17, we changed URL parsing to reject keys containing a semicolon (https://go.dev/issue/25192). If a Go 1.17 ReverseProxy forwards a request to a backend which treats semicolons as a parameter separator (as Go 1.16 and earlier did), the proxy and backend may disagree on the parameter values of the request.

Thanks to Oxeye for pointing out this issue: https://www.oxeye.io/blog/golang-parameter-smuggling-attack

@seankhliao seankhliao added Security NeedsFix The path to resolution is known, but the work has not been done. labels Aug 24, 2022
@gopherbot
Copy link

gopherbot commented Aug 25, 2022

Change https://go.dev/cl/425417 mentions this issue: net/http/httputil: ReverseProxy should not forward unparseable query parameters

@seankhliao seankhliao added this to the Go1.20 milestone Aug 27, 2022
@chrisguiney
Copy link
Contributor

chrisguiney commented Aug 29, 2022

Is this going to be present in a 1.19.x release, or strictly a 1.20 change?

Unfortunately it's behavior that I'm currently depending on, so I'll need to find a way to maintain the current behavior. For context, I'm reverse proxying to a server that doesn't use ; as a separator, but does allow them as part of a key or value.

I've accepted that there's apparently no room for discussion on how to handle semicolons. I do hope that there can at least be a way to restore prior behavior. Working around this is going to be difficult given the changeset in https://go.dev/cl/425417

@neild
Copy link
Contributor Author

neild commented Aug 29, 2022

Is this going to be present in a 1.19.x release, or strictly a 1.20 change?

We don't even know what the fix for this is yet.

The basic requirement as I see it is that if a proxy Director or Rewrite func parses the query parameters, we must not forward unparsed parameters by default. Given that, what change can we make that has the least impact and preserves the most flexibility?

@chrisguiney
Copy link
Contributor

chrisguiney commented Aug 30, 2022

Ah sorry, I hadn't looked closely enough at the PR to see it wasn't merged yet.

My ideal solution at this point would be to introduce url.QueryParser and url.QueryEncoder interfaces, with default implementations calling url.ParseQuery and Values.Encode respectively (and exposed through package variables url.DefaultQueryParser and url.DefaultQueryEncoder).

From there, updating ReverseProxy with a url.QueryParser field that defaults to url.DefaultQueryParser. The same could be done for the net/http package, and anywhere else urls are parsed in the standard library.

This approach should maintain backwards compatibility, while allowing people with particular needs the ability to supply their own parser/encoder. In the long run, I think this is the way to go -- urls are tricky things that are both ubiquitous and varying interpretations of the RFCs exist throughout the internet.

I also realize that's touching more components than just httputil.ReverseProxy -- and would be a much bigger lift. I'd personally be happy with even just a boolean flag, or context variable to restore previous behavior, or maybe a QueryParser func(s) (Values, error) field


quick sketch of the interface described above:

var (
    DefaultQueryParser QueryParser = defaultQueryParser{}
    DefaultQueryEncoder QueryEncoder = defaultQueryParser{}
)

type QueryParser {
    Parse(string) (Values, error)
}

type QueryEncoder {
    Encode(Values) (string, error)
}

type defaultQueryParser struct{}

func (defaultQueryParser) Parse(s string) (Values, error) {
    return ParseQuery(s)
}

func (defaultQueryParser) Encode(v Values) (string, error) {
    return v.Encode(), nil
}

@neild neild self-assigned this Sep 12, 2022
@neild
Copy link
Contributor Author

neild commented Sep 13, 2022

Behavior which I think strikes the right balance between security and compatibility:

Clean the outgoing request's req.URL.RawQuery to remove unparseable parameters

  • If req.Form != nil after calling ReverseProxy.Director.
  • Before calling ReverseProxy.Rewrite.

When using the Director hook, RawQuery is cleaned only if the hook parsed the query parameters. A Director func which doesn't examine query parameters will pass garbage through untouched, but that's okay because it (probably) wasn't making decisions based on it.

When using the Rewrite hook (new in 1.20, intended to be the recommended replacement for Director for all uses), RawQuery gets cleaned by default. (We can't easily conditionalize this on whether the hook parses the form or not, because a design goal of Rewrite is that we give it the outgoing request we intend to send--we don't want to modify that request further after Rewrite returns.) To disable this, you can just restore the old value in Rewrite:

Rewriter: func(r *ProxyRequest) {
  r.Out.URL = r.In.URL // use uncleaned query parameters
}

@gopherbot
Copy link

gopherbot commented Sep 22, 2022

Change https://go.dev/cl/432976 mentions this issue: net/http/httputil: avoid query parameter smuggling

@neild
Copy link
Contributor Author

neild commented Sep 23, 2022

This is CVE-2022-2880.

@neild
Copy link
Contributor Author

neild commented Sep 23, 2022

@gopherbot please open backport issues

@gopherbot
Copy link

gopherbot commented Sep 23, 2022

Backport issue(s) opened: #55842 (for 1.18), #55843 (for 1.19).

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

gopherbot commented Sep 23, 2022

Change https://go.dev/cl/433695 mentions this issue: [release-branch.go1.18] net/http/httputil: avoid query parameter smuggling

@gopherbot
Copy link

gopherbot commented Sep 23, 2022

Change https://go.dev/cl/433735 mentions this issue: [release-branch.go1.19] net/http/httputil: avoid query parameter smuggling

gopherbot pushed a commit that referenced this issue Sep 28, 2022
…gling

Query parameter smuggling occurs when a proxy's interpretation
of query parameters differs from that of a downstream server.
Change ReverseProxy to avoid forwarding ignored query parameters.

Remove unparsable query parameters from the outbound request

   * if req.Form != nil after calling ReverseProxy.Director; and
   * before calling ReverseProxy.Rewrite.

This change preserves the existing behavior of forwarding the
raw query untouched if a Director hook does not parse the query
by calling Request.ParseForm (possibly indirectly).

Fixes #55843
For #54663
For CVE-2022-2880

Change-Id: If1621f6b0e73a49d79059dae9e6b256e0ff18ca9
Reviewed-on: https://go-review.googlesource.com/c/go/+/432976
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/433735
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue Sep 28, 2022
…gling

Query parameter smuggling occurs when a proxy's interpretation
of query parameters differs from that of a downstream server.
Change ReverseProxy to avoid forwarding ignored query parameters.

Remove unparsable query parameters from the outbound request

   * if req.Form != nil after calling ReverseProxy.Director; and
   * before calling ReverseProxy.Rewrite.

This change preserves the existing behavior of forwarding the
raw query untouched if a Director hook does not parse the query
by calling Request.ParseForm (possibly indirectly).

Fixes #55842
For #54663
For CVE-2022-2880

Change-Id: If1621f6b0e73a49d79059dae9e6b256e0ff18ca9
Reviewed-on: https://go-review.googlesource.com/c/go/+/432976
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
(cherry picked from commit 7c84234)
Reviewed-on: https://go-review.googlesource.com/c/go/+/433695
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Projects
None yet
Development

No branches or pull requests

4 participants