diff --git a/weed/server/filer_server_handlers.go b/weed/server/filer_server_handlers.go index da6a3e9d6..5880aacfe 100644 --- a/weed/server/filer_server_handlers.go +++ b/weed/server/filer_server_handlers.go @@ -216,15 +216,16 @@ func (fs *FilerServer) maybeCheckJwtAuthorization(r *http.Request, isWrite bool) return true } - return fs.checkJwtAuthorization(r, isWrite, jwtScopedRequestPaths(r)) + return fs.checkJwtAuthorization(r, isWrite, func() ([]string, error) { return jwtScopedRequestPaths(r), nil }) } // checkJwtAuthorization verifies the request carries a valid filer JWT for the -// requested access level and, for prefix-restricted tokens, that every path in -// scopedPaths falls within the token's AllowedPrefixes. Callers whose write -// target is not r.URL.Path — the TUS handler, whose URL points at the /.tus -// route — pass the resolved filer path(s) here instead. -func (fs *FilerServer) checkJwtAuthorization(r *http.Request, isWrite bool, scopedPaths []string) bool { +// requested access level and, for prefix-restricted tokens, that every resolved +// path falls within AllowedPrefixes. resolveScopedPaths is invoked only for a +// prefix-restricted token, so callers that read extra state to name their target +// (the TUS handler reads the session's stored path) pay for it only when it is +// consulted; a resolution error denies the request. +func (fs *FilerServer) checkJwtAuthorization(r *http.Request, isWrite bool, resolveScopedPaths func() ([]string, error)) bool { var signingKey security.SigningKey @@ -263,9 +264,11 @@ func (fs *FilerServer) checkJwtAuthorization(r *http.Request, isWrite bool, scop } if len(claims.AllowedPrefixes) > 0 { - // Copy and move name their source via a query parameter, not r.URL.Path. - // Scope every path the request reads or relocates, or a prefix-restricted - // token could reach data outside its allowed subtree. + scopedPaths, err := resolveScopedPaths() + if err != nil { + glog.V(1).Infof("jwt scope resolution failed from %s: %v", r.RemoteAddr, err) + return false + } for _, p := range scopedPaths { if !anyComponentPrefixMatches(claims.AllowedPrefixes, p) { glog.V(1).Infof("jwt path not allowed from %s: %v", r.RemoteAddr, p) diff --git a/weed/server/filer_server_tus_handlers.go b/weed/server/filer_server_tus_handlers.go index d2d815fe5..038bf5947 100644 --- a/weed/server/filer_server_tus_handlers.go +++ b/weed/server/filer_server_tus_handlers.go @@ -53,8 +53,7 @@ func (fs *FilerServer) tusHandler(w http.ResponseWriter, r *http.Request) { // Check if this is an upload location (contains upload ID after {tusPrefix}/.uploads/) uploadsPrefix := tusPrefix + "/.uploads/" if strings.HasPrefix(reqPath, uploadsPrefix) { - uploadID := strings.TrimPrefix(reqPath, uploadsPrefix) - uploadID = strings.Split(uploadID, "/")[0] // Get just the ID, not any trailing path + uploadID := fs.tusUploadID(reqPath) switch r.Method { case http.MethodHead: @@ -81,21 +80,53 @@ func (fs *FilerServer) tusHandler(w http.ResponseWriter, r *http.Request) { } // checkTusJwtAuthorization enforces filer JWT authorization for a TUS request. -// HEAD reads the current offset (read access); POST/PATCH/DELETE mutate the -// namespace (write access). Only creation (POST) names a new target path, taken -// from the request URL, so prefix-restricted tokens are scoped against that -// resolved filer path rather than the /.tus route. HEAD/PATCH/DELETE act on an -// existing session addressed by an unguessable id, whose target was already -// scope-checked at creation, so no further path scoping is applied to them. +// HEAD is a read; POST/PATCH/DELETE are writes. func (fs *FilerServer) checkTusJwtAuthorization(r *http.Request) bool { isWrite := r.Method != http.MethodHead - var scopedPaths []string - if r.Method == http.MethodPost { + return fs.checkJwtAuthorization(r, isWrite, func() ([]string, error) { + return fs.tusScopedPaths(r) + }) +} + +// tusScopedPaths returns the filer path(s) a prefix-restricted token must be +// allowed to reach. POST names its target in the URL; HEAD/PATCH/DELETE act on an +// existing session whose stored TargetPath must be re-checked, since the session +// id is not an authorization boundary against a tenant who already holds a token. +// A missing session stays unscoped (handler answers 404); any other lookup error +// is returned so the caller fails closed. +func (fs *FilerServer) tusScopedPaths(r *http.Request) ([]string, error) { + switch r.Method { + case http.MethodPost: if target := fs.tusTargetPath(r); target != "" && target != "/" { - scopedPaths = append(scopedPaths, target) + return []string{target}, nil + } + case http.MethodHead, http.MethodPatch, http.MethodDelete: + uploadID := fs.tusUploadID(r.URL.Path) + if uploadID == "" { + return nil, nil + } + session, err := fs.readTusSessionInfo(r.Context(), uploadID) + if err != nil { + if errors.Is(err, filer_pb.ErrNotFound) { + return nil, nil + } + return nil, err + } + if target := session.TargetPath; target != "" && target != "/" { + return []string{target}, nil } } - return fs.checkJwtAuthorization(r, isWrite, scopedPaths) + return nil, nil +} + +// tusUploadID extracts the session id from a {TusBasePath}/.uploads/{id} path, or +// "" when the path is not an uploads route. +func (fs *FilerServer) tusUploadID(reqPath string) string { + uploadsPrefix := fs.option.TusBasePath + "/.uploads/" + if !strings.HasPrefix(reqPath, uploadsPrefix) { + return "" + } + return strings.Split(strings.TrimPrefix(reqPath, uploadsPrefix), "/")[0] } // tusTargetPath resolves the filer path a TUS create request targets from the diff --git a/weed/server/filer_server_tus_idor_test.go b/weed/server/filer_server_tus_idor_test.go new file mode 100644 index 000000000..c8ee40c09 --- /dev/null +++ b/weed/server/filer_server_tus_idor_test.go @@ -0,0 +1,132 @@ +package weed_server + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/filer" + "github.com/seaweedfs/seaweedfs/weed/security" + "github.com/seaweedfs/seaweedfs/weed/util" +) + +// newTusIDORTestServer builds a FilerServer backed by an in-memory store seeded +// with TUS sessions (uploadID -> stored TargetPath), so the JWT check can resolve +// a session's target the way the real handler does. The store is returned so a +// test can seed additional (e.g. corrupt) session entries. +func newTusIDORTestServer(t *testing.T, writeKey, readKey string, sessions map[string]string) (*FilerServer, *renameTestStore) { + t.Helper() + store := newRenameTestStore() + fs := &FilerServer{ + filer: newRenameTestFiler(store), + filerGuard: security.NewGuard(nil, writeKey, 0, readKey, 0), + option: &FilerOption{TusBasePath: "/.tus"}, + } + for uploadID, targetPath := range sessions { + data, err := json.Marshal(&TusSession{ID: uploadID, TargetPath: targetPath, Size: 46}) + if err != nil { + t.Fatalf("marshal session %s: %v", uploadID, err) + } + if err := store.InsertEntry(context.Background(), &filer.Entry{FullPath: util.FullPath(fs.tusSessionInfoPath(uploadID)), Content: data}); err != nil { + t.Fatalf("seed session %s: %v", uploadID, err) + } + } + return fs, store +} + +// TestFilerServer_checkTusJwtAuthorization_CrossPrefixSessionHijack reproduces +// GHSA-99q7-x53r-6j4g: a prefix-restricted token acting on another tenant's +// existing TUS session (HEAD/PATCH/DELETE) must be scoped against the session's +// stored TargetPath, not authorized on signature and method alone. +func TestFilerServer_checkTusJwtAuthorization_CrossPrefixSessionHijack(t *testing.T) { + const writeKey = "write-secret" + const readKey = "read-secret" + + fs, store := newTusIDORTestServer(t, writeKey, readKey, map[string]string{ + "victim-session": "/buckets/secret/victim.bin", + "own-session": "/buckets/allowed/own.bin", + }) + + // A session whose .info is unreadable (corrupt JSON) must fail closed rather + // than authorize a prefix-restricted token against a target we cannot resolve. + if err := store.InsertEntry(context.Background(), &filer.Entry{ + FullPath: util.FullPath(fs.tusSessionInfoPath("corrupt-session")), + Content: []byte("{not valid json"), + }); err != nil { + t.Fatalf("seed corrupt session: %v", err) + } + + attackerWrite := signFilerToken(t, writeKey, []string{"/buckets/allowed"}, nil) + attackerRead := signFilerToken(t, readKey, []string{"/buckets/allowed"}, nil) + + tests := []struct { + name string + method string + path string + token string + expectAuthorized bool + }{ + // The IDOR: a token scoped to /buckets/allowed must not act on a session + // whose target is /buckets/secret, regardless of the verb. + {"patch victim session denied", http.MethodPatch, "/.tus/.uploads/victim-session", attackerWrite, false}, + {"delete victim session denied", http.MethodDelete, "/.tus/.uploads/victim-session", attackerWrite, false}, + {"head victim session denied", http.MethodHead, "/.tus/.uploads/victim-session", attackerRead, false}, + + // The same token acting on its own in-prefix session is still allowed. + {"patch own session allowed", http.MethodPatch, "/.tus/.uploads/own-session", attackerWrite, true}, + {"delete own session allowed", http.MethodDelete, "/.tus/.uploads/own-session", attackerWrite, true}, + {"head own session allowed", http.MethodHead, "/.tus/.uploads/own-session", attackerRead, true}, + + // An unrestricted token (no AllowedPrefixes) keeps working and triggers no + // session lookup. + {"patch unrestricted allowed", http.MethodPatch, "/.tus/.uploads/victim-session", signFilerToken(t, writeKey, nil, nil), true}, + + // An unknown session leaves the request unscoped so the handler can answer + // 404, rather than being denied on a path that cannot be resolved. + {"patch unknown session allowed", http.MethodPatch, "/.tus/.uploads/does-not-exist", attackerWrite, true}, + + // A corrupt/unreadable session fails closed: the target cannot be resolved + // so a prefix-restricted token must be denied, not authorized. + {"patch corrupt session denied", http.MethodPatch, "/.tus/.uploads/corrupt-session", attackerWrite, false}, + {"head corrupt session denied", http.MethodHead, "/.tus/.uploads/corrupt-session", attackerRead, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(tt.method, tt.path, nil) + req.Header.Set("Authorization", "Bearer "+tt.token) + if got := fs.checkTusJwtAuthorization(req); got != tt.expectAuthorized { + t.Errorf("checkTusJwtAuthorization(%s %s) = %v, want %v", tt.method, tt.path, got, tt.expectAuthorized) + } + }) + } +} + +// TestFilerServer_tusHandler_CrossPrefixPatchRejected drives the full handler: +// an attacker PATCH against another tenant's session must be rejected with 401 +// before any bytes are written to the session's target path. +func TestFilerServer_tusHandler_CrossPrefixPatchRejected(t *testing.T) { + const writeKey = "write-secret" + const readKey = "read-secret" + + fs, _ := newTusIDORTestServer(t, writeKey, readKey, map[string]string{ + "victim-session": "/buckets/secret/victim.bin", + }) + attacker := signFilerToken(t, writeKey, []string{"/buckets/allowed"}, nil) + + req := httptest.NewRequest(http.MethodPatch, "/.tus/.uploads/victim-session", strings.NewReader("PWNED-BY-CROSS-PREFIX-TUS-SESSION-HIJACK")) + req.Header.Set("Tus-Resumable", TusVersion) + req.Header.Set("Upload-Offset", "0") + req.Header.Set("Content-Type", "application/offset+octet-stream") + req.Header.Set("Authorization", "Bearer "+attacker) + + rec := httptest.NewRecorder() + fs.tusHandler(rec, req) + + if rec.Code != http.StatusUnauthorized { + t.Errorf("cross-prefix PATCH = %d, want %d", rec.Code, http.StatusUnauthorized) + } +} diff --git a/weed/server/filer_server_tus_session.go b/weed/server/filer_server_tus_session.go index 009d2a3eb..5cfd705e5 100644 --- a/weed/server/filer_server_tus_session.go +++ b/weed/server/filer_server_tus_session.go @@ -181,13 +181,14 @@ func (fs *FilerServer) saveTusSession(ctx context.Context, session *TusSession) return nil } -// getTusSession retrieves a TUS session by upload ID, including chunks from directory listing -func (fs *FilerServer) getTusSession(ctx context.Context, uploadID string) (*TusSession, error) { +// readTusSessionInfo reads and decodes a session's .info file without listing its +// chunks, the cheap lookup the authorization check needs for the stored TargetPath. +func (fs *FilerServer) readTusSessionInfo(ctx context.Context, uploadID string) (*TusSession, error) { infoPath := util.FullPath(fs.tusSessionInfoPath(uploadID)) entry, err := fs.filer.FindEntry(ctx, infoPath) if err != nil { if err == filer_pb.ErrNotFound { - return nil, fmt.Errorf("TUS upload session not found: %s", uploadID) + return nil, fmt.Errorf("TUS upload session not found: %s: %w", uploadID, filer_pb.ErrNotFound) } return nil, fmt.Errorf("find session: %w", err) } @@ -196,6 +197,15 @@ func (fs *FilerServer) getTusSession(ctx context.Context, uploadID string) (*Tus if err := json.Unmarshal(entry.Content, &session); err != nil { return nil, fmt.Errorf("unmarshal session: %w", err) } + return &session, nil +} + +// getTusSession retrieves a TUS session by upload ID, including chunks from directory listing +func (fs *FilerServer) getTusSession(ctx context.Context, uploadID string) (*TusSession, error) { + session, err := fs.readTusSessionInfo(ctx, uploadID) + if err != nil { + return nil, err + } // Load chunks from directory listing with pagination (atomic read, no race condition) sessionDirPath := util.FullPath(fs.tusSessionPath(uploadID)) @@ -248,7 +258,7 @@ func (fs *FilerServer) getTusSession(ctx context.Context, uploadID string) (*Tus session.Offset = contiguousEnd } - return &session, nil + return session, nil } // saveTusChunk stores the chunk info as a separate file entry