caddyhttp: Refactor and export SanitizedPathJoin for use in fastcgi (#4207)

8 files changed, 131 insertions, 134 deletions

diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index d93a5c8..784b2b9 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -20,7 +20,9 @@ import (
 	"io"
 	"net"
 	"net/http"
+	"path/filepath"
 	"strconv"
+	"strings"
 
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
@@ -217,6 +219,31 @@ func StatusCodeMatches(actual, configured int) bool {
 	return false
 }
 
+// SanitizedPathJoin performs filepath.Join(root, reqPath) that
+// is safe against directory traversal attacks. It uses logic
+// similar to that in the Go standard library, specifically
+// in the implementation of http.Dir. The root is assumed to
+// be a trusted path, but reqPath is not; and the output will
+// never be outside of root. The resulting path can be used
+// with the local file system.
+func SanitizedPathJoin(root, reqPath string) string {
+	if root == "" {
+		root = "."
+	}
+
+	path := filepath.Join(root, filepath.Clean("/"+reqPath))
+
+	// filepath.Join also cleans the path, and cleaning strips
+	// the trailing slash, so we need to re-add it afterwards.
+	// if the length is 1, then it's a path to the root,
+	// and that should return ".", so we don't append the separator.
+	if strings.HasSuffix(reqPath, "/") && len(reqPath) > 1 {
+		path += separator
+	}
+
+	return path
+}
+
 // tlsPlaceholderWrapper is a no-op listener wrapper that marks
 // where the TLS listener should be in a chain of listener wrappers.
 // It should only be used if another listener wrapper must be placed
@@ -242,6 +269,8 @@ const (
 	DefaultHTTPSPort = 443
 )
 
+const separator = string(filepath.Separator)
+
 // Interface guard
 var _ caddy.ListenerWrapper = (*tlsPlaceholderWrapper)(nil)
 var _ caddyfile.Unmarshaler = (*tlsPlaceholderWrapper)(nil)
diff --git a/modules/caddyhttp/caddyhttp_test.go b/modules/caddyhttp/caddyhttp_test.go
new file mode 100644
index 0000000..09011fe
--- /dev/null
+++ b/modules/caddyhttp/caddyhttp_test.go
@@ -0,0 +1,94 @@
+package caddyhttp
+
+import (
+	"net/url"
+	"path/filepath"
+	"testing"
+)
+
+func TestSanitizedPathJoin(t *testing.T) {
+	// For reference:
+	// %2e = .
+	// %2f = /
+	// %5c = \
+	for i, tc := range []struct {
+		inputRoot string
+		inputPath string
+		expect    string
+	}{
+		{
+			inputPath: "",
+			expect:    ".",
+		},
+		{
+			inputPath: "/",
+			expect:    ".",
+		},
+		{
+			inputPath: "/foo",
+			expect:    "foo",
+		},
+		{
+			inputPath: "/foo/",
+			expect:    "foo" + separator,
+		},
+		{
+			inputPath: "/foo/bar",
+			expect:    filepath.Join("foo", "bar"),
+		},
+		{
+			inputRoot: "/a",
+			inputPath: "/foo/bar",
+			expect:    filepath.Join("/", "a", "foo", "bar"),
+		},
+		{
+			inputPath: "/foo/../bar",
+			expect:    "bar",
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/foo/../bar",
+			expect:    filepath.Join("/", "a", "b", "bar"),
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/..%2fbar",
+			expect:    filepath.Join("/", "a", "b", "bar"),
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/%2e%2e%2fbar",
+			expect:    filepath.Join("/", "a", "b", "bar"),
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/%2e%2e%2f%2e%2e%2f",
+			expect:    filepath.Join("/", "a", "b") + separator,
+		},
+		{
+			inputRoot: "C:\\www",
+			inputPath: "/foo/bar",
+			expect:    filepath.Join("C:\\www", "foo", "bar"),
+		},
+		{
+			inputRoot: "C:\\www",
+			inputPath: "/D:\\foo\\bar",
+			expect:    filepath.Join("C:\\www", "D:\\foo\\bar"),
+		},
+	} {
+		// we don't *need* to use an actual parsed URL, but it
+		// adds some authenticity to the tests since real-world
+		// values will be coming in from URLs; thus, the test
+		// corpus can contain paths as encoded by clients, which
+		// more closely emulates the actual attack vector
+		u, err := url.Parse("http://test:9999" + tc.inputPath)
+		if err != nil {
+			t.Fatalf("Test %d: invalid URL: %v", i, err)
+		}
+		actual := SanitizedPathJoin(tc.inputRoot, u.Path)
+		if actual != tc.expect {
+			t.Errorf("Test %d: SanitizedPathJoin('%s', '%s') =>  %s (expected '%s')",
+				i, tc.inputRoot, tc.inputPath, actual, tc.expect)
+		}
+	}
+}
diff --git a/modules/caddyhttp/fileserver/browse.go b/modules/caddyhttp/fileserver/browse.go
index 3122f12..cd9bcbc 100644
--- a/modules/caddyhttp/fileserver/browse.go
+++ b/modules/caddyhttp/fileserver/browse.go
@@ -207,7 +207,7 @@ func isSymlinkTargetDir(f os.FileInfo, root, urlPath string) bool {
 	if !isSymlink(f) {
 		return false
 	}
-	target := sanitizedPathJoin(root, path.Join(urlPath, f.Name()))
+	target := caddyhttp.SanitizedPathJoin(root, path.Join(urlPath, f.Name()))
 	targetInfo, err := os.Stat(target)
 	if err != nil {
 		return false
diff --git a/modules/caddyhttp/fileserver/matcher.go b/modules/caddyhttp/fileserver/matcher.go
index 064398c..739b9e0 100644
--- a/modules/caddyhttp/fileserver/matcher.go
+++ b/modules/caddyhttp/fileserver/matcher.go
@@ -185,7 +185,7 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {
 		if strings.HasSuffix(file, "/") {
 			suffix += "/"
 		}
-		fullpath = sanitizedPathJoin(root, suffix)
+		fullpath = caddyhttp.SanitizedPathJoin(root, suffix)
 		return
 	}
 
diff --git a/modules/caddyhttp/fileserver/matcher_test.go b/modules/caddyhttp/fileserver/matcher_test.go
index e319907..746a5d7 100644
--- a/modules/caddyhttp/fileserver/matcher_test.go
+++ b/modules/caddyhttp/fileserver/matcher_test.go
@@ -94,7 +94,7 @@ func TestFileMatcher(t *testing.T) {
 			t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath)
 		}
 
-		fileType, ok := repl.Get("http.matchers.file.type")
+		fileType, _ := repl.Get("http.matchers.file.type")
 		if fileType != tc.expectedType {
 			t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType)
 		}
@@ -197,7 +197,7 @@ func TestPHPFileMatcher(t *testing.T) {
 			t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath)
 		}
 
-		fileType, ok := repl.Get("http.matchers.file.type")
+		fileType, _ := repl.Get("http.matchers.file.type")
 		if fileType != tc.expectedType {
 			t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType)
 		}
diff --git a/modules/caddyhttp/fileserver/staticfiles.go b/modules/caddyhttp/fileserver/staticfiles.go
index 9266332..45f8610 100644
--- a/modules/caddyhttp/fileserver/staticfiles.go
+++ b/modules/caddyhttp/fileserver/staticfiles.go
@@ -161,7 +161,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
 	filesToHide := fsrv.transformHidePaths(repl)
 
 	root := repl.ReplaceAll(fsrv.Root, ".")
-	filename := sanitizedPathJoin(root, r.URL.Path)
+	filename := caddyhttp.SanitizedPathJoin(root, r.URL.Path)
 
 	fsrv.logger.Debug("sanitized path join",
 		zap.String("site_root", root),
@@ -185,7 +185,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
 	var implicitIndexFile bool
 	if info.IsDir() && len(fsrv.IndexNames) > 0 {
 		for _, indexPage := range fsrv.IndexNames {
-			indexPath := sanitizedPathJoin(filename, indexPage)
+			indexPath := caddyhttp.SanitizedPathJoin(filename, indexPage)
 			if fileHidden(indexPath, filesToHide) {
 				// pretend this file doesn't exist
 				fsrv.logger.Debug("hiding index file",
@@ -435,42 +435,6 @@ func (fsrv *FileServer) transformHidePaths(repl *caddy.Replacer) []string {
 	return hide
 }
 
-// sanitizedPathJoin performs filepath.Join(root, reqPath) that
-// is safe against directory traversal attacks. It uses logic
-// similar to that in the Go standard library, specifically
-// in the implementation of http.Dir. The root is assumed to
-// be a trusted path, but reqPath is not.
-func sanitizedPathJoin(root, reqPath string) string {
-	// TODO: Caddy 1 uses this:
-	// prevent absolute path access on Windows, e.g. http://localhost:5000/C:\Windows\notepad.exe
-	// if runtime.GOOS == "windows" && len(reqPath) > 0 && filepath.IsAbs(reqPath[1:]) {
-	// TODO.
-	// }
-
-	// TODO: whereas std lib's http.Dir.Open() uses this:
-	// if filepath.Separator != '/' && strings.ContainsRune(name, filepath.Separator) {
-	// 	return nil, errors.New("http: invalid character in file path")
-	// }
-
-	// TODO: see https://play.golang.org/p/oh77BiVQFti for another thing to consider
-
-	if root == "" {
-		root = "."
-	}
-
-	path := filepath.Join(root, filepath.Clean("/"+reqPath))
-
-	// filepath.Join also cleans the path, and cleaning strips
-	// the trailing slash, so we need to re-add it afterwards.
-	// if the length is 1, then it's a path to the root,
-	// and that should return ".", so we don't append the separator.
-	if strings.HasSuffix(reqPath, "/") && len(reqPath) > 1 {
-		path += separator
-	}
-
-	return path
-}
-
 // fileHidden returns true if filename is hidden according to the hide list.
 // filename must be a relative or absolute file system path, not a request
 // URI path. It is expected that all the paths in the hide list are absolute
diff --git a/modules/caddyhttp/fileserver/staticfiles_test.go b/modules/caddyhttp/fileserver/staticfiles_test.go
index 690f46a..5d6133c 100644
--- a/modules/caddyhttp/fileserver/staticfiles_test.go
+++ b/modules/caddyhttp/fileserver/staticfiles_test.go
@@ -15,96 +15,12 @@
 package fileserver
 
 import (
-	"net/url"
 	"path/filepath"
 	"runtime"
 	"strings"
 	"testing"
 )
 
-func TestSanitizedPathJoin(t *testing.T) {
-	// For easy reference:
-	// %2e = .
-	// %2f = /
-	// %5c = \
-	for i, tc := range []struct {
-		inputRoot string
-		inputPath string
-		expect    string
-	}{
-		{
-			inputPath: "",
-			expect:    ".",
-		},
-		{
-			inputPath: "/",
-			expect:    ".",
-		},
-		{
-			inputPath: "/foo",
-			expect:    "foo",
-		},
-		{
-			inputPath: "/foo/",
-			expect:    "foo" + separator,
-		},
-		{
-			inputPath: "/foo/bar",
-			expect:    filepath.Join("foo", "bar"),
-		},
-		{
-			inputRoot: "/a",
-			inputPath: "/foo/bar",
-			expect:    filepath.Join("/", "a", "foo", "bar"),
-		},
-		{
-			inputPath: "/foo/../bar",
-			expect:    "bar",
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/foo/../bar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/..%2fbar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/%2e%2e%2fbar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/%2e%2e%2f%2e%2e%2f",
-			expect:    filepath.Join("/", "a", "b") + separator,
-		},
-		{
-			inputRoot: "C:\\www",
-			inputPath: "/foo/bar",
-			expect:    filepath.Join("C:\\www", "foo", "bar"),
-		},
-		// TODO: test more windows paths... on windows... sigh.
-	} {
-		// we don't *need* to use an actual parsed URL, but it
-		// adds some authenticity to the tests since real-world
-		// values will be coming in from URLs; thus, the test
-		// corpus can contain paths as encoded by clients, which
-		// more closely emulates the actual attack vector
-		u, err := url.Parse("http://test:9999" + tc.inputPath)
-		if err != nil {
-			t.Fatalf("Test %d: invalid URL: %v", i, err)
-		}
-		actual := sanitizedPathJoin(tc.inputRoot, u.Path)
-		if actual != tc.expect {
-			t.Errorf("Test %d: [%s %s] => %s (expected %s)",
-				i, tc.inputRoot, tc.inputPath, actual, tc.expect)
-		}
-	}
-}
-
 func TestFileHidden(t *testing.T) {
 	for i, tc := range []struct {
 		inputHide []string
diff --git a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go
index d7a0e36..40e0207 100644
--- a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go
+++ b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go
@@ -20,7 +20,6 @@ import (
 	"fmt"
 	"net"
 	"net/http"
-	"path"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -218,12 +217,7 @@ func (t Transport) buildEnv(r *http.Request) (map[string]string, error) {
 	}
 
 	// SCRIPT_FILENAME is the absolute path of SCRIPT_NAME
-	scriptFilename := filepath.Join(root, scriptName)
-
-	// Add vhost path prefix to scriptName. Otherwise, some PHP software will
-	// have difficulty discovering its URL.
-	pathPrefix, _ := r.Context().Value(caddy.CtxKey("path_prefix")).(string)
-	scriptName = path.Join(pathPrefix, scriptName)
+	scriptFilename := caddyhttp.SanitizedPathJoin(root, scriptName)
 
 	// Ensure the SCRIPT_NAME has a leading slash for compliance with RFC3875
 	// Info: https://tools.ietf.org/html/rfc3875#section-4.1.13
@@ -288,7 +282,7 @@ func (t Transport) buildEnv(r *http.Request) (map[string]string, error) {
 	// PATH_TRANSLATED should only exist if PATH_INFO is defined.
 	// Info: https://www.ietf.org/rfc/rfc3875 Page 14
 	if env["PATH_INFO"] != "" {
-		env["PATH_TRANSLATED"] = filepath.Join(root, pathInfo) // Info: http://www.oreilly.com/openbook/cgi/ch02_04.html
+		env["PATH_TRANSLATED"] = caddyhttp.SanitizedPathJoin(root, pathInfo) // Info: http://www.oreilly.com/openbook/cgi/ch02_04.html
 	}
 
 	// compliance with the CGI specification requires that