opened
zhyale
username_0
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://..../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.
r.Form parse error when parameter include SQL Injection test
issue
created
agnivade
username_1
Complete repro -

```go
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;--'`

@username_4 - Is this expected ?
comment
created
fraenkel
username_2
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
comment
created
odeke-em
username_3
Hello @username_0, thank you for filing this issue and welcome to the Go project!

@username_1 and @username_2 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:
```go
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
```shell
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
<img width="1276" alt="screen shot 2018-05-01 at 5 08 58 am" src="https://user-images.githubusercontent.com/4898263/39472383-d41e7482-4cfd-11e8-9326-d8b2a668e17a.png">

and that was what we used to add `;` as a separator, making ';' synoymous with '&' in https://golang.org/cl/4973062 https://github.com/golang/go/issues/2210#issue-51278892 

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 '&'
<img width="1167" alt="screen shot 2018-05-01 at 5 19 53 am" src="https://user-images.githubusercontent.com/4898263/39472615-51c970a2-4cff-11e8-80ec-4b1428326774.png">



Node.js doesn't seem to recognize ';' as a separator
```javascript
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 https://github.com/golang/go/issues/2210#issuecomment-66058500, 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 @username_19 @andybons @tombergan @agl  @username_7 @username_5
comment
created
bradfitz
username_4
So we can't change it during Go 1.x.

Repurposing this to be a Go 2.x issue.
comment
created
mikesamuel
username_5
+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:

```go
// 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
```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.
comment
created
FiloSottile
username_6
It was recently pointed out that this behavior divergence can [lead to cache poisoning attacks](https://snyk.io/blog/cache-poisoning-in-popular-open-source-packages/) 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.

* PHP used to support both but now defaults to only `&`. https://www.php.net/manual/en/ini.core.php#ini.sect.data-handling
* Rails (via Rack) currently supports both, but landed a change in January following the Snyk blog post. Unclear when it will reach a release. https://github.com/rack/rack/issues/1732
* Python currently supports both, but will switch in 3.10 and assigned it a CVE. https://github.com/python/cpython/pull/24297

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 https://github.com/golang/go/issues/25192#issuecomment-432369818, 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 are 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.
comment
created
rsc
username_7
It seems kind of backward that every single HTTP server implementation in the world is being charged with coping with a decision made by a few proxies. If they don't think ; is special, then they could easily force it not to be special by escaping the ; as they pass it through. (AWS doing the opposite of that is even more mind-boggling.)

Can we find some way to make this change less silent? 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.
comment
created
rsc
username_7
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?
comment
created
rsc
username_7
@username_19 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?
comment
created
rsc
username_7
This is good advice. It boils down to "Make sure your server and your cache proxy agree about how to split query arguments." The simplest, most guaranteed way to do this is to have the proxy rewrite the query to be unambiguous.

### 4. Go servers behind nginx are _not_ affected by default.

The nginx docs say the [default proxy cache key](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_cache_key) is `$scheme$proxy_host$request_uri`. That does _not_ interpret the query parameters at all, so it has no problem with semicolons versus ampersands. 

The way to get into trouble is to use a key like `$arg_x` which extracts the `x` parameter from the query string and does so without considering semicolons (the code is ngx\_http\_arg called from ngx\_http\_variable\_argument in the nginx code base; there is no configuration to allow semicolons). Go users, and Python users, and Ruby users, and many others, _cannot_ sync the nginx behavior and their server's behavior while also using query parameter cache keys. But again, these keys are not used by default. Only users who use these keys need to make a change.

### 5. A potential non-breaking Go change.

To not break users, we could add a url.AllowQuerySemicolon function that controls whether ParseQuery accepts semicolons. It can default to true, but users who want to use nginx's query argument cache keys would call AllowQuerySemicolon(false) to bring the Go implementation in line with nginx's.

It is not clear to me that we have enough justification to take the next step of changing the default and breaking users, all for the very few users who are using nginx _and_ setting a custom cache key using query argument parameters.
Again, **this does not affect all Go programs sitting behind nginx**. It only affects those who set a custom proxy_cache_key and use a `$arg_xxx` form.

In the long term, it still seems like nginx should rewrite the query strings to escape semicolons in requests that are consulting the proxy cache and using query parameter argument keys, ensuring that nginx's understanding of the request URL matches the server's, 100% of the time, no matter what the server is.
comment
created
FiloSottile
username_6
I think focusing on nginx is optimistic. nginx happens to be a popular reverse proxy that exhibits this behavior, but I doubt it's the only one. We should check Cloudflare, Fastly, Akamai, AWS, GCP, Azure, Varnish, F5, and Apache, off the top of my head. If they all need to change, then changing the server frameworks is not "changing the thing in a lot of places instead of in one place" (which I generally agree with as an argument).

At that point, I think it becomes a matter of what's more expected and predictable. The HTML 4 spec is hardly authoritative for URL query strings in HTTP, so we just don't have an easy reference here. As of today, I found the `;` surprising. If I were to write or audit a regex-based cache key, I would not have flagged the lack of semicolon support, nor would I have requested that the regex be wired into the rewriting engine (which is usually not possible). Now that Python, PHP, and Rails are switching to the better-known behavior, the situation will get worse over time, not better. In five years, we'll be the odd one out, and it will be hard to convince all new reverse proxies to do query string mangling to support Go's bizarre URL parsing.

If you are not convinced, I think we should check with all those other reverse proxy vendors before declining this proposal.
comment
created
rsc
username_7
It sounds like the argument is that "Go should break all the semicolon users because all the others are too". 
And maybe that's true. What we don't really know is how many semicolon users there are.

Again, this does not happen in reverse proxies out of the box, only when you start trying to distinguish by query 
parameters.
comment
created
rsc
username_7
Twitter threads gathering info:
https://twitter.com/username_4/status/1374774763059482624
https://twitter.com/username_6/status/1374777816139628545
comment
created
joneskoo
username_8
Many on the Twitter threads confusing this with semicolons in path section. That’s not what we’re talking about here, right?

not in scope:
`https://example.com/a;b=c;foo=42`

In scope:
`https://example.com/a?b=c;foo=42`

notably JSESSIONID mentioned a few times.

https://play.golang.org/p/OUVtI15HkU3
comment
created
AGWA
username_9
I'm a semicolon user.  I like the ergonomic benefits from not having to escape `&` in HTML.

The backwards compatibility break for this would be really nasty.  Unlike removing TLS ciphers or support for common name, this won't result in a predictable error.  Instead the program will behave differently, and a log message is cold comfort if the program has already taken some undesirable action based on misinterpreting a query string.  The bar for such a compatibility break should be very high, and I don't think this proposal should qualify. Parsing URLs is a minefield and [practically every implementation does something differently](https://www.blackhat.com/docs/us-17/thursday/us-17-Tsai-A-New-Era-Of-SSRF-Exploiting-URL-Parser-In-Trending-Programming-Languages.pdf).  Any proxy server which relies on parsing query strings in the same manner as their backends is insecure, and removing semicolon separators from Go doesn't change that.

To provide an example of how futile this effort is, nginx's query string parser ([`ngx_http_arg`](https://github.com/nginx/nginx/blob/2e94c81b0bf1d8e695d9afd074c79520aa03081a/src/http/ngx_http_parse.c#L2120)) differs from Go's in the following three additional ways:

|nginx behavior|Example|Go value for `id`|nginx value for `id`|
| --- | --- | --- | --- |
|Keys are case-insensitive|`iD=1&id=2`|`2`|`1`|
|Keys are not unescaped|`%69d=1&id=2`|`1`|`2`|
|Keys without equal signs are not supported|`id&id=1`|(empty)|`1`|

Consequentially, this proposal would not help nginx users at all, and I would not be surprised if other reverse proxies also deviated from Go in at least one of the above ways.

I realize that there aren't many semicolon users (@username_6's Twitter poll has been bouncing between 1.5%-2%), but it strikes me as pretty inequitable to force us to deal with a change in application behavior whose only purpose is to try to paper over other people's risky behavior.
comment
created
rsc
username_7
@username_8, you are correct: semicolons before a ? are unrelated to this issue.
comment
created
rsc
username_7
@username_9, indeed. A major problem with this change is that it will be a reason users can't easily update to a newer version of Go, which will increase our support burden as users lag behind. By itself, it may not seem like much to break a small fraction of the user base, but over time that kind of thing adds up. 

Thanks very much for the additional nginx examples. I think these support the idea that any kind of proxy caching that tries to strip "unnecessary" parts of the URL really needs to canonicalize the URL to enforce its interpretation before passing the query along. That is, there may be a real problem here, but the fix is being applied in the wrong place. Even if we "fixed" Go to match nginx in all these cases, it would still not match other caching proxies.

Again, this does not happen in reverse proxies out of the box, only when you start trying to filter "unimportant" parts of the query string. It's starting to sound like you should basically never do that, and reverse proxies should remove that feature entirely.
comment
created
FiloSottile
username_6
This is not practical, alas. Not for any reason I like, to be clear: it's mostly tracking parameters. But they are very much a thing and "don't get to your business goals because parser alignment is impossible (and anyway we don't like your practices)" is not a winning argument.

Taking a step back, it sounds like there are two ways out of this as an ecosystem:

1. we convince all reverse proxies to change the semantics of cache keys so that they rewrite the upstream URL to a canonicalized representation, on the argument that it's safer and more robust, and that they should mitigate our exclusive off-spec behavior
2. we converge on the specified behavior, filing issues with reverse proxies that depart from the spec on the argument that they are off-spec and introducing a vulnerability

I am not a fan of (2) at all, but (1) doesn't sound like it will happen, and this format is unusually simple and well-specified, so it might just be possible to match the spec.
comment
created
rsc
username_7
I still can't escape the feeling this is all security theater, especially given what @username_9 pointed out.
The suggestion is to break existing Go users (we don't know how many, but greater than zero).
And the benefit is ... unclear beyond speculation about possibly being one step in a much longer path involving many players toward converging on shared behavior.

I just don't yet see how the benefits outweigh the costs.

Also the linked WHATWG spec is about JavaScript behavior specifically.
(The rest of the spec is not, but the parsing of the query is only relevant to constructing a JavaScript object.)

I'm sorry for being a pain about this, but breaking users is a big deal and we need to be sure it is worth it.
And that evidence is absent so far. Starting from the very beginning with Snyk's sloppy blog post, there have been too many fuzzy claims that look different when you get up close and really examine them.
comment
created
slrz
username_10
If other implementations go ahead with this change it will probably cause existing users to move away from their semicolon usage. Let enough time pass and it might be rare enough for Go to follow without annoying too many users.

Now, leaning back and watching while others take flak for breaking users may not be the nicest thing to do **if** the change in question is absolutely necessary for the ecosystem's continued health. In this case though, I think it's appropriate given that Python, Rails and friends don't have a strong commitment to compatibility in the first place (unlike Go) and the proposed change doesn't seem all that urgent to me.
comment
created
rsc
username_7
It will probably annoy the same number as it would today.
comment
created
beoran
username_11
The simplest backwards compatible option is to add a  url.AllowQuerySemicolon(bool) function, as username_7 suggested. Not to +1, but what is there /against/ that suggestion? Whether it should default to true or false is debatable, but either way, letting the Go users  have a knob to keep backwards compatibility seems desirable.
comment
created
Mouvedia
username_12
1. option default to true
2. option default to false
3. deprecation notice
4. removal
comment
created
rsc
username_7
Filippo and I spent an hour discussing this. The problem today is a “superset” attack, where Go sees a superset of the values that the proxy does for:

	url?x=y;id=evil&id=good

In this case, the proxy thinks the id is good but Go thinks the id is evil, resulting in the evil entry being cached as good.

Go's current parser also leads to a “subset” situation, where Go sees a subset of the values that the proxy does. In particular, Go rejects (drops) keys and values % followed by non-two-hex-digits, but the most common code paths don't check the error, so in

	url?id=evil%&id=good

Go sees good while other implementations may see both evil% and good. We are unaware of any attack on this “subset” situation.

Because of that, we propose to do the following.

1. Make url.ParseQuery split on & but then reject key=value pairs containing semicolons.
2. Make package http log about ParseQuery errors mentioning the semicolon problem.

The effect will be to make the semicolon-preceded or -terminated pairs disappear, turning the superset attack into a subset situation. Because we are unaware of any subset-based attack, this is an improvement.

The log message should make clear what is going on for users who are confused about no longer seeing certain key-value pairs in their apps.
comment
created
rsc
username_7
Since there seem to be no objections to the proposal in the previous comment, I am going to mark this likely accept. The final acceptance won't happen until after the freeze, but we still would like to get this in for Go 1.17 for security reasons.
comment
created
rsc
username_7
Based on the discussion above, this proposal seems like a **[likely accept](https://golang.org/s/proposal-status#likely-accept)**.
— username_7 for the proposal review group
comment
created
antichris
username_13
`;` is the more reasonable choice and (unrealistic as it sounds, especially when everyone's been using `&amp;`s out of pure force of habit for decades now and some are even actually moving to drop `;` support entirely) should actually be the preferred separator. So, if the idea is to drop the support for splitting on semicolons *unconditionally*, I'm against it.

It's not like Go can flip the webdev world now, dominated by javascript kiddies and wordpress "engineers", but we can at least keep the saner choice an opt-in for those who can appreciate (and properly set their infrastructure up around) it. In other words, what https://github.com/golang/go/issues/25192#issuecomment-820166005 said, just really do default to `false`.
comment
created
FiloSottile
username_6
@username_13, this remark is not consistent with the [Gopher values](https://golang.org/conduct). The Javascript world built many things, and enabled even more people to build things. We want Go to be a tool that works with that ecosystem, and to be welcoming of people that come from that community.

The only technical argument in favor of using semicolons is that it's easier to insert them in manually composed HTML. If you have more please do contribute them. Ecosystem consistency is also a technical argument, and that's what we are weighting it against here.

Knobs are extra complexity, but if you have a proposal for where to insert that knob we'll listen and weight it against the benefit. `url.AllowQuerySemicolon(bool)` is not an option, because package-global state would lead to conflicts between libraries that expect and set the value to be different.
comment
created
andrius4669
username_14
somehow people are forgetting that it's often not necessary at all to escape `&` in case of url parameter usage, because the usual `&x=y` has very little chance to match `&<something>;` sequence for character references, unless parameter key contains `;` AND character reference is actually meaningful AND it's not escaped for some reason (url.QueryEscape does escape `;`).
comment
created
rsc
username_7
@username_9's handler converter is nice for opt-in use. I filed #45973 for that.
comment
created
rsc
username_7
No change in consensus, so **[accepted](https://golang.org/s/proposal-status#accepted)**. 🎉
This issue now tracks the work of implementing the proposal.
— username_7 for the proposal review group
comment
created
FiloSottile
username_6
I'd like to request a freeze exception for this fix. We deemed it not a backportable security fix (also because of the potential for disruption and the limited real-world impact), but it's conceivable that it could have security value for some applications, so we'd like not to wait for Go 1.18.

Russ, should we also land AllowQuerySemicolons (#45973) during the freeze? I'm leaning towards yes.

/cc @golang/release @username_7
comment
created
dmitshur
username_15
Thank you for making this freeze exception request. It is approved.
comment
created
yktoo
username_16
With the change https://golang.org/cl/325697 in place our servers now produce lots of log lines like

```log
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?
comment
created
antichris
username_13
@username_16 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 #50034 or give it a thumbs-up if it is relevant to your case.

[`AllowQuerySemicolons`]: https://pkg.go.dev/net/http#AllowQuerySemicolons
"AllowQuerySemicolons - http package - net/http - pkg.go.dev"
comment
created
seankhliao
username_17
`http.Server` takes a `*log.Logger`, you can filter it out there if you want.
comment
created
yktoo
username_16
Thanks @username_13, #49399/#50034 describe my concerns exactly. @username_17 yes log filtering seems the only workaround at the moment, albeit very ugly.
comment
created
shinny-chengzhi
username_18
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.
comment
created
ianlancetaylor
username_19
@username_18 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
comment
created
shinny-chengzhi
username_18
@username_19 Thanks, that will do it.
comment