Path incorrectly decoded during clean

[tebruno99]

MIGRATED - Refer to original discussion & golang bugs when reviewing

From mux created by cjellick: gorilla#354

What version of Go are you running? (Paste the output of go version)
go version go1.10 linux/amd64

What version of gorilla/mux are you at? (Paste the output of git rev-parse HEAD inside $GOPATH/src/github.com/gorilla/mux)
07ba1fd

Describe your problem (and what you have tried so far)
The path cleaning performed in Router.ServerHTTP() works solely on request.URL.Path which decodes all characters. So characters, particularly slashes, that were escaped are unescaped in the redirect location sent back to the user as part of the 301 redirect.

Paste a minimal, runnable, reproduction of your issue below (use backticks to format it)

package main

import (
	"fmt"
	"net/http"

	"github.com/gorilla/mux"
)

func handler(w http.ResponseWriter, req *http.Request) {
	fmt.Fprint(w, "hi!\n")
}
func main() {
	r := mux.NewRouter()
	r.HandleFunc("/hi", handler)

	server0 := &http.Server{
		Addr:    ":7777",
		Handler: r,
	}
	server0.ListenAndServe()

}

And then hit it with curl using a URL like this:

curl -i 'http://localhost:7777/asdf/..%2Fanotherpath'
HTTP/1.1 301 Moved Permanently
Location: /anotherpath
Date: Tue, 13 Mar 2018 16:08:34 GMT
Content-Length: 0

%2F is and escaped /` and should not be interpreted as a path segment delimiter.

In this case, the location of the redirect is /anotherpath but it should be /..%2Fanotherpath

[tebruno99]

I'm OK with a PR to use RawPath. Needs significant tests + must pass existing tests.

[tebruno99]

The cleaning code is fine and I agree it mirrors net/http, I think the problem is you are cleaning and setting req.URL.Path and not req.URL.RawPath. This makes RawPath an invalid escaping of Path and thus causes url.String() (used in the rediect) to ignore it.

According to the HTTP spec, you are doing the wrong thing by turning %2F in / and then treating it as a path segment delimeter:

When a URI is dereferenced, the components and subcomponents
   significant to the scheme-specific dereferencing process (if any)
   must be parsed and separated before the percent-encoded octets within
   those components can be safely decoded, as otherwise the data may be
   mistaken for component delimiters.

I think the more correct thing to do would be to continue to clean the path but also mirror net/url's setPath() for setting both Path and RawPath correctly so that the url you pass back as the redirect location is accurate:

// setPath sets the Path and RawPath fields of the URL based on the provided
// escaped path p. It maintains the invariant that RawPath is only specified
// when it differs from the default encoding of the path.
// For example:
// - setPath("/foo/bar")   will set Path="/foo/bar" and RawPath=""
// - setPath("/foo%2fbar") will set Path="/foo/bar" and RawPath="/foo%2fbar"
// setPath will return an error only if the provided path contains an invalid
// escaping.
func (u *URL) setPath(p string) error {
	path, err := unescape(p, encodePath)
	if err != nil {
		return err
	}
	u.Path = path
	if escp := escape(path, encodePath); p == escp {
		// Default encoding is fine.
		u.RawPath = ""
	} else {
		u.RawPath = p
	}
	return nil
}

[tebruno99]

• You can use Route.SkipClean() to avoid cleaning a specific route - https://godoc.org/github.com/gorilla/mux#Route.SkipClean - there is also a similar method on Router
• The path cleaning code mirrors net/http - https://github.com/gorilla/mux/blob/master/mux.go#L439-L456 - and isn't something we can change. Most users do want mux to clean such paths, which is why we provide an escape hatch for the few cases where you may not.

[tebruno99]

Concerningly it looks like net/http.ServeMux suffers from the same issue - golang/go#14815

I'm not sure how anyone could consider a request to http://example.com/test/..%2Ffoobar to result in a HTTP 301 redirect to /foobar to be acceptable behaviour and not worthy of a breaking change to fix.

[tebruno99]

I just ran into this. This bug can cause SSRF security vulnerabilities due to directory traversal in code relying on gorilla/mux and I'm extremely surprised it's been known about for so long (and incorrectly marked as closed) considering the severity.

I'm all for soliciting patches from the community to fix issues, but leaving a security vulnerability open for years is extremely concerning.

[tebruno99]

While SkipClean fixes the issue, it also completely bypasses the URL canonicalisation which may be undesirable.

UseEncodedPath looks like a better option, although considering it opens the door for SSRF security vulnerabilities it arguably should be enabled by default (I'm not sure why unlike SkipClean it doesn't take a boolean though so once enabled it's not possible to disable the functionality).

Unfortunately it looks like there's a bug when UseEncodedPath is enabled since it reads from url.EscapedPath(), but then writes back to url.Path which is then escaped. For example a request to /foo/../bar%25 would be redirected to /bar%2525 instead of /bar%25 or /foo/..%2fbar becomes /foo/..%252fbar instead of remaining unchanged. Simply writing to RawPath instead of Path does not fix the issue as Path must match the unescaped version of RawPath or it is not used.

It looks like the solution is to call url.ParseRequestURI() on the cleaned path, then write the returned url's Path and RawPath back to the copied req.URL.

Setting UseEncodedPath to be enabled by default does not break any existing tests, nor does fixing the behaviour of UseEncodedPath to re-parse rather than writing to url.Path directly (although adding unit tests for the changed behaviours would be a good idea).

Would there be interest in merging these changes so UseEncodedPath is default (adding a new func to be able to disable the now-default behaviour, along with a warning why that would likely be a bad idea), deprecating the existing UseEncodedPath func in favour of the new one, and fixing the double-escape problem described above, and adding appropriate unit tests for the changed behaviour? If so I'm more than happy to submit a PR in the coming days with those changes (or multiple PRs if preferred).