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/url: don't parse ';' as a separator in query string [freeze exception] #25192

Closed
zhyale opened this issue May 1, 2018 · 45 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted release-blocker Security
Milestone

Comments

@zhyale
Copy link

zhyale commented May 1, 2018

Please answer these questions before submitting your issue. Thanks!

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

go1.10.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

CentOS 7 with kernel 3.10.0-514.10.2.el7.x86_64

What did you do?

I create a program to detect attack such as SQL Injection, when test case is:
http://..../testcase?id=1%27;--

and I use:
r.ParseForm()
params := r.Form
fmt.Println("params:", params, "count:", len(params))
for key, values := range params {
fmt.Println("param", key, ":", values)
}

Got:
params: map[--:[] id:[1']] count: 2
param id : [1']
param -- : []

What did you expect to see?

expect only one expression in this case:
key: id
value: 1';--

What did you see instead?

I got two key:[value] pairs.

@agnivade agnivade changed the title r.Form parse error when parameter include SQL Injection test net/http: ParseForm incorrectly parses parameters during an SQL Injection test May 1, 2018
@agnivade
Copy link
Contributor

agnivade commented May 1, 2018

Complete repro -

package main

import (
	"fmt"
	"net/http"
)

func main() {
	http.HandleFunc("/foo", func(w http.ResponseWriter, req *http.Request) {
		err := req.ParseForm()
		if err != nil {
			fmt.Println(err)
			w.Write([]byte("error"))
			return
		}
		params := req.Form
		fmt.Println("params:", params, "count:", len(params))
		for key, values := range params {
			fmt.Println("param", key, ":", values)
		}
		w.Write([]byte("OK"))
	})

	fmt.Println("starting on port 9999")
	server := &http.Server{
		Addr: ":9999",
	}
	server.ListenAndServe()
}

curl 'http://localhost:9999/foo?id=1%27;--'

@bradfitz - Is this expected ?

@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 1, 2018
@agnivade agnivade added this to the Go1.11 milestone May 1, 2018
@fraenkel
Copy link
Contributor

fraenkel commented May 1, 2018

Both & and ; are used to split key value pairs. The ; is optional since it allows you to provide a URL as a value with a query string.
See https://en.wikipedia.org/wiki/Query_string under Web Forms

@odeke-em
Copy link
Member

odeke-em commented May 1, 2018

Hello @zhyale, thank you for filing this issue and welcome to the Go project!

@agnivade and @fraenkel thank you for the responses too.

So I believe, the root cause of the question here is rather: Why is a semi colon being used as a separator in url.Parse?

If you run https://play.golang.org/p/QVz18jWspPF or inlined below:

package main

import (
	"log"
	"net/url"
)

func main() {
	log.SetFlags(0)
	u, err := url.Parse("http://localhost:9999/foo?id=1%27;--")
	if err != nil {
		log.Fatalf("Failed to parse URL: %v", err)
	}
	log.Printf("%#v\n", u.Query())
}

You'll see the real symptom

url.Values{"--":[]string{""}, "id":[]string{"1'"}}

A W3C recommendation https://www.w3.org/TR/html401/appendix/notes.html#h-B.2.2 recommended using ; as a separator for url-encoded params for form-data
screen shot 2018-05-01 at 5 08 58 am

and that was what we used to add ; as a separator, making ';' synoymous with '&' in https://golang.org/cl/4973062 #2210 (comment)

However, unfortunately that recommendation just got superseded very recently in mid-December 2017 by https://www.w3.org/TR/2017/REC-html52-20171214/

and now that points to https://url.spec.whatwg.org/#urlsearchparams
in which we can see that the only separator here is '&'
screen shot 2018-05-01 at 5 19 53 am

Node.js doesn't seem to recognize ';' as a separator

url.parse('http://localhost:9999/foo?id=1%27;--', true);
Url {
  protocol: 'http:',
  slashes: true,
  auth: null,
  host: 'localhost:9999',
  port: '9999',
  hostname: 'localhost',
  hash: null,
  search: '?id=1%27;--',
  query: { id: '1\';--' },
  pathname: '/foo',
  path: '/foo?id=1%27;--',
  href: 'http://localhost:9999/foo?id=1%27;--' }

and the old survey of what other languages do while technically still correct per #2210 (comment), any future adoptions of the new recommendation from W3C will mean that those languages will also change their behavior.

Now the big question is: this behavior has been around since 2011, changing it 7 years later might massively break code for many users. Perhaps we need a survey of the make-up and interpretation of query strings from some frontend server?

Also this perhaps will need some security considerations as well or maybe a wide scale adoption of the recommendation first?

In addition to those already paged, I will page some more folks to help pitch in thoughts about the fate of the new W3C recommendation /cc @ianlancetaylor @andybons @tombergan @agl @rsc @mikesamuel

@odeke-em odeke-em changed the title net/http: ParseForm incorrectly parses parameters during an SQL Injection test net/url: ';' is parsed as a separator in query string May 1, 2018
@bradfitz bradfitz added v2 A language change or incompatible library change NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels May 1, 2018
@bradfitz bradfitz modified the milestones: Go1.11, Go2 May 1, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 1, 2018
@bradfitz
Copy link
Contributor

bradfitz commented May 1, 2018

The Go 1 docs say:

Query is expected to be a list of key=value settings separated by ampersands or semicolons.

So we can't change it during Go 1.x.

Repurposing this to be a Go 2.x issue.

@mikesamuel
Copy link
Contributor

IIUC, the problem is that

Array.from(new URL(`https://a/?a&b;c`).searchParams.keys())
// [ "a", "b;c" ]

but

values, _ := url.ParseQuery(`a&b;c`)
fmt.Println(reflect.ValueOf(values).MapKeys())  // [c a b]

Assuming that's right:

Now the big question is: this behavior has been around since 2011, changing it 7 years later might massively break code for many users. Perhaps we need a survey of the make-up and interpretation of query strings from some frontend server?

+1

We might be able to get a handle on the size of potential breakage by surveying URL composition libraries to get an idea of how many sources are likely to produce URLs that are substantially different given non-crafted inputs.

For example:

// html/template
t, _ := template.New("T").Parse(`<a href="/?a={{.}}&b=b">`)
t.Execute(&out, `foo;bar&baz`)
out.String() == `<a href="/?a=foo%3bbar%26baz&b=b">`

// net/url
url.QueryEscape(`foo;bar&baz`) == `foo%3Bbar%26baz`
url.QueryUnescape(`foo%3Bbar%26baz`) == `foo;bar&baz`

In vanilla JS

encodeURIComponent(`foo;bar&baz`) == `foo%3Bbar%26baz`
encodeURI(`foo;bar&baz`) == `foo;bar&baz`

I think this is mostly a correctness and interop issue but that the security consequences are not severe.

That said, there are risks to inferring more structure than the spec allows.

If a survey finds that there are widely used url.QueryEscape / encodeURIComponent counterparts that escape & but do not escape ;, and a trusted partner composes URLs thus:

goServiceUrl = "//example.com/?"
    + "securityIrrelevantParam=" + laxEscapeQuery(untrustedString)
    + "&importantParam=" + laxEscapeQuery(trustedString)

then untrustedString might inject a ;importantParam which would be available as the zero-th entry in Go's query map.

The risk is pretty small as long as laxEscapeQuery escapes '=' and one workaround is to consistently enforce has-exactly-one constraints.

@bigluck
Copy link

bigluck commented Oct 23, 2018

Hi everybody,
I'm having the same problem by using AWS API Gateway in front of my application.
When the proxed endpoint is configured with the HTTP Proxy integration configuration on, AWS API Gateway transform a valid request like this one:

GET /service?cid=002125b3-26bd-48c4-ac90-9683d804be2d&tm=1532974971748&ua=Mozilla%2F5.0%20(Macintosh%3B%20Intel%20Mac%20OS%20X%2010_13_6)%20AppleWebKit%2F537.36%20(KHTML%2C%20like%20Gecko)%20Chrome%2F67.0.3396.99%20Safari%2F537.36&z=b075dd02-77f8-44bb-a389-866dcc80c9b6 HTTP/1.1

into:

GET /service?cid=002125b3-26bd-48c4-ac90-9683d804be2d&tm=1532974971748&ua=Mozilla/5.0+(Macintosh;+Intel+Mac+OS+X+10_13_6)+AppleWebKit/537.36+(KHTML,+like+Gecko)+Chrome/67.0.3396.99+Safari/537.36&z=b075dd02-77f8-44bb-a389-866dcc80c9b6 HTTP/1.1

And Golang parse the ua parameter into Mozilla/5.0 (Macintosh

Here a Go Playground to test it: https://play.golang.org/p/BeKy_UuSyoO

@FiloSottile FiloSottile added Security and removed v2 A language change or incompatible library change NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Mar 3, 2021
@FiloSottile FiloSottile modified the milestones: Go2, Go1.17 Mar 3, 2021
@FiloSottile FiloSottile changed the title net/url: ';' is parsed as a separator in query string proposal: net/url: don't parse ';' as a separator in query string Mar 3, 2021
@FiloSottile
Copy link
Contributor

FiloSottile commented Mar 3, 2021

It was recently pointed out that this behavior divergence can lead to cache poisoning attacks in reverse proxies that cache based on a subset of query parameters: if Go treats https://example.com/?x;id=1337&id=123 as a request for ID 1337, while the cache treats it as a request for ID 123, the cache can be made to serve the wrong response.

At the risk of repeating myself, I want to point out that relying on parser alignment for security is doomed, so this will break again, and often in subtler ways that we can't fix universally. However, this is probably broken most of the time now, so we should fix it.

I did a quick survey of other popular languages and frameworks.

A more interesting question would be how caching proxies behave, but a lot of them are ad-hoc and hard to test. Snyk seems to think at least Varnish uses only &. As pointed out in #25192 (comment), AWS is not only ignoring ;, but it's unescaping it while proxying.

I think the W3C and WHATWG recommendations are sort of red herrings: they are about x-www-form-urlencoded, not URL queries. On the other hand, the entire reason anyone supported ; is that it was more convenient to use for x-www-form-urlencoded forms, as it did not require escaping & in HTML.

Given Rails and Python are waiting for the next scheduled release to fix this, we will too, and ship the fix in Go 1.17. It would be too harsh of a behavior change for too vague of a security benefit to land in a security release anyway.

This is technically a backwards compatibility promise violation, but I am invoking the security exception. More broadly, we should think whether the thing we want to provide compatibility with is "the exact self-contained behavior of the library when it was introduced" or "correctly interoperate with the current ecosystem". Realistically, we're already doing more of the latter with crypto/tls and crypto/x509, because doing the exact same thing as 5 years ago would be useless and insecure. HTTP and web specifications are similarly living and evolving, and we should figure out what our policy about that is.

@rsc
Copy link
Contributor

rsc commented Mar 3, 2021

It seems wrong that every single HTTP server implementation in the world is being charged with coping with a decision made by a few proxies. If the proxy doesn't think ; is special, then it could easily enforce that view by escaping the ; as it passes the request through. And if it did think ; was special, it could enforce that by rewriting to &. Then just a few proxies would need changing instead of all the HTTP server implementations in the world.

The fact that we're having this discussion and not the nginx developers is utterly backward.
(And AWS unescaping the ; is even more backward.)

Anyway, that rant aside, if we do make this change, can we find some way to make it not a silent change? I worry about people updating to Go 1.17 and having their servers misbehave but with no idea why. Maybe a logging print for each request that ignores ; as separator, limited to one per second or something like that?

It also seems like a hard requirement to be able to opt back in to the old behavior. Users with client apps sending ; that don't have the misfortune of being behind nginx will not like being forced to rewrite and redeploy their apps just to get Go 1.17.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Mar 10, 2021
@rsc
Copy link
Contributor

rsc commented Mar 10, 2021

Adding to the minutes. It's getting a bit late to make a change in Go 1.17 so we should try to converge on a plan soon.

@bigluck, if you still use the AWS API Gateway, does it still unescape semicolons? Or does anyone else know?

@rsc
Copy link
Contributor

rsc commented Mar 10, 2021

@ianlancetaylor found private mail saying that https://angular.io/api/common/http/HttpUrlEncodingCodec did not escape semicolons at least in 2019, leading to confusion as well. Does anyone know if that is still the case?

And does anyone know of any clients that do use semicolons for separators and would break if we stopped recognizing them?

@rsc rsc moved this from Incoming to Active in Proposals (old) Mar 10, 2021
@rsc
Copy link
Contributor

rsc commented Mar 10, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

zmb3 added a commit to gravitational/teleport that referenced this issue Sep 23, 2021
Go 1.17 (by default) disallows using ; as a separator. Our tests fail
on Go 1.17 with the following error. Since we only do this in test
code, it's easiest just to switch to & so we don't have a breakage
when we start compiling with 1.17.

See also: golang/go#25192

Error:

    2021/09/03 10:21:57 http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192

    ----------------------------------------------------------------------
    FAIL: apiserver_test.go:473: WebSuite.TestSAMLSuccess

    apiserver_test.go:524:
        c.Assert(u.Scheme+"://"+u.Host+u.Path, Equals, fixtures.SAMLOktaSSO)
    ... obtained string = ":///web/msg/error/login"
    ... expected string = "https://dev-813354.oktapreview.com/app/gravitationaldev813354_teleportsaml_1/exkafftca6RqPVgyZ0h7/sso/saml"
@yktoo
Copy link

yktoo commented Dec 13, 2021

With the change https://golang.org/cl/325697 in place our servers now produce lots of log lines like

2021/12/13 08:44:51 http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192
2021/12/13 08:44:51 http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192
2021/12/13 08:57:37 http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192
2021/12/13 09:11:37 http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192
2021/12/13 09:19:30 http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192

This is really not helpful, and since it's a web server, anyone can flood you with requests with a ;. Can this be switched off somehow?

@antichris
Copy link

antichris commented Dec 13, 2021

@yktoo Have you looked into using AllowQuerySemicolons or writing your own middleware to replace all semicolons with URL escapes? Alternatively, you could join the convo at #49399 and/or #50034 or give either a thumbs-up if it is relevant to your case.

@seankhliao
Copy link
Member

http.Server takes a *log.Logger, you can filter it out there if you want.

@yktoo
Copy link

yktoo commented Dec 13, 2021

Thanks @antichris, #49399/#50034 describe my concerns exactly. @seankhliao yes log filtering seems the only workaround at the moment, albeit very ugly.

@shinny-chengzhi
Copy link

I have been caught this by surprise and want to restore the old behavior. But it's unclear to me how to do this while supporting both 1.17&pre-1.17 since calling AllowQuerySemicolons will result in build error in 1.16.

@ianlancetaylor
Copy link
Contributor

@shinny-chengzhi You can use a build tag to compile code differently for go1.16 and go1.17.

https://pkg.go.dev/cmd/go#hdr-Build_constraints

@shinny-chengzhi
Copy link

@ianlancetaylor Thanks, that will do it.

fishy added a commit to fishy/baseplate.go that referenced this issue Oct 27, 2022
It's used by upstream server to log some of the kinda-breaking issues,
for example [1]. Since the majority of the http servers at Reddit are
public facing, when those happens it's usually just user messing with
us, not really that the http client we control doing things wrong. This
gives us a way to suppress those logs, and also emit counters to better
track how many times those happened.

This also makes the upstream http server to use the same json logger by
zap as the rest of our code.

[1]: golang/go#25192 (comment)
fishy added a commit to fishy/baseplate.go that referenced this issue Oct 27, 2022
It's used by upstream server to log some of the kinda-breaking issues,
for example [1]. Since the majority of the http servers at Reddit are
public facing, when those happens it's usually just user messing with
us, not really that the http client we control doing things wrong. This
gives us a way to suppress those logs, and also emit counters to better
track how many times those happened.

This also makes the upstream http server to use the same json logger by
zap as the rest of our code, at warning level.

[1]: golang/go#25192 (comment)
fishy added a commit to fishy/baseplate.go that referenced this issue Oct 27, 2022
It's used by upstream server to log some of the kinda-breaking issues,
for example [1]. Since the majority of the http servers at Reddit are
public facing, when those happens it's usually just user messing with
us, not really that the http client we control doing things wrong. This
gives us a way to suppress those logs, and also emit counters to better
track how many times those happened.

This also makes the upstream http server to use the same json logger by
zap as the rest of our code, at warning level.

[1]: golang/go#25192 (comment)
fishy added a commit to reddit/baseplate.go that referenced this issue Oct 27, 2022
It's used by upstream server to log some of the kinda-breaking issues,
for example [1]. Since the majority of the http servers at Reddit are
public facing, when those happens it's usually just user messing with
us, not really that the http client we control doing things wrong. This
gives us a way to suppress those logs, and also emit counters to better
track how many times those happened.

This also makes the upstream http server to use the same json logger by
zap as the rest of our code, at warning level.

[1]: golang/go#25192 (comment)
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted release-blocker Security
Projects
No open projects
Development

No branches or pull requests