From f8a2c602971c2a85747b8eda7e01a40b585b3149 Mon Sep 17 00:00:00 2001
From: Will Norris <will@tailscale.com>
Date: Sat, 1 Jun 2024 20:40:59 -0700
Subject: [PATCH] caddyhttp: properly sanitize requests for root path (#6360)

SanitizePathJoin protects against directory traversal attacks by
checking for requests whose URL path look like they are trying to
request something other than a local file, and returns the root
directory in those cases.

The method is also careful to ensure that requests which contain a
trailing slash include a trailing slash in the returned value.  However,
for requests that contain only a slash (requests for the root path), the
IsLocal check returns early before the matching trailing slash is
re-added.

This change updates SanitizePathJoin to only perform the
filepath.IsLocal check if the cleaned request URL path is non-empty.

---

This change also updates the existing SanitizePathJoin tests to use
filepath.FromSlash rather than filepath.Join. This makes the expected
value a little easier to read, but also has the advantage of not being
processed by filepath.Clean like filepath.Join is. This means that the
exact expect value will be compared, not the result of first cleaning
it.

Fixes #6352
---
 modules/caddyhttp/caddyhttp.go      |  2 +-
 modules/caddyhttp/caddyhttp_test.go | 28 ++++++++++++++++++----------
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index f42b166c..e1e71f4a 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -239,7 +239,7 @@ func SanitizedPathJoin(root, reqPath string) string {
 	}
 
 	relPath := path.Clean("/" + reqPath)[1:] // clean path and trim the leading /
-	if !filepath.IsLocal(relPath) {
+	if relPath != "" && !filepath.IsLocal(relPath) {
 		// path is unsafe (see https://github.com/golang/go/issues/56336#issuecomment-1416214885)
 		return root
 	}
diff --git a/modules/caddyhttp/caddyhttp_test.go b/modules/caddyhttp/caddyhttp_test.go
index 4763c383..0062c671 100644
--- a/modules/caddyhttp/caddyhttp_test.go
+++ b/modules/caddyhttp/caddyhttp_test.go
@@ -26,22 +26,28 @@ func TestSanitizedPathJoin(t *testing.T) {
 			inputPath: "/",
 			expect:    ".",
 		},
+		{
+			// fileserver.MatchFile passes an inputPath of "//" for some try_files values.
+			// See https://github.com/caddyserver/caddy/issues/6352
+			inputPath: "//",
+			expect:    filepath.FromSlash("./"),
+		},
 		{
 			inputPath: "/foo",
 			expect:    "foo",
 		},
 		{
 			inputPath: "/foo/",
-			expect:    "foo" + separator,
+			expect:    filepath.FromSlash("foo/"),
 		},
 		{
 			inputPath: "/foo/bar",
-			expect:    filepath.Join("foo", "bar"),
+			expect:    filepath.FromSlash("foo/bar"),
 		},
 		{
 			inputRoot: "/a",
 			inputPath: "/foo/bar",
-			expect:    filepath.Join("/", "a", "foo", "bar"),
+			expect:    filepath.FromSlash("/a/foo/bar"),
 		},
 		{
 			inputPath: "/foo/../bar",
@@ -50,32 +56,34 @@ func TestSanitizedPathJoin(t *testing.T) {
 		{
 			inputRoot: "/a/b",
 			inputPath: "/foo/../bar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
+			expect:    filepath.FromSlash("/a/b/bar"),
 		},
 		{
 			inputRoot: "/a/b",
 			inputPath: "/..%2fbar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
+			expect:    filepath.FromSlash("/a/b/bar"),
 		},
 		{
 			inputRoot: "/a/b",
 			inputPath: "/%2e%2e%2fbar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
+			expect:    filepath.FromSlash("/a/b/bar"),
 		},
 		{
+			// inputPath fails the IsLocal test so only the root is returned,
+			// but with a trailing slash since one was included in inputPath
 			inputRoot: "/a/b",
 			inputPath: "/%2e%2e%2f%2e%2e%2f",
-			expect:    "/a/b", // inputPath fails the IsLocal test so only the root is returned
+			expect:    filepath.FromSlash("/a/b/"),
 		},
 		{
 			inputRoot: "/a/b",
 			inputPath: "/foo%2fbar",
-			expect:    filepath.Join("/", "a", "b", "foo", "bar"),
+			expect:    filepath.FromSlash("/a/b/foo/bar"),
 		},
 		{
 			inputRoot: "/a/b",
 			inputPath: "/foo%252fbar",
-			expect:    filepath.Join("/", "a", "b", "foo%2fbar"),
+			expect:    filepath.FromSlash("/a/b/foo%2fbar"),
 		},
 		{
 			inputRoot: "C:\\www",
@@ -92,7 +100,7 @@ func TestSanitizedPathJoin(t *testing.T) {
 			// https://github.com/golang/go/issues/56336#issuecomment-1416214885
 			inputRoot: "root",
 			inputPath: "/a/b/../../c",
-			expect:    filepath.Join("root", "c"),
+			expect:    filepath.FromSlash("root/c"),
 		},
 	} {
 		// we don't *need* to use an actual parsed URL, but it