fix(http): canonicalize paths before checking access rules (#6045)

http/auth.go

@@ -106,6 +106,8 @@ func withUser(fn handleFunc) handleFunc {
 		if err != nil {
 			return http.StatusInternalServerError, err
 		}
+
+		canonicalizeRequestPath(r)
 		return fn(w, r, d)
 	}
 }

http/data.go

@@ -35,6 +35,13 @@ type data struct {
 
 // Check implements rules.Checker.
 func (d *data) Check(path string) bool {
+	// Rules are written as "/"-separated virtual paths, but callers hand us
+	// paths built by the OS as well as ones taken from the request: afero.Walk
+	// and filepath.Join use "\" on Windows, where the filesystem also treats it
+	// as a separator. Canonicalize first so the authorization decision does not
+	// depend on which separator the caller happened to use.
+	path = slashClean(path)
+
 	// When the filesystem has been rebased (e.g. a public share rooted at a
 	// subdirectory), the incoming path is relative to that root. Resolve it
 	// back to the user's original scope before matching rules, otherwise rules
@@ -49,13 +56,13 @@ func (d *data) Check(path string) bool {
 
 	allow := true
 	for _, rule := range d.settings.Rules {
-		if rule.Matches(path) {
+		if rule.Matches(path, d.server.CaseInsensitiveFs) {
 			allow = rule.Allow
 		}
 	}
 
 	for _, rule := range d.user.Rules {
-		if rule.Matches(path) {
+		if rule.Matches(path, d.server.CaseInsensitiveFs) {
 			allow = rule.Allow
 		}
 	}

http/http.go

@@ -5,7 +5,9 @@ import (
 	"net/http"
 
 	"github.com/gorilla/mux"
+	"github.com/spf13/afero"
 
+	"github.com/filebrowser/filebrowser/v2/files"
 	"github.com/filebrowser/filebrowser/v2/settings"
 	"github.com/filebrowser/filebrowser/v2/storage"
 )
@@ -25,6 +27,7 @@ func NewHandler(
 	assetsFs fs.FS,
 ) (http.Handler, error) {
 	server.Clean()
+	server.CaseInsensitiveFs = files.CaseInsensitive(afero.NewOsFs(), server.Root)
 
 	r := mux.NewRouter()
 	r.Use(func(next http.Handler) http.Handler {

http/preview.go

@@ -47,8 +47,10 @@ func previewHandler(imgSvc ImgService, fileCache FileCache, enableThumbnails, re
 		}
 
 		file, err := files.NewFileInfo(&files.FileOptions{
-			Fs:         d.user.Fs,
-			Path:       "/" + vars["path"],
+			Fs: d.user.Fs,
+			// Preview reads its path from mux.Vars, not r.URL.Path, so it does
+			// not get the canonicalization withUser applies.
+			Path:       slashClean(vars["path"]),
 			Modify:     d.user.Perm.Modify,
 			Expand:     true,
 			ReadHeader: d.server.TypeDetectionByHeader,

http/public.go

@@ -52,14 +52,15 @@ var withHashFile = func(fn handleFunc) handleFunc {
 			return errToStatus(err), err
 		}
 
-		// share base path
-		basePath := link.Path
+		// share base path. Canonicalized because it roots both the rebased
+		// filesystem and checkerPrefix below, and a stored path that is not
+		// "/"-separated would make the two disagree on Windows.
+		basePath := slashClean(link.Path)
 
 		// file relative path
 		filePath := ""
 
 		if file.IsDir {
-			basePath = filepath.Clean(link.Path)
 			filePath = ifPath
 		}
 
@@ -109,7 +110,9 @@ func ifPathWithName(r *http.Request) (id, filePath string) {
 	case 1:
 		return r.URL.Path, "/"
 	default:
-		return pathElements[0], path.Join("/", path.Join(pathElements[1:]...))
+		// Public share routes do not pass through withUser, so canonicalize the
+		// share-relative path here instead.
+		return pathElements[0], slashClean(path.Join(pathElements[1:]...))
 	}
 }
 

http/raw.go

@@ -17,13 +17,6 @@ import (
 	"github.com/mholt/archives"
 )
 
-func slashClean(name string) string {
-	if name == "" || name[0] != '/' {
-		name = "/" + name
-	}
-	return gopath.Clean(name)
-}
-
 func parseQueryFiles(r *http.Request, f *files.FileInfo, _ *users.User) ([]string, error) {
 	var fileSlice []string
 	names := strings.Split(r.URL.Query().Get("files"), ",")

http/resource.go

@@ -221,8 +221,8 @@ func resourcePatchHandler(fileCache FileCache) handleFunc {
 		dst := r.URL.Query().Get("destination")
 		action := r.URL.Query().Get("action")
 		dst, err := url.QueryUnescape(dst)
-		dst = path.Clean("/" + dst)
-		src = path.Clean("/" + src)
+		dst = slashClean(dst)
+		src = slashClean(src)
 		if !d.Check(src) || !d.Check(dst) {
 			return http.StatusForbidden, nil
 		}

http/rules_path_test.go

@@ -0,0 +1,139 @@
+package fbhttp
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/filebrowser/filebrowser/v2/diskcache"
+	"github.com/filebrowser/filebrowser/v2/rules"
+	"github.com/filebrowser/filebrowser/v2/settings"
+	"github.com/filebrowser/filebrowser/v2/storage"
+	"github.com/filebrowser/filebrowser/v2/users"
+)
+
+// denyRuleStorage builds a scoped storage whose global settings deny denyPath.
+func denyRuleStorage(t *testing.T, userScope, denyPath string, perm users.Permissions, key []byte) *storage.Storage {
+	t.Helper()
+	st := scopedUserStorage(t, userScope, perm, key)
+	if err := st.Settings.Save(&settings.Settings{
+		Key:   key,
+		Rules: []rules.Rule{{Path: denyPath, Allow: false}},
+	}); err != nil {
+		t.Fatalf("failed to save settings: %v", err)
+	}
+	return st
+}
+
+// Regression for GHSA-fgm5-pw99-w2p7: on a case-insensitive filesystem
+// /Secret.txt and /secret.TXT name the same file, so a deny rule for one must
+// cover the other. The rule check runs before any filesystem access, so the 403
+// is asserted on every platform; the flag is what does the work, which the
+// case-sensitive half of the test pins down.
+func TestRuleDeniesCaseVariantWhenFsIsCaseInsensitive(t *testing.T) {
+	userScope := t.TempDir()
+	if err := os.WriteFile(filepath.Join(userScope, "Secret.txt"), []byte("SECRET"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	key := []byte("test-signing-key")
+	perm := users.Permissions{Download: true}
+	st := denyRuleStorage(t, userScope, "/Secret.txt", perm, key)
+	signed := signToken(t, perm, key)
+
+	get := func(path string, caseInsensitive bool) *httptest.ResponseRecorder {
+		req, _ := http.NewRequest(http.MethodGet, path, http.NoBody)
+		req.Header.Set("X-Auth", signed)
+		rec := httptest.NewRecorder()
+		handle(rawHandler, "", st, &settings.Server{CaseInsensitiveFs: caseInsensitive}).ServeHTTP(rec, req)
+		return rec
+	}
+
+	t.Run("canonical path is denied either way", func(t *testing.T) {
+		for _, caseInsensitive := range []bool{false, true} {
+			if rec := get("/Secret.txt", caseInsensitive); rec.Code != http.StatusForbidden {
+				t.Errorf("GET /Secret.txt (caseInsensitive=%v) = %d; want 403", caseInsensitive, rec.Code)
+			}
+		}
+	})
+
+	t.Run("case variant is denied on a case-insensitive filesystem", func(t *testing.T) {
+		if rec := get("/secret.TXT", true); rec.Code != http.StatusForbidden {
+			t.Errorf("VULNERABLE: GET /secret.TXT = %d, body=%q; want 403", rec.Code, rec.Body.String())
+		}
+	})
+
+	t.Run("case variant is a distinct path on a case-sensitive filesystem", func(t *testing.T) {
+		if rec := get("/secret.TXT", false); rec.Code == http.StatusForbidden {
+			t.Error("GET /secret.TXT = 403 with folding off; the rule must not widen on a case-sensitive filesystem")
+		}
+	})
+}
+
+// Regression for GHSA-fgm5-pw99-w2p7: a path is canonicalized before it is
+// matched against the rules, so a traversal sequence cannot reach a denied file
+// by spelling its way there. gorilla/mux cleans forward-slash traversal before
+// routing, so this is defense in depth on POSIX; on Windows the equivalent
+// backslash form reaches the handler intact, which cleanSeparators covers.
+func TestRuleDeniesTraversalToDeniedPath(t *testing.T) {
+	userScope := t.TempDir()
+	if err := os.MkdirAll(filepath.Join(userScope, "allow"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(userScope, "Secret.txt"), []byte("SECRET"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	key := []byte("test-signing-key")
+	perm := users.Permissions{Download: true}
+	st := denyRuleStorage(t, userScope, "/Secret.txt", perm, key)
+
+	req, _ := http.NewRequest(http.MethodGet, "/allow/../Secret.txt", http.NoBody)
+	req.Header.Set("X-Auth", signToken(t, perm, key))
+	rec := httptest.NewRecorder()
+	handle(rawHandler, "", st, &settings.Server{}).ServeHTTP(rec, req)
+
+	if rec.Code != http.StatusForbidden {
+		t.Fatalf("VULNERABLE: GET /allow/../Secret.txt = %d, body=%q; want 403", rec.Code, rec.Body.String())
+	}
+}
+
+// Canonicalizing the request path must not drop a trailing separator: POST
+// distinguishes "/dir/" (create a directory) from "/dir" (write a file), and
+// PUT rejects the directory form outright.
+func TestCanonicalizeRequestPathKeepsTrailingSlash(t *testing.T) {
+	userScope := t.TempDir()
+
+	key := []byte("test-signing-key")
+	perm := users.Permissions{Create: true, Modify: true}
+	st := scopedUserStorage(t, userScope, perm, key)
+	signed := signToken(t, perm, key)
+
+	t.Run("post creates a directory", func(t *testing.T) {
+		req, _ := http.NewRequest(http.MethodPost, "/newdir/", http.NoBody)
+		req.Header.Set("X-Auth", signed)
+		rec := httptest.NewRecorder()
+		handle(resourcePostHandler(diskcache.NewNoOp()), "", st, &settings.Server{}).ServeHTTP(rec, req)
+
+		info, err := os.Stat(filepath.Join(userScope, "newdir"))
+		if err != nil {
+			t.Fatalf("POST /newdir/ = %d, body=%q; directory not created: %v", rec.Code, rec.Body.String(), err)
+		}
+		if !info.IsDir() {
+			t.Error("POST /newdir/ created a file, not a directory")
+		}
+	})
+
+	t.Run("put rejects a directory path", func(t *testing.T) {
+		req, _ := http.NewRequest(http.MethodPut, "/newdir/", http.NoBody)
+		req.Header.Set("X-Auth", signed)
+		rec := httptest.NewRecorder()
+		handle(resourcePutHandler, "", st, &settings.Server{}).ServeHTTP(rec, req)
+
+		if rec.Code != http.StatusMethodNotAllowed {
+			t.Errorf("PUT /newdir/ = %d; want 405", rec.Code)
+		}
+	})
+}

http/utils.go

@@ -6,12 +6,56 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	gopath "path"
+	"path/filepath"
 	"strings"
 
 	libErrors "github.com/filebrowser/filebrowser/v2/errors"
 	imgErrors "github.com/filebrowser/filebrowser/v2/img"
 )
 
+// slashClean canonicalizes a virtual path to the absolute, "/"-separated,
+// lexically cleaned form that both the rule checker and the scoped filesystem
+// expect.
+//
+// Paths reach us either verbatim from a request or built by the OS — afero.Walk
+// and filepath.Join use "\" on Windows. There the filesystem resolves "\" as a
+// separator while the rule checker treated it as an ordinary character, so
+// "/allow\..\Secret.txt" evaded a rule for "/Secret.txt" and still opened it.
+// Normalizing with the host's own separator makes both layers agree on which
+// file a path names, and is a no-op on POSIX, where "\" is a legal character in
+// a filename and must stay one.
+func slashClean(name string) string {
+	return cleanSeparators(name, string(filepath.Separator))
+}
+
+// canonicalizeRequestPath rewrites the request path to its canonical virtual
+// form, so that the filesystem operation, the hook, the stored share record and
+// the thumbnail cache key all use the same string the rule check approved.
+//
+// A trailing separator is preserved: handlers distinguish "/dir/" from "/dir"
+// to decide whether to create a directory, and gopath.Clean would drop it.
+func canonicalizeRequestPath(r *http.Request) {
+	p := slashClean(r.URL.Path)
+	trailing := strings.HasSuffix(r.URL.Path, "/") || strings.HasSuffix(r.URL.Path, string(filepath.Separator))
+	if p != "/" && trailing {
+		p += "/"
+	}
+	r.URL.Path = p
+}
+
+// cleanSeparators is slashClean with the host separator passed in explicitly,
+// so the Windows behaviour can be exercised on any platform.
+func cleanSeparators(name, sep string) string {
+	if sep != "/" {
+		name = strings.ReplaceAll(name, sep, "/")
+	}
+	if name == "" || name[0] != '/' {
+		name = "/" + name
+	}
+	return gopath.Clean(name)
+}
+
 func renderJSON(w http.ResponseWriter, _ *http.Request, data interface{}) (int, error) {
 	marsh, err := json.Marshal(data)
 

http/utils_test.go

@@ -0,0 +1,45 @@
+package fbhttp
+
+import "testing"
+
+// cleanSeparators takes the host separator explicitly so the Windows behaviour
+// can be asserted from any platform. See GHSA-fgm5-pw99-w2p7: on Windows a
+// backslash is a path separator the filesystem resolves but the rule checker
+// used to treat as an ordinary character, so "/allow\..\Secret.txt" evaded a
+// rule for "/Secret.txt" and still opened it.
+func TestCleanSeparators(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		name string
+		in   string
+		sep  string
+		want string
+	}{
+		// Windows: a backslash is a separator and must be resolved before the
+		// path is matched against a rule.
+		{"windows traversal", `/allow\..\Secret.txt`, `\`, "/Secret.txt"},
+		{"windows leading backslash", `\Secret.txt`, `\`, "/Secret.txt"},
+		{"windows mixed separators", `/a/b\..\..\c`, `\`, "/c"},
+		{"windows already canonical", "/Secret.txt", `\`, "/Secret.txt"},
+		{"windows relative", `allow\..\Secret.txt`, `\`, "/Secret.txt"},
+		{"windows empty", "", `\`, "/"},
+
+		// POSIX: a backslash is a legal filename character and must survive, or
+		// files named with one become unreachable.
+		{"posix backslash is a filename character", `/a\b.txt`, "/", `/a\b.txt`},
+		{"posix traversal", "/allow/../Secret.txt", "/", "/Secret.txt"},
+		{"posix relative", "Secret.txt", "/", "/Secret.txt"},
+		{"posix multiple slashes", "//a///b", "/", "/a/b"},
+		{"posix empty", "", "/", "/"},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			if got := cleanSeparators(tc.in, tc.sep); got != tc.want {
+				t.Errorf("cleanSeparators(%q, %q) = %q; want %q", tc.in, tc.sep, got, tc.want)
+			}
+		})
+	}
+}