diff --git a/weed/server/filer_server_handlers.go b/weed/server/filer_server_handlers.go index 5880aacfe..7050aee1f 100644 --- a/weed/server/filer_server_handlers.go +++ b/weed/server/filer_server_handlers.go @@ -216,66 +216,58 @@ func (fs *FilerServer) maybeCheckJwtAuthorization(r *http.Request, isWrite bool) return true } - return fs.checkJwtAuthorization(r, isWrite, func() ([]string, error) { return jwtScopedRequestPaths(r), nil }) + return fs.checkJwtAuthorization(r, isWrite, jwtScopedRequestPaths(r)) } // checkJwtAuthorization verifies the request carries a valid filer JWT for the -// 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 { +// requested access level and, for prefix-restricted tokens, that every path in +// scopedPaths falls within AllowedPrefixes. +func (fs *FilerServer) checkJwtAuthorization(r *http.Request, isWrite bool, scopedPaths []string) bool { + claims, ok := fs.authenticateFilerJwt(r, isWrite) + if !ok { + return false + } + return authorizeFilerJwtPaths(r, claims, scopedPaths) +} + +// authenticateFilerJwt verifies the JWT signature and method claims. A nil claims +// with ok true means authentication is disabled for this access level. Splitting +// authentication from path authorization lets a handler load an indirect resource +// (the TUS session target) only after the caller's credential is verified. +func (fs *FilerServer) authenticateFilerJwt(r *http.Request, isWrite bool) (*security.SeaweedFilerClaims, bool) { var signingKey security.SigningKey - if isWrite { signingKey = fs.filerGuard.SigningKey() - if len(signingKey) == 0 { - return true - } } else { signingKey = fs.filerGuard.ReadSigningKey() - if len(signingKey) == 0 { - return true - } + } + if len(signingKey) == 0 { + return nil, true } tokenStr := security.GetJwt(r) if tokenStr == "" { glog.V(1).Infof("missing jwt from %s", r.RemoteAddr) - return false + return nil, false } token, err := security.DecodeJwt(signingKey, tokenStr, &security.SeaweedFilerClaims{}) if err != nil { glog.V(1).Infof("jwt verification error from %s: %v", r.RemoteAddr, err) - return false + return nil, false } if !token.Valid { glog.V(1).Infof("jwt invalid from %s: %v", r.RemoteAddr, tokenStr) - return false + return nil, false } claims, ok := token.Claims.(*security.SeaweedFilerClaims) if !ok { glog.V(1).Infof("jwt claims not of type *SeaweedFilerClaims from %s", r.RemoteAddr) - return false + return nil, false } - if len(claims.AllowedPrefixes) > 0 { - 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) - return false - } - } - } if len(claims.AllowedMethods) > 0 { hasMethod := false for _, method := range claims.AllowedMethods { @@ -286,10 +278,30 @@ func (fs *FilerServer) checkJwtAuthorization(r *http.Request, isWrite bool, reso } if !hasMethod { glog.V(1).Infof("jwt method not allowed from %s: %v", r.RemoteAddr, r.Method) - return false + return nil, false } } + return claims, true +} + +// authorizeFilerJwtPaths checks the resource scope after authentication. A +// prefix-restricted token must name at least one resource path; an empty list +// fails closed, so a caller that cannot resolve its target never falls open. +func authorizeFilerJwtPaths(r *http.Request, claims *security.SeaweedFilerClaims, scopedPaths []string) bool { + if claims == nil || len(claims.AllowedPrefixes) == 0 { + return true + } + if len(scopedPaths) == 0 { + glog.V(1).Infof("jwt resource path missing from %s", r.RemoteAddr) + 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) + return false + } + } return true } diff --git a/weed/server/filer_server_tus_handlers.go b/weed/server/filer_server_tus_handlers.go index 038bf5947..623cbe6ef 100644 --- a/weed/server/filer_server_tus_handlers.go +++ b/weed/server/filer_server_tus_handlers.go @@ -17,6 +17,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/glog" "github.com/seaweedfs/seaweedfs/weed/operation" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/security" "github.com/seaweedfs/seaweedfs/weed/stats" "github.com/seaweedfs/seaweedfs/weed/util" ) @@ -28,10 +29,9 @@ func (fs *FilerServer) tusHandler(w http.ResponseWriter, r *http.Request) { // OPTIONS is capability discovery only and carries no data, so it is left // unauthenticated like the main filer OPTIONS handler. Every other TUS method - // mutates or reads the filer namespace and must pass the same JWT check the - // rest of the filer API enforces: the TUS routes are otherwise only wrapped by - // filerGuard.WhiteList, which is a no-op for the filer, so without this they - // bypass jwt.filer_signing entirely. + // authenticates the credential first, then authorizes the server-stored + // TargetPath below, once routing has resolved which resource it acts on. + var claims *security.SeaweedFilerClaims if r.Method != http.MethodOptions { tusVersion := r.Header.Get("Tus-Resumable") if tusVersion != TusVersion { @@ -39,7 +39,8 @@ func (fs *FilerServer) tusHandler(w http.ResponseWriter, r *http.Request) { return } - if !fs.checkTusJwtAuthorization(r) { + var authenticated bool + if claims, authenticated = fs.authenticateFilerJwt(r, r.Method != http.MethodHead); !authenticated { writeJsonError(w, r, http.StatusUnauthorized, errors.New("wrong jwt")) return } @@ -53,17 +54,51 @@ 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 := fs.tusUploadID(reqPath) + // Session ids this server mints are canonical UUIDs. Rejecting aliases + // (a trailing path or any non-canonical spelling) keeps one URL bound to + // one stored authorization resource. + uploadID := strings.TrimPrefix(reqPath, uploadsPrefix) + if !isCanonicalTusUploadID(uploadID) { + writeTusSessionNotFound(w, r.Method) + return + } + + switch r.Method { + case http.MethodHead, http.MethodPatch, http.MethodDelete: + default: + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + + ctx := r.Context() + if r.Method == http.MethodPatch { + ctx = context.WithoutCancel(ctx) + } + session, err := fs.readTusSessionInfo(ctx, uploadID) + if err != nil { + // A transient filer error resolves to "not found"; log it so it is + // distinguishable from a genuinely missing session. + glog.V(1).Infof("TUS session %s not resolved: %v", uploadID, err) + writeTusSessionNotFound(w, r.Method) + return + } + if !authorizeFilerJwtPaths(r, claims, []string{session.TargetPath}) { + writeJsonError(w, r, http.StatusUnauthorized, errors.New("wrong jwt")) + return + } + if err := fs.loadTusSessionChunks(ctx, session); err != nil { + glog.Errorf("Failed to load TUS session %s chunks: %v", uploadID, err) + writeTusSessionNotFound(w, r.Method) + return + } switch r.Method { case http.MethodHead: - fs.tusHeadHandler(w, r, uploadID) + fs.tusHeadHandler(w, session) case http.MethodPatch: - fs.tusPatchHandler(w, r, uploadID) + fs.tusPatchHandler(w, r, session) case http.MethodDelete: - fs.tusDeleteHandler(w, r, uploadID) - default: - w.WriteHeader(http.StatusMethodNotAllowed) + fs.tusDeleteHandler(w, r, session) } return } @@ -73,60 +108,31 @@ func (fs *FilerServer) tusHandler(w http.ResponseWriter, r *http.Request) { case http.MethodOptions: fs.tusOptionsHandler(w, r) case http.MethodPost: + if !authorizeFilerJwtPaths(r, claims, []string{fs.tusTargetPath(r)}) { + writeJsonError(w, r, http.StatusUnauthorized, errors.New("wrong jwt")) + return + } fs.tusCreateHandler(w, r) default: w.WriteHeader(http.StatusMethodNotAllowed) } } -// checkTusJwtAuthorization enforces filer JWT authorization for a TUS request. -// HEAD is a read; POST/PATCH/DELETE are writes. -func (fs *FilerServer) checkTusJwtAuthorization(r *http.Request) bool { - isWrite := r.Method != http.MethodHead - return fs.checkJwtAuthorization(r, isWrite, func() ([]string, error) { - return fs.tusScopedPaths(r) - }) +// writeTusSessionNotFound answers a request whose session cannot be resolved. +// DELETE is idempotent and returns 204 for a missing session; other verbs 404. +func writeTusSessionNotFound(w http.ResponseWriter, method string) { + if method == http.MethodDelete { + w.WriteHeader(http.StatusNoContent) + return + } + http.Error(w, "Upload not found", http.StatusNotFound) } -// 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 != "/" { - 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 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] +// isCanonicalTusUploadID reports whether uploadID is a canonical UUID, the only +// form this server mints, so an aliased or crafted id cannot address a session. +func isCanonicalTusUploadID(uploadID string) bool { + id, err := uuid.Parse(uploadID) + return err == nil && id.String() == uploadID } // tusTargetPath resolves the filer path a TUS create request targets from the @@ -138,7 +144,17 @@ func (fs *FilerServer) tusTargetPath(r *http.Request) string { if target != "" && !strings.HasPrefix(target, "/") { target = "/" + target } - return target + return canonicalTusTargetPath(target) +} + +// canonicalTusTargetPath normalises an absolute filer target, or returns "" when +// the input is empty or not absolute, so authorization and the final write agree +// on one path. +func canonicalTusTargetPath(target string) string { + if target == "" || !strings.HasPrefix(target, "/") { + return "" + } + return path.Clean(target) } // writeTusCompleteError maps a completeTusUpload failure to the same HTTP status @@ -251,9 +267,8 @@ func (fs *FilerServer) tusCreateHandler(w http.ResponseWriter, r *http.Request) // Check if upload is complete if bytesWritten == session.Size { - // Refresh session to get updated chunks - session, err = fs.getTusSession(ctx, uploadID) - if err != nil { + // Ensure the pinned session still exists, then refresh its chunks. + if err = fs.refreshTusSessionChunks(ctx, session); err != nil { glog.Errorf("Failed to get updated TUS session: %v", err) http.Error(w, "Failed to complete upload", http.StatusInternalServerError) return @@ -273,15 +288,7 @@ func (fs *FilerServer) tusCreateHandler(w http.ResponseWriter, r *http.Request) } // tusHeadHandler handles HEAD requests to get current upload offset -func (fs *FilerServer) tusHeadHandler(w http.ResponseWriter, r *http.Request, uploadID string) { - ctx := r.Context() - - session, err := fs.getTusSession(ctx, uploadID) - if err != nil { - http.Error(w, "Upload not found", http.StatusNotFound) - return - } - +func (fs *FilerServer) tusHeadHandler(w http.ResponseWriter, session *TusSession) { w.Header().Set("Upload-Offset", strconv.FormatInt(session.Offset, 10)) w.Header().Set("Upload-Length", strconv.FormatInt(session.Size, 10)) w.Header().Set("Cache-Control", "no-store") @@ -289,7 +296,7 @@ func (fs *FilerServer) tusHeadHandler(w http.ResponseWriter, r *http.Request, up } // tusPatchHandler handles PATCH requests to upload data -func (fs *FilerServer) tusPatchHandler(w http.ResponseWriter, r *http.Request, uploadID string) { +func (fs *FilerServer) tusPatchHandler(w http.ResponseWriter, r *http.Request, session *TusSession) { // Use a context that ignores cancellation from the request context. // The filer's connection has an inactivity timeout: after the request body is fully read, // internal operations (assigning file IDs, uploading to volume servers, completing uploads) @@ -303,13 +310,6 @@ func (fs *FilerServer) tusPatchHandler(w http.ResponseWriter, r *http.Request, u return } - // Get current session - session, err := fs.getTusSession(ctx, uploadID) - if err != nil { - http.Error(w, "Upload not found", http.StatusNotFound) - return - } - // Validate Upload-Offset header uploadOffsetStr := r.Header.Get("Upload-Offset") if uploadOffsetStr == "" { @@ -350,9 +350,8 @@ func (fs *FilerServer) tusPatchHandler(w http.ResponseWriter, r *http.Request, u // Check if upload is complete if newOffset == session.Size { - // Refresh session to get updated chunks - session, err = fs.getTusSession(ctx, uploadID) - if err != nil { + // Ensure the authorized session still exists, then refresh its chunks. + if err = fs.refreshTusSessionChunks(ctx, session); err != nil { glog.Errorf("Failed to get updated TUS session: %v", err) http.Error(w, "Failed to complete upload", http.StatusInternalServerError) return @@ -370,10 +369,10 @@ func (fs *FilerServer) tusPatchHandler(w http.ResponseWriter, r *http.Request, u } // tusDeleteHandler handles DELETE requests to cancel uploads -func (fs *FilerServer) tusDeleteHandler(w http.ResponseWriter, r *http.Request, uploadID string) { +func (fs *FilerServer) tusDeleteHandler(w http.ResponseWriter, r *http.Request, session *TusSession) { ctx := r.Context() - if err := fs.deleteTusSession(ctx, uploadID); err != nil { + if err := fs.deleteTusSession(ctx, session.ID); err != nil { glog.Errorf("Failed to delete TUS session: %v", err) http.Error(w, "Failed to delete upload", http.StatusInternalServerError) return diff --git a/weed/server/filer_server_tus_idor_test.go b/weed/server/filer_server_tus_idor_test.go index c8ee40c09..aa4ae5806 100644 --- a/weed/server/filer_server_tus_idor_test.go +++ b/weed/server/filer_server_tus_idor_test.go @@ -5,128 +5,110 @@ import ( "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) - +// TestFilerServer_tusHandler_CrossPrefixSessionHijack reproduces +// GHSA-99q7-x53r-6j4g: a prefix-restricted token acting on another tenant's TUS +// session (HEAD/PATCH/DELETE) must be scoped against the session's stored +// TargetPath, not authorized on signature and method alone. The victim's session +// must survive a denied mutation. +func TestFilerServer_tusHandler_CrossPrefixSessionHijack(t *testing.T) { tests := []struct { - name string - method string - path string - token string - expectAuthorized bool + name string + method string + prefix string + expectStatus int + expectExists 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}, + {"cross-prefix HEAD denied", http.MethodHead, "/buckets/allowed", http.StatusUnauthorized, true}, + {"matching-prefix HEAD allowed", http.MethodHead, "/buckets/secret", http.StatusOK, true}, + {"cross-prefix PATCH denied", http.MethodPatch, "/buckets/allowed", http.StatusUnauthorized, true}, + {"matching-prefix PATCH allowed", http.MethodPatch, "/buckets/secret", http.StatusNoContent, true}, + {"cross-prefix DELETE denied", http.MethodDelete, "/buckets/allowed", http.StatusUnauthorized, true}, + {"matching-prefix DELETE allowed", http.MethodDelete, "/buckets/secret", http.StatusNoContent, 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) + fs, store := newTusTestServer(t, map[string]string{tusTestUploadID: "/buckets/secret/victim.bin"}) + + signingKey := tusTestWriteKey + if tt.method == http.MethodHead { + signingKey = tusTestReadKey + } + token := signFilerToken(t, signingKey, []string{tt.prefix}, nil) + req := httptest.NewRequest(tt.method, "/.tus/.uploads/"+tusTestUploadID, http.NoBody) + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Tus-Resumable", TusVersion) + if tt.method == http.MethodPatch { + req.Header.Set("Content-Type", "application/offset+octet-stream") + req.Header.Set("Upload-Offset", "0") + } + rec := httptest.NewRecorder() + + fs.tusHandler(rec, req) + + if rec.Code != tt.expectStatus { + t.Fatalf("%s status = %d, want %d; body=%q", tt.method, rec.Code, tt.expectStatus, rec.Body.String()) + } + _, err := store.FindEntry(context.Background(), util.FullPath(fs.tusSessionInfoPath(tusTestUploadID))) + if tt.expectExists && err != nil { + t.Fatalf("session removed after %s: %v", tt.method, err) + } + if !tt.expectExists && err == nil { + t.Fatalf("session still present after authorized %s", tt.method) } }) } } -// 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" +// TestFilerServer_tusHandler_RejectsAliasesAndInvalidMetadata covers the routing +// and metadata guards: a non-canonical or aliased upload id is rejected before +// any lookup, and a session whose stored id, target or size is unusable resolves +// to "not found" rather than being authorized or acted upon. +func TestFilerServer_tusHandler_RejectsAliasesAndInvalidMetadata(t *testing.T) { + tests := []struct { + name string + routeID string + stored TusSession + }{ + {"trailing path alias", tusTestUploadID + "/extra", TusSession{ID: tusTestUploadID, TargetPath: "/buckets/secret/victim.bin", Size: 1}}, + {"non-canonical route id", "not-a-uuid", TusSession{ID: tusTestUploadID, TargetPath: "/buckets/secret/victim.bin", Size: 1}}, + {"stored id mismatch", tusTestUploadID, TusSession{ID: "00000000-0000-0000-0000-000000000000", TargetPath: "/buckets/secret/victim.bin", Size: 1}}, + {"empty stored target", tusTestUploadID, TusSession{ID: tusTestUploadID, TargetPath: "", Size: 1}}, + {"root stored target", tusTestUploadID, TusSession{ID: tusTestUploadID, TargetPath: "/", Size: 1}}, + {"relative stored target", tusTestUploadID, TusSession{ID: tusTestUploadID, TargetPath: "buckets/secret/x.bin", Size: 1}}, + {"oversize stored size", tusTestUploadID, TusSession{ID: tusTestUploadID, TargetPath: "/buckets/secret/victim.bin", Size: TusMaxSize + 1}}, + } - fs, _ := newTusIDORTestServer(t, writeKey, readKey, map[string]string{ - "victim-session": "/buckets/secret/victim.bin", - }) - attacker := signFilerToken(t, writeKey, []string{"/buckets/allowed"}, nil) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fs, store := newTusTestServer(t, nil) + data, err := json.Marshal(&tt.stored) + if err != nil { + t.Fatalf("marshal session: %v", err) + } + if err := store.InsertEntry(context.Background(), &filer.Entry{ + FullPath: util.FullPath(fs.tusSessionInfoPath(tusTestUploadID)), + Content: data, + }); err != nil { + t.Fatalf("seed session: %v", err) + } - 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) + req := httptest.NewRequest(http.MethodHead, "/.tus/.uploads/"+tt.routeID, nil) + req.Header.Set("Authorization", "Bearer "+signFilerToken(t, tusTestReadKey, nil, nil)) + req.Header.Set("Tus-Resumable", TusVersion) + rec := httptest.NewRecorder() - rec := httptest.NewRecorder() - fs.tusHandler(rec, req) + fs.tusHandler(rec, req) - if rec.Code != http.StatusUnauthorized { - t.Errorf("cross-prefix PATCH = %d, want %d", rec.Code, http.StatusUnauthorized) + if rec.Code != http.StatusNotFound { + t.Fatalf("HEAD %s = %d, want %d; body=%q", tt.routeID, rec.Code, http.StatusNotFound, rec.Body.String()) + } + }) } } diff --git a/weed/server/filer_server_tus_jwt_test.go b/weed/server/filer_server_tus_jwt_test.go index 53a6f49de..1d77e20b5 100644 --- a/weed/server/filer_server_tus_jwt_test.go +++ b/weed/server/filer_server_tus_jwt_test.go @@ -1,13 +1,24 @@ package weed_server import ( + "context" + "encoding/json" "net/http" "net/http/httptest" + "os" "testing" "time" "github.com/golang-jwt/jwt/v5" + "github.com/seaweedfs/seaweedfs/weed/filer" "github.com/seaweedfs/seaweedfs/weed/security" + "github.com/seaweedfs/seaweedfs/weed/util" +) + +const ( + tusTestWriteKey = "write-secret" + tusTestReadKey = "read-secret" + tusTestUploadID = "9f6f0d4b-6556-48f6-b953-0d8fca1966f1" ) // signFilerToken builds a signed filer JWT for tests. @@ -27,46 +38,79 @@ func signFilerToken(t *testing.T, signingKey string, allowedPrefixes, allowedMet return str } -func TestFilerServer_checkTusJwtAuthorization(t *testing.T) { - const writeKey = "write-secret" - const readKey = "read-secret" +// newTusTestServer builds a FilerServer backed by an in-memory store seeded with +// TUS sessions (uploadID -> stored TargetPath), so the handler can resolve a +// session's target the way production does. The store is returned so a test can +// mutate or inspect the seeded session. +func newTusTestServer(t *testing.T, sessions map[string]string) (*FilerServer, *renameTestStore) { + t.Helper() + store := newRenameTestStore() fs := &FilerServer{ - filerGuard: security.NewGuard(nil, writeKey, 0, readKey, 0), + filer: newRenameTestFiler(store), + filerGuard: security.NewGuard(nil, tusTestWriteKey, 0, tusTestReadKey, 0), option: &FilerOption{TusBasePath: "/.tus"}, } + for uploadID, targetPath := range sessions { + seedTusSession(t, fs, store, TusSession{ID: uploadID, TargetPath: targetPath, Size: 1}) + } + return fs, store +} + +// seedTusSession writes a session directory and its .info metadata into the store. +func seedTusSession(t *testing.T, fs *FilerServer, store *renameTestStore, session TusSession) { + t.Helper() + data, err := json.Marshal(&session) + if err != nil { + t.Fatalf("marshal session %s: %v", session.ID, err) + } + dir := &filer.Entry{FullPath: util.FullPath(fs.tusSessionPath(session.ID)), Attr: filer.Attr{Mode: os.ModeDir | 0755}} + info := &filer.Entry{FullPath: util.FullPath(fs.tusSessionInfoPath(session.ID)), Content: data} + for _, entry := range []*filer.Entry{dir, info} { + if err := store.InsertEntry(context.Background(), entry); err != nil { + t.Fatalf("seed session %s: %v", session.ID, err) + } + } +} + +func TestFilerServer_checkJwtAuthorization(t *testing.T) { + fs := &FilerServer{ + filerGuard: security.NewGuard(nil, tusTestWriteKey, 0, tusTestReadKey, 0), + option: &FilerOption{TusBasePath: "/.tus"}, + } + victim := []string{"/buckets/secret/victim.bin"} + uploadPath := "/.tus/.uploads/" + tusTestUploadID tests := []struct { name string method string path string token string + scopedPaths []string expectAuthorized bool }{ - // The advisory: with a filer signing key configured, an unauthenticated - // TUS request must be rejected the same as a normal filer write. - {"create without token denied", http.MethodPost, "/.tus/buckets/secret/owned.txt", "", false}, - {"patch without token denied", http.MethodPatch, "/.tus/.uploads/abc", "", false}, - {"delete without token denied", http.MethodDelete, "/.tus/.uploads/abc", "", false}, - {"head without token denied", http.MethodHead, "/.tus/.uploads/abc", "", false}, + // With a filer signing key configured, an unauthenticated request is denied. + {"post without token denied", http.MethodPost, "/.tus/buckets/secret/owned.txt", "", []string{"/buckets/secret/owned.txt"}, false}, + {"patch without token denied", http.MethodPatch, uploadPath, "", victim, false}, + {"head without token denied", http.MethodHead, uploadPath, "", victim, false}, // A valid token for the right access level is accepted. - {"create with write token allowed", http.MethodPost, "/.tus/buckets/data/ok.txt", signFilerToken(t, writeKey, nil, nil), true}, - {"patch with write token allowed", http.MethodPatch, "/.tus/.uploads/abc", signFilerToken(t, writeKey, nil, nil), true}, - {"head with read token allowed", http.MethodHead, "/.tus/.uploads/abc", signFilerToken(t, readKey, nil, nil), true}, + {"post write token allowed", http.MethodPost, "/.tus/buckets/data/ok.txt", signFilerToken(t, tusTestWriteKey, nil, nil), []string{"/buckets/data/ok.txt"}, true}, + {"head read token allowed", http.MethodHead, uploadPath, signFilerToken(t, tusTestReadKey, nil, nil), victim, true}, - // HEAD is a read, so a write-only token must not authorize it and a read - // token must not authorize a write. - {"head with write token denied", http.MethodHead, "/.tus/.uploads/abc", signFilerToken(t, writeKey, nil, nil), false}, - {"create with read token denied", http.MethodPost, "/.tus/buckets/data/ok.txt", signFilerToken(t, readKey, nil, nil), false}, + // HEAD is a read, so a write token must not authorize it, nor a read token a write. + {"head write token denied", http.MethodHead, uploadPath, signFilerToken(t, tusTestWriteKey, nil, nil), victim, false}, + {"post read token denied", http.MethodPost, "/.tus/buckets/data/ok.txt", signFilerToken(t, tusTestReadKey, nil, nil), []string{"/buckets/data/ok.txt"}, false}, - // Prefix-restricted tokens are scoped against the resolved target path - // (URL minus the /.tus prefix), not the /.tus route. - {"create within allowed prefix", http.MethodPost, "/.tus/buckets/allowed/ok.txt", signFilerToken(t, writeKey, []string{"/buckets/allowed"}, nil), true}, - {"create outside allowed prefix denied", http.MethodPost, "/.tus/buckets/secret/owned.txt", signFilerToken(t, writeKey, []string{"/buckets/allowed"}, nil), false}, + // Prefix-restricted tokens are scoped against the resolved target path. + {"within allowed prefix", http.MethodPost, "/.tus/buckets/allowed/ok.txt", signFilerToken(t, tusTestWriteKey, []string{"/buckets/allowed"}, nil), []string{"/buckets/allowed/ok.txt"}, true}, + {"outside allowed prefix denied", http.MethodPost, "/.tus/buckets/secret/owned.txt", signFilerToken(t, tusTestWriteKey, []string{"/buckets/allowed"}, nil), []string{"/buckets/secret/owned.txt"}, false}, + + // Fail closed: a prefix-restricted token with no resolved resource is denied. + {"restricted token without resource denied", http.MethodPatch, uploadPath, signFilerToken(t, tusTestWriteKey, []string{"/buckets/allowed"}, nil), nil, false}, // Method-restricted tokens are checked against the actual HTTP method. - {"patch with matching method allowed", http.MethodPatch, "/.tus/.uploads/abc", signFilerToken(t, writeKey, nil, []string{"POST", "PATCH", "DELETE"}), true}, - {"patch outside allowed methods denied", http.MethodPatch, "/.tus/.uploads/abc", signFilerToken(t, writeKey, nil, []string{"POST"}), false}, + {"matching method allowed", http.MethodPatch, uploadPath, signFilerToken(t, tusTestWriteKey, nil, []string{"POST", "PATCH", "DELETE"}), victim, true}, + {"method not allowed denied", http.MethodPatch, uploadPath, signFilerToken(t, tusTestWriteKey, nil, []string{"POST"}), victim, false}, } for _, tt := range tests { @@ -75,21 +119,18 @@ func TestFilerServer_checkTusJwtAuthorization(t *testing.T) { if tt.token != "" { 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) + if got := fs.checkJwtAuthorization(req, tt.method != http.MethodHead, tt.scopedPaths); got != tt.expectAuthorized { + t.Errorf("checkJwtAuthorization(%s %s) = %v, want %v", tt.method, tt.path, got, tt.expectAuthorized) } }) } } // TestFilerServer_tusHandler_UnauthenticatedRejected exercises the full handler -// entry point: OPTIONS discovery stays open, but an unauthenticated write is -// rejected with 401 before any session is created. +// entry point: OPTIONS discovery stays open, but an unauthenticated request is +// rejected with 401 before any session is created or looked up. func TestFilerServer_tusHandler_UnauthenticatedRejected(t *testing.T) { - fs := &FilerServer{ - filerGuard: security.NewGuard(nil, "write-secret", 0, "read-secret", 0), - option: &FilerOption{TusBasePath: "/.tus"}, - } + fs, _ := newTusTestServer(t, map[string]string{tusTestUploadID: "/buckets/secret/victim.bin"}) // OPTIONS is capability discovery and must not require a token. optionsReq := httptest.NewRequest(http.MethodOptions, "/.tus/buckets/secret/owned.txt", nil) @@ -108,4 +149,64 @@ func TestFilerServer_tusHandler_UnauthenticatedRejected(t *testing.T) { if postRec.Code != http.StatusUnauthorized { t.Errorf("unauthenticated POST = %d, want %d", postRec.Code, http.StatusUnauthorized) } + + // A missing credential is rejected before the upload id triggers a metadata + // lookup, so an existing session is not an unauthenticated resource oracle. + headReq := httptest.NewRequest(http.MethodHead, "/.tus/.uploads/"+tusTestUploadID, nil) + headReq.Header.Set("Tus-Resumable", TusVersion) + headRec := httptest.NewRecorder() + fs.tusHandler(headRec, headReq) + if headRec.Code != http.StatusUnauthorized { + t.Errorf("unauthenticated HEAD = %d, want %d", headRec.Code, http.StatusUnauthorized) + } +} + +// TestFilerServer_refreshTusSessionChunks_RevalidatesPinnedSession verifies a +// pinned session cannot complete after it is deleted or its target is replaced +// between authorization and completion. +func TestFilerServer_refreshTusSessionChunks_RevalidatesPinnedSession(t *testing.T) { + pin := func(t *testing.T) (*FilerServer, *renameTestStore, *TusSession) { + t.Helper() + fs, store := newTusTestServer(t, nil) + seedTusSession(t, fs, store, TusSession{ + ID: tusTestUploadID, + TargetPath: "/buckets/secret/victim.bin", + Size: 1, + CreatedAt: time.Unix(1700000000, 123), + }) + session, err := fs.readTusSessionInfo(context.Background(), tusTestUploadID) + if err != nil { + t.Fatalf("pin session: %v", err) + } + return fs, store, session + } + + t.Run("deleted before completion", func(t *testing.T) { + fs, store, session := pin(t) + if err := store.DeleteEntry(context.Background(), util.FullPath(fs.tusSessionInfoPath(tusTestUploadID))); err != nil { + t.Fatalf("delete session info: %v", err) + } + if err := fs.refreshTusSessionChunks(context.Background(), session); err == nil { + t.Fatal("refresh succeeded after the session was deleted") + } + }) + + t.Run("replaced before completion", func(t *testing.T) { + fs, store, session := pin(t) + replaced := *session + replaced.TargetPath = "/buckets/other/replacement.bin" + data, err := json.Marshal(&replaced) + if err != nil { + t.Fatalf("marshal replacement: %v", err) + } + if err := store.InsertEntry(context.Background(), &filer.Entry{ + FullPath: util.FullPath(fs.tusSessionInfoPath(tusTestUploadID)), + Content: data, + }); err != nil { + t.Fatalf("replace session info: %v", err) + } + if err := fs.refreshTusSessionChunks(context.Background(), session); err == nil { + t.Fatal("refresh succeeded after the session target was replaced") + } + }) } diff --git a/weed/server/filer_server_tus_session.go b/weed/server/filer_server_tus_session.go index 5cfd705e5..4bc838466 100644 --- a/weed/server/filer_server_tus_session.go +++ b/weed/server/filer_server_tus_session.go @@ -181,9 +181,14 @@ func (fs *FilerServer) saveTusSession(ctx context.Context, session *TusSession) return nil } -// readTusSessionInfo reads and decodes a session's .info file without listing its -// chunks, the cheap lookup the authorization check needs for the stored TargetPath. +// readTusSessionInfo reads and validates a session's immutable .info metadata +// without listing its chunks, the cheap lookup the authorization check needs for +// the stored TargetPath. It rejects a metadata file whose id, target or size is +// unusable so a corrupt or replaced session cannot be authorized or completed. func (fs *FilerServer) readTusSessionInfo(ctx context.Context, uploadID string) (*TusSession, error) { + if !isCanonicalTusUploadID(uploadID) { + return nil, fmt.Errorf("invalid TUS upload id: %q", uploadID) + } infoPath := util.FullPath(fs.tusSessionInfoPath(uploadID)) entry, err := fs.filer.FindEntry(ctx, infoPath) if err != nil { @@ -197,18 +202,39 @@ func (fs *FilerServer) readTusSessionInfo(ctx context.Context, uploadID string) if err := json.Unmarshal(entry.Content, &session); err != nil { return nil, fmt.Errorf("unmarshal session: %w", err) } + if session.ID != uploadID { + return nil, fmt.Errorf("TUS session id mismatch: got %q, want %q", session.ID, uploadID) + } + target := canonicalTusTargetPath(session.TargetPath) + if target == "" || target == "/" { + return nil, fmt.Errorf("invalid TUS target path: %q", session.TargetPath) + } + if session.Size < 0 || session.Size > TusMaxSize { + return nil, fmt.Errorf("invalid TUS upload size: %d", session.Size) + } + // Pin authorization and every later operation to the same canonical path. + session.TargetPath = target return &session, nil } -// getTusSession retrieves a TUS session by upload ID, including chunks from directory listing +// getTusSession retrieves a validated TUS session by upload ID, including its +// chunks and current offset. func (fs *FilerServer) getTusSession(ctx context.Context, uploadID string) (*TusSession, error) { session, err := fs.readTusSessionInfo(ctx, uploadID) if err != nil { return nil, err } + if err := fs.loadTusSessionChunks(ctx, session); err != nil { + return nil, err + } + return session, nil +} +// loadTusSessionChunks refreshes a session's chunks and offset from its session +// directory, leaving the immutable .info metadata untouched. +func (fs *FilerServer) loadTusSessionChunks(ctx context.Context, session *TusSession) error { // Load chunks from directory listing with pagination (atomic read, no race condition) - sessionDirPath := util.FullPath(fs.tusSessionPath(uploadID)) + sessionDirPath := util.FullPath(fs.tusSessionPath(session.ID)) session.Chunks = nil session.Offset = 0 @@ -217,7 +243,7 @@ func (fs *FilerServer) getTusSession(ctx context.Context, uploadID string) (*Tus for { entries, hasMore, err := fs.filer.ListDirectoryEntries(ctx, sessionDirPath, lastFileName, false, int64(pageSize), "", "", "") if err != nil { - return nil, fmt.Errorf("list session directory: %w", err) + return fmt.Errorf("list session directory: %w", err) } for _, e := range entries { @@ -258,7 +284,22 @@ func (fs *FilerServer) getTusSession(ctx context.Context, uploadID string) (*Tus session.Offset = contiguousEnd } - return session, nil + return nil +} + +// refreshTusSessionChunks verifies the pinned session still exists and still +// identifies the same upload before refreshing its chunk state, so a PATCH +// cannot complete after a concurrent DELETE or metadata replacement and land at +// a TargetPath other than the one that was authorized. +func (fs *FilerServer) refreshTusSessionChunks(ctx context.Context, session *TusSession) error { + stored, err := fs.readTusSessionInfo(ctx, session.ID) + if err != nil { + return err + } + if stored.TargetPath != session.TargetPath || stored.Size != session.Size || !stored.CreatedAt.Equal(session.CreatedAt) { + return fmt.Errorf("TUS session identity changed: %s", session.ID) + } + return fs.loadTusSessionChunks(ctx, session) } // saveTusChunk stores the chunk info as a separate file entry