net/url: don't parse ';' as a separator in query string [freeze exception]

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.

@bradfitz

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 ?

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

@zhyale

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

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 '&'

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

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.

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.

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

@antichris

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

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.

@shinny-chengzhi

@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

@ianlancetaylor

@ianlancetaylor Thanks, that will do it.

httpbp: Set http.Server.ErrorLog

net/url: reject query values with semicolons

Navigation Menu

Repository navigation

net/url: don't parse ';' as a separator in query string [freeze exception] #25192

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

61 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

net/url: don't parse ';' as a separator in query string [freeze exception] #25192

Description

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

Activity

agnivade commented on May 1, 2018

fraenkel commented on May 1, 2018

odeke-em commented on May 1, 2018

bradfitz commented on May 1, 2018

mikesamuel commented on May 1, 2018

bigluck commented on Oct 23, 2018

61 remaining items

yktoo commented on Dec 13, 2021

shinny-chengzhi commented on Mar 4, 2022

ianlancetaylor commented on Mar 4, 2022

shinny-chengzhi commented on Mar 7, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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

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