From e6d70cf24c0cd79a1787601dc99104ec7e7ca3ef Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Sun, 26 Jul 2026 10:00:18 +0200 Subject: [PATCH] fix(http): canonicalize paths before checking access rules (#6045) --- files/case.go | 34 ++++++++++ files/case_test.go | 44 +++++++++++++ http/auth.go | 2 + http/data.go | 11 +++- http/http.go | 3 + http/preview.go | 6 +- http/public.go | 11 ++-- http/raw.go | 7 -- http/resource.go | 4 +- http/rules_path_test.go | 139 ++++++++++++++++++++++++++++++++++++++++ http/utils.go | 44 +++++++++++++ http/utils_test.go | 45 +++++++++++++ rules/rules.go | 20 ++++-- rules/rules_test.go | 59 ++++++++++++++--- settings/settings.go | 5 ++ 15 files changed, 403 insertions(+), 31 deletions(-) create mode 100644 files/case.go create mode 100644 files/case_test.go create mode 100644 http/rules_path_test.go create mode 100644 http/utils_test.go diff --git a/files/case.go b/files/case.go new file mode 100644 index 00000000..24f06bb2 --- /dev/null +++ b/files/case.go @@ -0,0 +1,34 @@ +package files + +import ( + "path/filepath" + "runtime" + "strings" + + "github.com/spf13/afero" +) + +// CaseInsensitive reports whether the filesystem backing root treats file names +// case-insensitively, as NTFS, APFS, HFS+, exFAT and CIFS do. It creates a +// probe file and looks it up under a different case, mirroring how git detects +// core.ignoreCase. +// +// It probes rather than inferring from runtime.GOOS because neither direction +// holds: macOS can be formatted case-sensitively, and a Linux host commonly +// serves a case-insensitive mount. When the root cannot be written to, it falls +// back to the host's usual default rather than assuming case-sensitivity, since +// a read-only root is still exposed to the disclosure this guards against. +func CaseInsensitive(fs afero.Fs, root string) bool { + probe, err := afero.TempFile(fs, root, "fb-case-probe-") + if err != nil { + return runtime.GOOS == "windows" || runtime.GOOS == "darwin" + } + + name := probe.Name() + probe.Close() + defer fs.Remove(name) //nolint:errcheck + + dir, base := filepath.Split(name) + _, err = fs.Stat(filepath.Join(dir, strings.ToUpper(base))) + return err == nil +} diff --git a/files/case_test.go b/files/case_test.go new file mode 100644 index 00000000..8feb708c --- /dev/null +++ b/files/case_test.go @@ -0,0 +1,44 @@ +package files + +import ( + "strings" + "testing" + + "github.com/spf13/afero" +) + +// MemMapFs keys its entries by exact name, so it is case-sensitive. +func TestCaseInsensitiveReportsCaseSensitiveFs(t *testing.T) { + fs := afero.NewMemMapFs() + if err := fs.MkdirAll("/root", 0o755); err != nil { + t.Fatal(err) + } + + if CaseInsensitive(fs, "/root") { + t.Error("CaseInsensitive() = true for a case-sensitive filesystem; want false") + } + assertNoProbeLeft(t, fs, "/root") +} + +// Whatever the verdict, the probe file must not be left behind in the user's +// data directory. +func TestCaseInsensitiveRemovesProbe(t *testing.T) { + root := t.TempDir() + fs := afero.NewOsFs() + + t.Logf("CaseInsensitive(%s) = %v", root, CaseInsensitive(fs, root)) + assertNoProbeLeft(t, fs, root) +} + +func assertNoProbeLeft(t *testing.T, fs afero.Fs, root string) { + t.Helper() + entries, err := afero.ReadDir(fs, root) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), "fb-case-probe-") { + t.Errorf("probe file left behind: %s", e.Name()) + } + } +} diff --git a/http/auth.go b/http/auth.go index 25a2f1e3..2f64f70c 100644 --- a/http/auth.go +++ b/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) } } diff --git a/http/data.go b/http/data.go index 37ad5ef0..cbb438ef 100644 --- a/http/data.go +++ b/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 } } diff --git a/http/http.go b/http/http.go index 0a11f94e..46ab9508 100644 --- a/http/http.go +++ b/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 { diff --git a/http/preview.go b/http/preview.go index e57d597b..327313ce 100644 --- a/http/preview.go +++ b/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, diff --git a/http/public.go b/http/public.go index 211f8a87..b8fdf52c 100644 --- a/http/public.go +++ b/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:]...)) } } diff --git a/http/raw.go b/http/raw.go index 8f8c5751..6824c2e7 100644 --- a/http/raw.go +++ b/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"), ",") diff --git a/http/resource.go b/http/resource.go index c4a7b2f3..8ad66cc3 100644 --- a/http/resource.go +++ b/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 } diff --git a/http/rules_path_test.go b/http/rules_path_test.go new file mode 100644 index 00000000..d1b2905a --- /dev/null +++ b/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) + } + }) +} diff --git a/http/utils.go b/http/utils.go index dc51ccb6..2d132a6b 100644 --- a/http/utils.go +++ b/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) diff --git a/http/utils_test.go b/http/utils_test.go new file mode 100644 index 00000000..4f118f83 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/rules/rules.go b/rules/rules.go index 71f66930..e10b2e37 100644 --- a/rules/rules.go +++ b/rules/rules.go @@ -25,17 +25,29 @@ func MatchHidden(path string) bool { return path != "" && strings.HasPrefix(filepath.Base(path), ".") } -// Matches matches a path against a rule. -func (r *Rule) Matches(path string) bool { +// Matches matches a path against a rule. When fold is true the comparison is +// case-insensitive: on a case-insensitive filesystem two paths differing only +// in case name the same file, so a rule written for one spelling has to cover +// the others or it can be trivially evaded. +// +// Regex rules are never folded. An admin-authored pattern means what it says, +// and lowering its input would silently change it; use (?i) for those. +func (r *Rule) Matches(path string, fold bool) bool { if r.Regex { return r.Regexp.MatchString(path) } - if path == r.Path { + rulePath := r.Path + if fold { + path = strings.ToLower(path) + rulePath = strings.ToLower(rulePath) + } + + if path == rulePath { return true } - prefix := r.Path + prefix := rulePath if prefix != "/" && !strings.HasSuffix(prefix, "/") { prefix += "/" } diff --git a/rules/rules_test.go b/rules/rules_test.go index 3046a2bd..8a3df634 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -9,30 +9,69 @@ func TestRuleMatches(t *testing.T) { name string rulePath string testPath string + fold bool want bool }{ - {"exact match", "/uploads", "/uploads", true}, - {"child path", "/uploads", "/uploads/file.txt", true}, - {"sibling prefix", "/uploads", "/uploads_backup/secret.txt", false}, - {"root rule", "/", "/anything", true}, - {"trailing slash rule", "/uploads/", "/uploads/file.txt", true}, - {"trailing slash no sibling", "/uploads/", "/uploads_backup/file.txt", false}, - {"nested child", "/data/shared", "/data/shared/docs/file.txt", true}, - {"nested sibling", "/data/shared", "/data/shared_private/file.txt", false}, + {"exact match", "/uploads", "/uploads", false, true}, + {"child path", "/uploads", "/uploads/file.txt", false, true}, + {"sibling prefix", "/uploads", "/uploads_backup/secret.txt", false, false}, + {"root rule", "/", "/anything", false, true}, + {"trailing slash rule", "/uploads/", "/uploads/file.txt", false, true}, + {"trailing slash no sibling", "/uploads/", "/uploads_backup/file.txt", false, false}, + {"nested child", "/data/shared", "/data/shared/docs/file.txt", false, true}, + {"nested sibling", "/data/shared", "/data/shared_private/file.txt", false, false}, + + // On a case-insensitive filesystem /Secret.txt and /secret.TXT name the + // same file, so a rule for one must cover the other. See + // GHSA-fgm5-pw99-w2p7. + {"case variant not folded", "/Secret.txt", "/secret.TXT", false, false}, + {"case variant folded", "/Secret.txt", "/secret.TXT", true, true}, + {"case variant child folded", "/Private", "/private/notes.txt", true, true}, + {"exact match folded", "/uploads", "/uploads", true, true}, + // Folding must not widen a rule past its own path segment. + {"sibling prefix folded", "/uploads", "/UPLOADS_backup/secret.txt", true, false}, + {"nested sibling folded", "/data/shared", "/data/SHARED_private/x", true, false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { t.Parallel() r := &Rule{Path: tc.rulePath} - got := r.Matches(tc.testPath) + got := r.Matches(tc.testPath, tc.fold) if got != tc.want { - t.Errorf("Rule{Path: %q}.Matches(%q) = %v; want %v", tc.rulePath, tc.testPath, got, tc.want) + t.Errorf("Rule{Path: %q}.Matches(%q, fold=%v) = %v; want %v", + tc.rulePath, tc.testPath, tc.fold, got, tc.want) } }) } } +// A regex rule is authored by an administrator and means exactly what it says, +// so folding must not reach it: the admin opts in with (?i) instead. +func TestRuleMatchesRegexIgnoresFold(t *testing.T) { + t.Parallel() + + cases := []struct { + raw string + path string + want bool + }{ + {`^/Secret\.txt$`, "/Secret.txt", true}, + {`^/Secret\.txt$`, "/secret.TXT", false}, + {`(?i)^/Secret\.txt$`, "/secret.TXT", true}, + } + + for _, tc := range cases { + for _, fold := range []bool{false, true} { + r := &Rule{Regex: true, Regexp: &Regexp{Raw: tc.raw}} + if got := r.Matches(tc.path, fold); got != tc.want { + t.Errorf("Rule{Regexp: %q}.Matches(%q, fold=%v) = %v; want %v", + tc.raw, tc.path, fold, got, tc.want) + } + } + } +} + func TestMatchHidden(t *testing.T) { cases := map[string]bool{ "/": false, diff --git a/settings/settings.go b/settings/settings.go index 44297fd5..89670bbb 100644 --- a/settings/settings.go +++ b/settings/settings.go @@ -63,6 +63,11 @@ type Server struct { AuthHook string `json:"authHook"` TokenExpirationTime string `json:"tokenExpirationTime"` FollowExternalSymlinks bool `json:"followExternalSymlinks"` + + // CaseInsensitiveFs is detected from Root at startup rather than + // configured, and tells the rule checker to match paths case-insensitively. + // It is never persisted. + CaseInsensitiveFs bool `json:"-"` } // Clean cleans any variables that might need cleaning.