Sanitize paths in static file server; some cleanup

Also remove AutomaticHTTPSError for now

3 files changed, 159 insertions, 50 deletions

diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index 41b0c66..e5b9f61 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -95,21 +95,11 @@ func (app *App) Validate() error {
 	return nil
 }
 
-// AutomaticHTTPSError represents an error received when attempting to enable automatic https.
-type AutomaticHTTPSError struct {
-	internal error
-}
-
-// Error returns the error string for automaticHTTPSError.
-func (a AutomaticHTTPSError) Error() string {
-	return fmt.Sprintf("enabling automatic HTTPS: %v", a.internal.Error())
-}
-
 // Start runs the app. It sets up automatic HTTPS if enabled.
 func (app *App) Start() error {
 	err := app.automaticHTTPS()
 	if err != nil {
-		return &AutomaticHTTPSError{internal: err}
+		return fmt.Errorf("enabling automatic HTTPS: %v", err)
 	}
 
 	for srvName, srv := range app.Servers {
@@ -243,7 +233,7 @@ func (app *App) automaticHTTPS() error {
 				if parts := strings.SplitN(port, "-", 2); len(parts) == 2 {
 					port = parts[0]
 				}
-				redirTo := "https://{request.host}"
+				redirTo := "https://{http.request.host}"
 
 				httpsPort := app.HTTPSPort
 				if httpsPort == 0 {
@@ -252,7 +242,7 @@ func (app *App) automaticHTTPS() error {
 				if port != strconv.Itoa(httpsPort) {
 					redirTo += ":" + port
 				}
-				redirTo += "{request.uri}"
+				redirTo += "{http.request.uri}"
 
 				redirRoutes = append(redirRoutes, ServerRoute{
 					matchers: []RequestMatcher{
diff --git a/modules/caddyhttp/staticfiles/staticfiles.go b/modules/caddyhttp/staticfiles/staticfiles.go
index e3af352..f9fd8d2 100644
--- a/modules/caddyhttp/staticfiles/staticfiles.go
+++ b/modules/caddyhttp/staticfiles/staticfiles.go
@@ -72,14 +72,21 @@ func (sf *StaticFiles) Provision(ctx caddy2.Context) error {
 	return nil
 }
 
+const (
+	selectionPolicyFirstExisting = "first_existing"
+	selectionPolicyLargestSize   = "largest_size"
+	selectionPolicySmallestSize  = "smallest_size"
+	selectionPolicyRecentlyMod   = "most_recently_modified"
+)
+
 // Validate ensures that sf has a valid configuration.
 func (sf *StaticFiles) Validate() error {
 	switch sf.SelectionPolicy {
 	case "",
-		"first_existing",
-		"largest_size",
-		"smallest_size",
-		"most_recently_modified":
+		selectionPolicyFirstExisting,
+		selectionPolicyLargestSize,
+		selectionPolicySmallestSize,
+		selectionPolicyRecentlyMod:
 	default:
 		return fmt.Errorf("unknown selection policy %s", sf.SelectionPolicy)
 	}
@@ -87,15 +94,6 @@ func (sf *StaticFiles) Validate() error {
 }
 
 func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
-	// TODO: Prevent directory traversal, see https://play.golang.org/p/oh77BiVQFti
-
-	// TODO: Still needed?
-	// // Prevent absolute path access on Windows, e.g.: http://localhost:5000/C:\Windows\notepad.exe
-	// // TODO: does stdlib http.Dir handle this? see first check of http.Dir.Open()...
-	// if runtime.GOOS == "windows" && len(reqPath) > 0 && filepath.IsAbs(reqPath[1:]) {
-	// 	return caddyhttp.Error(http.StatusNotFound, fmt.Errorf("request path was absolute"))
-	// }
-
 	repl := r.Context().Value(caddy2.ReplacerCtxKey).(caddy2.Replacer)
 
 	// map the request to a filename
@@ -113,7 +111,6 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 	// if the ultimate destination has changed, submit
 	// this request for a rehandling (internal redirect)
 	// if configured to do so
-	// TODO: double check this against https://docs.nginx.com/nginx/admin-guide/web-server/serving-static-content/
 	if r.URL.Path != pathBefore && sf.Rehandle {
 		return caddyhttp.ErrRehandle
 	}
@@ -121,6 +118,7 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 	// get information about the file
 	info, err := os.Stat(filename)
 	if err != nil {
+		err = mapDirOpenError(err, filename)
 		if os.IsNotExist(err) {
 			return caddyhttp.Error(http.StatusNotFound, err)
 		} else if os.IsPermission(err) {
@@ -136,7 +134,7 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 		filesToHide := sf.transformHidePaths(repl)
 
 		for _, indexPage := range sf.IndexNames {
-			indexPath := path.Join(filename, indexPage)
+			indexPath := sanitizedPathJoin(filename, indexPage)
 			if fileHidden(indexPath, filesToHide) {
 				// pretend this file doesn't exist
 				continue
@@ -150,8 +148,6 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 			// we found an index file that might work,
 			// so rewrite the request path and, if
 			// configured, do an internal redirect
-			// TODO: I don't know if the logic for rewriting
-			// the URL here is the right logic
 			r.URL.Path = path.Join(r.URL.Path, indexPage)
 			if sf.Rehandle {
 				return caddyhttp.ErrRehandle
@@ -172,6 +168,8 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 		return caddyhttp.Error(http.StatusNotFound, nil)
 	}
 
+	// TODO: content negotiation (brotli sidecar files, etc...)
+
 	// open the file
 	file, err := sf.openFile(filename, w)
 	if err != nil {
@@ -179,9 +177,7 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 	}
 	defer file.Close()
 
-	// TODO: Etag?
-
-	// TODO: content negotiation? (brotli sidecar files, etc...)
+	// TODO: Etag
 
 	// let the standard library do what it does best; note, however,
 	// that errors generated by ServeContent are written immediately
@@ -199,6 +195,7 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 func (sf *StaticFiles) openFile(filename string, w http.ResponseWriter) (*os.File, error) {
 	file, err := os.Open(filename)
 	if err != nil {
+		err = mapDirOpenError(err, filename)
 		if os.IsNotExist(err) {
 			return nil, caddyhttp.Error(http.StatusNotFound, err)
 		} else if os.IsPermission(err) {
@@ -213,6 +210,34 @@ func (sf *StaticFiles) openFile(filename string, w http.ResponseWriter) (*os.Fil
 	return file, nil
 }
 
+// mapDirOpenError maps the provided non-nil error from opening name
+// to a possibly better non-nil error. In particular, it turns OS-specific errors
+// about opening files in non-directories into os.ErrNotExist. See golang/go#18984.
+// Adapted from the Go standard library; originally written by Nathaniel Caza.
+// https://go-review.googlesource.com/c/go/+/36635/
+// https://go-review.googlesource.com/c/go/+/36804/
+func mapDirOpenError(originalErr error, name string) error {
+	if os.IsNotExist(originalErr) || os.IsPermission(originalErr) {
+		return originalErr
+	}
+
+	parts := strings.Split(name, string(filepath.Separator))
+	for i := range parts {
+		if parts[i] == "" {
+			continue
+		}
+		fi, err := os.Stat(strings.Join(parts[:i+1], string(filepath.Separator)))
+		if err != nil {
+			return originalErr
+		}
+		if !fi.IsDir() {
+			return os.ErrNotExist
+		}
+	}
+
+	return originalErr
+}
+
 // transformHidePaths performs replacements for all the elements of
 // sf.Hide and returns a new list of the transformed values.
 func (sf *StaticFiles) transformHidePaths(repl caddy2.Replacer) []string {
@@ -223,42 +248,60 @@ func (sf *StaticFiles) transformHidePaths(repl caddy2.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.
+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 = "."
+	}
+	return filepath.Join(root, filepath.FromSlash(path.Clean("/"+reqPath)))
+}
+
 // selectFile uses the specified selection policy (or first_existing
 // by default) to map the request r to a filename. The full path to
 // the file is returned if one is found; otherwise, an empty string
 // is returned.
 func (sf *StaticFiles) selectFile(r *http.Request, repl caddy2.Replacer) string {
 	root := repl.ReplaceAll(sf.Root, "")
-	if root == "" {
-		root = "."
-	}
 
 	if sf.Files == nil {
-		return filepath.Join(root, r.URL.Path)
+		return sanitizedPathJoin(root, r.URL.Path)
 	}
 
 	switch sf.SelectionPolicy {
-	// TODO: Make these policy names constants
-	case "", "first_existing":
+	case "", selectionPolicyFirstExisting:
 		filesToHide := sf.transformHidePaths(repl)
 		for _, f := range sf.Files {
 			suffix := repl.ReplaceAll(f, "")
-			// TODO: sanitize path
-			fullpath := filepath.Join(root, suffix)
+			fullpath := sanitizedPathJoin(root, suffix)
 			if !fileHidden(fullpath, filesToHide) && fileExists(fullpath) {
 				r.URL.Path = suffix
 				return fullpath
 			}
 		}
 
-	case "largest_size":
+	case selectionPolicyLargestSize:
 		var largestSize int64
 		var largestFilename string
 		var largestSuffix string
 		for _, f := range sf.Files {
 			suffix := repl.ReplaceAll(f, "")
-			// TODO: sanitize path
-			fullpath := filepath.Join(root, suffix)
+			fullpath := sanitizedPathJoin(root, suffix)
 			info, err := os.Stat(fullpath)
 			if err == nil && info.Size() > largestSize {
 				largestSize = info.Size()
@@ -269,14 +312,13 @@ func (sf *StaticFiles) selectFile(r *http.Request, repl caddy2.Replacer) string
 		r.URL.Path = largestSuffix
 		return largestFilename
 
-	case "smallest_size":
+	case selectionPolicySmallestSize:
 		var smallestSize int64
 		var smallestFilename string
 		var smallestSuffix string
 		for _, f := range sf.Files {
 			suffix := repl.ReplaceAll(f, "")
-			// TODO: sanitize path
-			fullpath := filepath.Join(root, suffix)
+			fullpath := sanitizedPathJoin(root, suffix)
 			info, err := os.Stat(fullpath)
 			if err == nil && (smallestSize == 0 || info.Size() < smallestSize) {
 				smallestSize = info.Size()
@@ -287,14 +329,13 @@ func (sf *StaticFiles) selectFile(r *http.Request, repl caddy2.Replacer) string
 		r.URL.Path = smallestSuffix
 		return smallestFilename
 
-	case "most_recently_modified":
+	case selectionPolicyRecentlyMod:
 		var recentDate time.Time
 		var recentFilename string
 		var recentSuffix string
 		for _, f := range sf.Files {
 			suffix := repl.ReplaceAll(f, "")
-			// TODO: sanitize path
-			fullpath := filepath.Join(root, suffix)
+			fullpath := sanitizedPathJoin(root, suffix)
 			info, err := os.Stat(fullpath)
 			if err == nil &&
 				(recentDate.IsZero() || info.ModTime().After(recentDate)) {
diff --git a/modules/caddyhttp/staticfiles/staticfiles_test.go b/modules/caddyhttp/staticfiles/staticfiles_test.go
new file mode 100644
index 0000000..f2e1c89
--- /dev/null
+++ b/modules/caddyhttp/staticfiles/staticfiles_test.go
@@ -0,0 +1,78 @@
+package staticfiles
+
+import (
+	"net/url"
+	"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/bar",
+			expect:    "foo/bar",
+		},
+		{
+			inputRoot: "/a",
+			inputPath: "/foo/bar",
+			expect:    "/a/foo/bar",
+		},
+		{
+			inputPath: "/foo/../bar",
+			expect:    "bar",
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/foo/../bar",
+			expect:    "/a/b/bar",
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/..%2fbar",
+			expect:    "/a/b/bar",
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/%2e%2e%2fbar",
+			expect:    "/a/b/bar",
+		},
+		{
+			inputRoot: "/a/b",
+			inputPath: "/%2e%2e%2f%2e%2e%2f",
+			expect:    "/a/b",
+		},
+	} {
+		// 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)
+		}
+	}
+}