From 56d3e291c4579c484b173fbcee07b3334c910c23 Mon Sep 17 00:00:00 2001 From: Moritz Marquardt Date: Sun, 27 Aug 2023 10:13:15 +0200 Subject: [PATCH] Security Fix: clean paths correctly to avoid circumvention of BlacklistedPaths --- server/context/context.go | 6 ++-- server/utils/utils.go | 14 ++++++++++ server/utils/utils_test.go | 56 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/server/context/context.go b/server/context/context.go index 481fee2..6650164 100644 --- a/server/context/context.go +++ b/server/context/context.go @@ -48,11 +48,9 @@ func (c *Context) Redirect(uri string, statusCode int) { http.Redirect(c.RespWriter, c.Req, uri, statusCode) } -// Path returns requested path. -// -// The returned bytes are valid until your request handler returns. +// Path returns the cleaned requested path. func (c *Context) Path() string { - return c.Req.URL.Path + return utils.CleanPath(c.Req.URL.Path) } func (c *Context) Host() string { diff --git a/server/utils/utils.go b/server/utils/utils.go index 30f948d..91ed359 100644 --- a/server/utils/utils.go +++ b/server/utils/utils.go @@ -1,6 +1,8 @@ package utils import ( + "net/url" + "path" "strings" ) @@ -11,3 +13,15 @@ func TrimHostPort(host string) string { } return host } + +func CleanPath(uriPath string) string { + unescapedPath, _ := url.PathUnescape(uriPath) + cleanedPath := path.Join("/", unescapedPath) + + // If the path refers to a directory, add a trailing slash. + if !strings.HasSuffix(cleanedPath, "/") && (strings.HasSuffix(unescapedPath, "/") || strings.HasSuffix(unescapedPath, "/.") || strings.HasSuffix(unescapedPath, "/..")) { + cleanedPath += "/" + } + + return cleanedPath +} diff --git a/server/utils/utils_test.go b/server/utils/utils_test.go index 2532392..b8fcea9 100644 --- a/server/utils/utils_test.go +++ b/server/utils/utils_test.go @@ -11,3 +11,59 @@ func TestTrimHostPort(t *testing.T) { assert.EqualValues(t, "", TrimHostPort(":")) assert.EqualValues(t, "example.com", TrimHostPort("example.com:80")) } + +// TestCleanPath is mostly copied from fasthttp, to keep the behaviour we had before migrating away from it. +// Source (MIT licensed): https://github.com/valyala/fasthttp/blob/v1.48.0/uri_test.go#L154 +// Copyright (c) 2015-present Aliaksandr Valialkin, VertaMedia, Kirill Danshin, Erik Dubbelboer, FastHTTP Authors +func TestCleanPath(t *testing.T) { + // double slash + testURIPathNormalize(t, "/aa//bb", "/aa/bb") + + // triple slash + testURIPathNormalize(t, "/x///y/", "/x/y/") + + // multi slashes + testURIPathNormalize(t, "/abc//de///fg////", "/abc/de/fg/") + + // encoded slashes + testURIPathNormalize(t, "/xxxx%2fyyy%2f%2F%2F", "/xxxx/yyy/") + + // dotdot + testURIPathNormalize(t, "/aaa/..", "/") + + // dotdot with trailing slash + testURIPathNormalize(t, "/xxx/yyy/../", "/xxx/") + + // multi dotdots + testURIPathNormalize(t, "/aaa/bbb/ccc/../../ddd", "/aaa/ddd") + + // dotdots separated by other data + testURIPathNormalize(t, "/a/b/../c/d/../e/..", "/a/c/") + + // too many dotdots + testURIPathNormalize(t, "/aaa/../../../../xxx", "/xxx") + testURIPathNormalize(t, "/../../../../../..", "/") + testURIPathNormalize(t, "/../../../../../../", "/") + + // encoded dotdots + testURIPathNormalize(t, "/aaa%2Fbbb%2F%2E.%2Fxxx", "/aaa/xxx") + + // double slash with dotdots + testURIPathNormalize(t, "/aaa////..//b", "/b") + + // fake dotdot + testURIPathNormalize(t, "/aaa/..bbb/ccc/..", "/aaa/..bbb/") + + // single dot + testURIPathNormalize(t, "/a/./b/././c/./d.html", "/a/b/c/d.html") + testURIPathNormalize(t, "./foo/", "/foo/") + testURIPathNormalize(t, "./../.././../../aaa/bbb/../../../././../", "/") + testURIPathNormalize(t, "./a/./.././../b/./foo.html", "/b/foo.html") +} + +func testURIPathNormalize(t *testing.T, requestURI, expectedPath string) { + cleanedPath := CleanPath(requestURI) + if cleanedPath != expectedPath { + t.Fatalf("Unexpected path %q. Expected %q. requestURI=%q", cleanedPath, expectedPath, requestURI) + } +}