fix: enforce rules on recursive operations and expired proxy tokens (#6053)

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
 		}
 

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())
+		}
+	})
+}

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 {

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 {

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")
+		}
+	})
+}