[BUG] Semicolon unduly acts as separator for query parameters (thereby creating a parser differential)

[jub0bs]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

The (*Router).Queries method splits query-parameter pairs on both ampersands and semicolons.

Expected Behavior

Up to (excl.) commit 75dcda0, gorilla/mux to relied on net/url (Go 1.12, at that time of that commit) for parsing the query string. That commit introduced a query-string parser based on net/url but modified for performance.

net/url used to split query params on both ampersands and semicolons, but it now (since Go 1.17) only splits query-param pairs on ampersands, in compliance with the URL Living Standard.

I expect gorilla/mux to follow suit, and the sooner the better.

Steps To Reproduce

Run the following server locally:

package main

import (
	"fmt"
	"log"
	"net/http"

	"github.com/gorilla/mux"
)

func main() {
	r := mux.NewRouter()
	r.HandleFunc("/", handle).Queries("foo", "{foo}", "bar", "{bar}")
	http.Handle("/", r)
	if err := http.ListenAndServe(":8080", r); err != http.ErrServerClosed {
		log.Fatal(err)
	}
}

func handle(w http.ResponseWriter, r *http.Request) {
	vars := mux.Vars(r)
	fmt.Fprintln(w, vars["foo"], vars["bar"])
}

Then run the following command in your shell:

curl -v 'localhost:8080/?foo=foo;bar=bar'

Actual output:

HTTP/1.1 200 OK
-snip-
 
foo bar

Expected output:

HTTP/1.1 404 Not Found
-snip-

Anything else?

Although splitting query-param pairs on both ampersands and semicolons is harmless in isolation, it creates interoperability issues when gorilla/mux is used in conjunction with other tools that only split query-param pairs on ampersands (and not on semicolons). Such a parser differential can indeed open the door to security vulnerabilities, such as Web cache poisoning (via query-parameter cloacking) and broken access control.

Broken access control, in particular, is a real risk for programmes that rely on both net/url and mux.Vars for parsing the query string; you may believe that such programmes are rare, but take a look at the Minio project, which does rely on such a mix. For instance, consider the simple server below:

package main

import (
	"fmt"
	"log"
	"net/http"
	"strings"

	"github.com/gorilla/mux"
)

func main() {
	r := mux.NewRouter()
	r.HandleFunc("/delete", handleDelete).Methods(http.MethodDelete).Queries("id", "{id:[0-9]+}")
	http.Handle("/", r)
	if err := http.ListenAndServe(":8080", r); err != http.ErrServerClosed {
		log.Fatal(err)
	}
}

func handleDelete(w http.ResponseWriter, r *http.Request) {
	if !isAuthorized(w, r) {
		return
	}
	id := mux.Vars(r)["id"]
	fmt.Fprintf(w, "resource %s deleted\n", id)
}

func isAuthorized(w http.ResponseWriter, r *http.Request) bool {
	token, ok := strings.CutPrefix(r.Header.Get("Authorization"), "Bearer ")
	if !ok {
		w.WriteHeader(http.StatusUnauthorized)
		return false
	}
	id := r.URL.Query().Get("id")
	// simplified auth implementation (for the sake of this example)
	const authorizedId = "1" // ID of the only resource that users are allowed to interact with
	if id != authorizedId || id != token {
		w.WriteHeader(http.StatusForbidden)
		return false
	}
	return true
}

Here, the intention is to only allow access to the resource identified by 1. Let's try it:

$ curl -w "%{http_code}\n" -XDELETE -H 'Authorization: Bearer 1' 'localhost:8080/delete?id=1'
resource 1 deleted
200
$ curl -w "%{http_code}\n" -XDELETE -H 'Authorization: Bearer 2' 'localhost:8080/delete?id=2'
403

So far, so good. But an attacker could delete a resource he or she doesn't own (e.g. the resource of ID 42) like this:

$ curl -w "%{http_code}\n" -XDELETE -H 'Authorization: Bearer 1' 'localhost:8080/delete?a;id=42&id=1'
resource 42 deleted
200

The problem is that r.URL.Query().Get("id") == "1", whereas mux.Vars(r)["id"] == "42". 😬

---

Reasons why I'm not reporting this as a security vulnerability:

• As pointed out above, this behaviour is harmless as far as gorilla/mux is used strictly in isolation; it only becomes problematic if gorilla/mux is used with other tools.
• I filed a security advisory about gorilla/handlers on GitHub as far back as last December but never heard back from the maintainers. Filing an issue on GitHub and opening a PR may be my best shot at raising awareness of this specific problem with gorilla/mux and fixing it.

---

Regarding remediation, you have several possible approaches:

1. Do nothing, other than warning your users about the risk of such a parser differential.
2. Release a new minor version that introduces a router option (perhaps one named StrictQueryParamSep) for only using ampersands (as opposed to ampersands and semicolons) as query-param separators. Use false as the default value for the time being. Give gorilla/mux users some time to migrate their clients (to no longer rely on semicolons as a query-param separators). Then, in a subsequent minor version, switch the option's default value to true.
3. Same as 2, but use true as the option's default value straight away, without any transition period.
4. Release a new minor version that outright drops all support for semicolons as query-param separators.

I think option 4 is safest, but breaking existing clients that rely on this behaviour is a risk; similar remark about option 3. On the other hand, option 1 seems callous. The best compromise may be option 2; I have implemented the latter in my local clone of the project and I'm ready to fire a PR.

The only element that gives me pause is that gorilla/mux lacks a changelog. If we go through with this fix, how are users going to be notified that the new version comes with a behavioural change?

[jub0bs]

One more thing...

What I'm proposing here is to bring gorilla/mux in line with the URL Living Standard. However, it's worth pointing out that net/url (even modern versions, from v1.17.0 up to v1.24.x) still is not fully compliant with the URL Living Standard insofar as it ignores query-param pairs that contain semicolons; see golang/go#50034.

Therefore, even if #782 (or my more extreme option, numbered 4) got merged, a parser differential between gorilla/mux and net/url would persist, one which could allow adversaries to smuggle (past net/url and to gorilla/mux) a query-param pair whose value contains a semicolon. Consider, for instance, query string id=1;&id=42:

• net/url (v1.17+) would see id=42
• gorilla/mux (v1.9+) would see id=1;

This leads me to wonder whether gorilla/mux

• should not simply copy the behaviour of net/url (at least for the time being), in order to get rid of the parser differential,
• and should coordinate releases with net/url if/whenever net/url: semicolons treated as invalid characters in query string golang/go#50034 gets fixed.

[hmh]

I am not against this change in gorilla/mux, but IMO it really ought to be considered an API break, and require /v2.

The change in the golang side caused a lot of damage, we need to go over with a fine comb on everything that goes from go < 1.17 to go >= 1.17 because of that.

So consider this a vote AGAINST changing the default behavior in gorilla/mux v1, and a vote FOR changing the default behaviour in an eventual gorilla/mux v2 to match net/url and net/http.

Note that I have absolutely nothing against adding a non-default way for gorilla/mux v1 to stop accepting ";" as a URL query parameter separator: it won't break existing code.