diff --git a/http/auth.go b/http/auth.go index 2f64f70c..4da5adcf 100644 --- a/http/auth.go +++ b/http/auth.go @@ -66,7 +66,7 @@ func (e extractor) ExtractToken(r *http.Request) (string, error) { return "", request.ErrNoTokenInRequest } -func renewableErr(err error, d *data) bool { +func renewableErr(err error, r *http.Request, d *data, tk *authToken) bool { if d.settings.AuthMethod != fbAuth.MethodProxyAuth || err == nil { return false } @@ -79,7 +79,38 @@ func renewableErr(err error, d *data) bool { return false } - return true + // The expiration is only waived because the trusted proxy, not the token, + // decides when the session ends. Require the proxy to still assert the same + // identity on this request, otherwise a token that leaked before it expired + // would authenticate on its own forever. + return proxyAsserts(r, d, tk.User.ID) +} + +// proxyAsserts reports whether the proxy-auth header on r identifies the user +// the token was issued for. The username is resolved through the user store, so +// that it is matched exactly as a regular proxy login would match it. +func proxyAsserts(r *http.Request, d *data, id uint) bool { + auther, err := d.store.Auth.Get(fbAuth.MethodProxyAuth) + if err != nil { + return false + } + + proxy, ok := auther.(*fbAuth.ProxyAuth) + if !ok || proxy.Header == "" { + return false + } + + username := r.Header.Get(proxy.Header) + if username == "" { + return false + } + + user, err := d.store.Users.Get(d.server.Root, d.server.FollowExternalSymlinks, username) + if err != nil { + return false + } + + return user.ID == id } func withUser(fn handleFunc) handleFunc { @@ -91,7 +122,7 @@ func withUser(fn handleFunc) handleFunc { var tk authToken p := jwt.NewParser(jwt.WithValidMethods([]string{jwt.SigningMethodHS256.Alg()}), jwt.WithExpirationRequired()) token, err := request.ParseFromRequest(r, &extractor{}, keyFunc, request.WithClaims(&tk), request.WithParser(p)) - if (err != nil || !token.Valid) && !renewableErr(err, d) { + if (err != nil || !token.Valid) && !renewableErr(err, r, d, &tk) { return http.StatusUnauthorized, nil } diff --git a/http/auth_test.go b/http/auth_test.go index bacff284..9389ecbb 100644 --- a/http/auth_test.go +++ b/http/auth_test.go @@ -6,11 +6,15 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/asdine/storm/v3" + "github.com/golang-jwt/jwt/v5" + fbAuth "github.com/filebrowser/filebrowser/v2/auth" "github.com/filebrowser/filebrowser/v2/settings" "github.com/filebrowser/filebrowser/v2/storage/bolt" + "github.com/filebrowser/filebrowser/v2/users" ) // Regression for the username-normalization home-directory collision @@ -69,3 +73,79 @@ func TestSignupRejectsCollidingNormalizedScope(t *testing.T) { t.Fatalf("scope owner = %q, want teamone-x", owner.Username) } } + +// Regression for GHSA-v3jv-rmh2-635j: under proxy auth with a non-default +// logout page the JWT expiration is waived, because the proxy owns the session +// lifetime. That exception used to apply to every route on the strength of the +// token alone, so a token stolen before it expired kept working — and could be +// renewed — indefinitely. The proxy must still assert the same identity. +func TestExpiredTokenNeedsProxyAssertion(t *testing.T) { + const proxyHeader = "X-Fb-User" + + key := []byte("test-signing-key") + perm := users.Permissions{Download: true} + st := scopedUserStorage(t, t.TempDir(), perm, key) + + if err := st.Settings.Save(&settings.Settings{ + Key: key, + AuthMethod: fbAuth.MethodProxyAuth, + LogoutPage: "/logged-out", + }); err != nil { + t.Fatalf("failed to save settings: %v", err) + } + if err := st.Auth.Save(&fbAuth.ProxyAuth{Header: proxyHeader}); err != nil { + t.Fatalf("failed to save auther: %v", err) + } + + expired := &authToken{ + User: userInfo{ID: 1, Username: "u", Perm: perm}, + RegisteredClaims: jwt.RegisteredClaims{ + IssuedAt: jwt.NewNumericDate(time.Now().Add(-2 * time.Hour)), + ExpiresAt: jwt.NewNumericDate(time.Now().Add(-time.Hour)), + }, + } + expiredToken, err := jwt.NewWithClaims(jwt.SigningMethodHS256, expired).SignedString(key) + if err != nil { + t.Fatalf("failed to sign token: %v", err) + } + + protected := withUser(func(w http.ResponseWriter, _ *http.Request, _ *data) (int, error) { + _, writeErr := w.Write([]byte("protected")) + return 0, writeErr + }) + + get := func(token, proxyUser string) *httptest.ResponseRecorder { + req, _ := http.NewRequest(http.MethodGet, "/", http.NoBody) + req.Header.Set("X-Auth", token) + if proxyUser != "" { + req.Header.Set(proxyHeader, proxyUser) + } + rec := httptest.NewRecorder() + handle(protected, "", st, &settings.Server{}).ServeHTTP(rec, req) + return rec + } + + t.Run("expired token alone is rejected", func(t *testing.T) { + if rec := get(expiredToken, ""); rec.Code != http.StatusUnauthorized { + t.Errorf("VULNERABLE: expired token without the proxy header = %d, body=%q; want 401", rec.Code, rec.Body.String()) + } + }) + + t.Run("expired token for another identity is rejected", func(t *testing.T) { + if rec := get(expiredToken, "someone-else"); rec.Code != http.StatusUnauthorized { + t.Errorf("VULNERABLE: expired token with a foreign proxy identity = %d; want 401", rec.Code) + } + }) + + t.Run("expired token the proxy still asserts is accepted", func(t *testing.T) { + if rec := get(expiredToken, "u"); rec.Code != http.StatusOK { + t.Errorf("expired token asserted by the proxy = %d, body=%q; want 200", rec.Code, rec.Body.String()) + } + }) + + t.Run("valid token needs no assertion", func(t *testing.T) { + if rec := get(signToken(t, perm, key), ""); rec.Code != http.StatusOK { + t.Errorf("valid token = %d, body=%q; want 200", rec.Code, rec.Body.String()) + } + }) +} diff --git a/http/data.go b/http/data.go index cbb438ef..57efa12d 100644 --- a/http/data.go +++ b/http/data.go @@ -35,25 +35,19 @@ 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 - // targeting paths below the share root would be silently bypassed. - if d.checkerPrefix != "" { - path = gopath.Join(d.checkerPrefix, path) - } - - if d.user.HideDotfiles && rules.MatchHidden(path) { + if d.user.HideDotfiles && rules.MatchHidden(d.rulePath(path)) { return false } + return d.CheckRules(path) +} + +// CheckRules reports whether the global and user rules allow path. Unlike +// Check, it ignores HideDotfiles: hiding dotfiles is a display preference, so +// it must not stop a user from operating on a tree that contains one. +func (d *data) CheckRules(path string) bool { + path = d.rulePath(path) + allow := true for _, rule := range d.settings.Rules { if rule.Matches(path, d.server.CaseInsensitiveFs) { @@ -70,6 +64,26 @@ func (d *data) Check(path string) bool { return allow } +// rulePath canonicalizes path into the form the rules are written in. +func (d *data) rulePath(path string) string { + // 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 + // targeting paths below the share root would be silently bypassed. + if d.checkerPrefix != "" { + path = gopath.Join(d.checkerPrefix, path) + } + + return path +} + func handle(fn handleFunc, prefix string, store *storage.Storage, server *settings.Server) http.Handler { handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { for k, v := range globalHeaders { diff --git a/http/resource.go b/http/resource.go index f38b3923..d1f7defe 100644 --- a/http/resource.go +++ b/http/resource.go @@ -103,6 +103,10 @@ func resourceDeleteHandler(fileCache FileCache) handleFunc { return errToStatus(err), err } + if err = checkDescendants(d, r.URL.Path, ""); err != nil { + return errToStatus(err), err + } + err = d.store.Share.DeleteWithPathPrefix(file.Path, d.user.ID) if err != nil { log.Printf("WARNING: Error(s) occurred while deleting associated shares with file: %s", err) @@ -259,6 +263,10 @@ func resourcePatchHandler(fileCache FileCache) handleFunc { } } + if err = checkDescendants(d, src, dst); err != nil { + return errToStatus(err), err + } + err = d.RunHook(func() error { return patchAction(r.Context(), action, src, dst, d, fileCache) }, action, src, dst, d.user) @@ -267,6 +275,42 @@ func resourcePatchHandler(fileCache FileCache) handleFunc { }) } +// checkDescendants reports an error if the rules deny any path inside the src +// tree or, when dst is not empty, the location that tree would land on. The +// handlers only authorize the root of the operation, but copy, rename and +// delete then recurse through the whole subtree, so a rule denying a descendant +// of an allowed directory would be bypassed by operating on the parent instead. +func checkDescendants(d *data, src, dst string) error { + if len(d.settings.Rules) == 0 && len(d.user.Rules) == 0 { + return nil + } + + return afero.Walk(d.user.Fs, src, func(fPath string, _ os.FileInfo, err error) error { + if err != nil { + return err + } + + if !d.CheckRules(fPath) { + return fberrors.ErrPermissionDenied + } + + if dst == "" { + return nil + } + + rel, err := filepath.Rel(src, fPath) + if err != nil { + return err + } + + if !d.CheckRules(filepath.Join(dst, rel)) { + return fberrors.ErrPermissionDenied + } + + return nil + }) +} + func checkParent(src, dst string) error { rel, err := filepath.Rel(src, dst) if err != nil { diff --git a/http/rules_recursive_test.go b/http/rules_recursive_test.go new file mode 100644 index 00000000..1484a351 --- /dev/null +++ b/http/rules_recursive_test.go @@ -0,0 +1,119 @@ +package fbhttp + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/filebrowser/filebrowser/v2/diskcache" + "github.com/filebrowser/filebrowser/v2/settings" + "github.com/filebrowser/filebrowser/v2/storage" + "github.com/filebrowser/filebrowser/v2/users" +) + +// Regression for GHSA-77x8-73f4-5485: copy, rename and delete authorize only +// the root of the operation and then recurse through the whole subtree, so a +// rule denying /src/secret used to be bypassed by copying, moving or deleting +// the allowed parent /src. +func TestRecursiveOperationsEnforceDescendantRules(t *testing.T) { + key := []byte("test-signing-key") + perm := users.Permissions{Create: true, Modify: true, Rename: true, Delete: true, Download: true} + + // scope builds a user scope holding an allowed parent with a denied child, + // plus an entirely allowed tree to prove legitimate operations still run. + scope := func(t *testing.T) (userScope string, st *storage.Storage) { + t.Helper() + userScope = t.TempDir() + for _, dir := range []string{filepath.Join("src", "secret"), "clean"} { + if err := os.MkdirAll(filepath.Join(userScope, dir), 0o755); err != nil { + t.Fatal(err) + } + } + files := map[string]string{ + filepath.Join("src", "public.txt"): "public", + filepath.Join("src", "secret", "marker.txt"): "SECRET", + filepath.Join("clean", "file.txt"): "clean", + } + for name, content := range files { + if err := os.WriteFile(filepath.Join(userScope, name), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + return userScope, denyRuleStorage(t, userScope, "/src/secret", perm, key) + } + + do := func(t *testing.T, st *storage.Storage, handler handleFunc, method, target string) *httptest.ResponseRecorder { + t.Helper() + req, _ := http.NewRequest(method, target, http.NoBody) + req.Header.Set("X-Auth", signToken(t, perm, key)) + rec := httptest.NewRecorder() + handle(handler, "", st, &settings.Server{}).ServeHTTP(rec, req) + return rec + } + + exists := func(t *testing.T, path string) bool { + t.Helper() + _, err := os.Stat(path) + return err == nil + } + + t.Run("denied descendant is not directly readable", func(t *testing.T) { + _, st := scope(t) + if rec := do(t, st, rawHandler, http.MethodGet, "/src/secret/marker.txt"); rec.Code != http.StatusForbidden { + t.Fatalf("GET /src/secret/marker.txt = %d; want 403", rec.Code) + } + }) + + t.Run("copying the parent does not expose it", func(t *testing.T) { + userScope, st := scope(t) + rec := do(t, st, resourcePatchHandler(diskcache.NewNoOp()), http.MethodPatch, "/src?action=copy&destination=/dst") + + if leaked := filepath.Join(userScope, "dst", "secret", "marker.txt"); exists(t, leaked) { + t.Fatalf("VULNERABLE: denied descendant copied to %s (status %d)", leaked, rec.Code) + } + if rec.Code != http.StatusForbidden { + t.Errorf("PATCH /src?action=copy = %d; want 403", rec.Code) + } + }) + + t.Run("renaming the parent does not expose it", func(t *testing.T) { + userScope, st := scope(t) + rec := do(t, st, resourcePatchHandler(diskcache.NewNoOp()), http.MethodPatch, "/src?action=rename&destination=/moved") + + if leaked := filepath.Join(userScope, "moved", "secret", "marker.txt"); exists(t, leaked) { + t.Fatalf("VULNERABLE: denied descendant moved to %s (status %d)", leaked, rec.Code) + } + if !exists(t, filepath.Join(userScope, "src", "secret", "marker.txt")) { + t.Error("denied descendant no longer at its original path") + } + if rec.Code != http.StatusForbidden { + t.Errorf("PATCH /src?action=rename = %d; want 403", rec.Code) + } + }) + + t.Run("deleting the parent does not delete it", func(t *testing.T) { + userScope, st := scope(t) + rec := do(t, st, resourceDeleteHandler(diskcache.NewNoOp()), http.MethodDelete, "/src") + + if !exists(t, filepath.Join(userScope, "src", "secret", "marker.txt")) { + t.Fatalf("VULNERABLE: denied descendant deleted through its parent (status %d)", rec.Code) + } + if rec.Code != http.StatusForbidden { + t.Errorf("DELETE /src = %d; want 403", rec.Code) + } + }) + + t.Run("an entirely allowed tree still copies", func(t *testing.T) { + userScope, st := scope(t) + rec := do(t, st, resourcePatchHandler(diskcache.NewNoOp()), http.MethodPatch, "/clean?action=copy&destination=/clean-copy") + + if rec.Code != http.StatusOK { + t.Fatalf("PATCH /clean?action=copy = %d, body=%q; want 200", rec.Code, rec.Body.String()) + } + if !exists(t, filepath.Join(userScope, "clean-copy", "file.txt")) { + t.Error("allowed tree was not copied") + } + }) +}