From 533f5060b932ea6d0379f292b8931c320adfbd5c Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 1 Nov 2026 18:36:31 -0800 Subject: [PATCH] filer: scope a prefix JWT to the directory a zip extract writes under (#12305) * filer: scope a prefix JWT to the directory a zip extract writes under Without a trailing slash, an extract request writes members beside the named path, in its parent directory, while the JWT check only saw the request path. A token scoped to /tenant1 could therefore extract into / by posting to /tenant1?extract=zip. Add the extract root to the paths a prefix-restricted token must cover. Co-Authored-By: Claude Opus 5.5 * filer: do not keep an extracted archive under the name ".." A multipart file name of ".." passes path.Base unchanged, so extract.keep stored the archive as an entry literally named ".." in the target directory, a name that local sinks resolve to the parent. Co-Authored-By: Claude Opus 5.5 * leveldb3: keep a non-local bucket name out of the per-bucket folder findDB splits the bucket out of a /buckets/ path and uses it as a folder name under the store directory. The name cannot hold a slash and dot names already go to the default DB, so this only rejects an empty or non-local name, but it also lets static analysis see the folder cannot escape the store directory. * filer: clean the zip extract root before scoping and writing A percent-encoded dot segment survives the path cleaning in front of the mux, so a request path ending in "/" reached extractZipRoot with ".." in it. The JWT scope check cleans the path it compares, but the members were written under the raw root, so the two could disagree. * leveldb3: apply the bucket folder check to bucket create and delete events findDB already keeps dot and non-local bucket names out of the per-bucket folders, but OnBucketCreation and OnBucketDeletion used the name as is. A directory event for /buckets/.. opened a DB in the parent of the store directory, and its delete removed that parent. Co-Authored-By: Claude Opus 5.5 * leveldb3: keep a bucket delete event off the default DB A bucket named _main maps to the default DB in findDB, so deleting /buckets/_main closed the default DB and removed its folder, taking every non-bucket entry with it. Co-Authored-By: Claude Opus 5.5 * leveldb3: keep bucket names with a path separator out of the store dir A bucket event named x/../_main passed the local-name check, and its delete removed the default DB folder once store.dir/x existed. Co-Authored-By: Claude Opus 5.5 * leveldb3: only reject the platform separator in a bucket name A backslash is an ordinary file name character on Unix, and a bucket folder named that way already exists for anyone who created one. Rejecting it on every platform would route those entries to the default DB after an upgrade. The slash can never appear in a bucket name here, and on Windows the backslash is the separator, so rejecting os.PathSeparator alongside the slash keeps the folder flat without changing Unix routing. * leveldb3: check for an escaped bucket db before its delete event --------- Co-authored-by: Claude Opus 5.5 --- weed/filer/leveldb3/leveldb3_store.go | 13 +++- weed/filer/leveldb3/leveldb3_store_bucket.go | 10 ++- weed/filer/leveldb3/leveldb3_store_test.go | 43 +++++++++++++ weed/server/filer_jwt_test.go | 61 +++++++++++++++++++ weed/server/filer_server_handlers.go | 8 ++- weed/server/filer_server_handlers_extract.go | 19 ++++-- .../filer_server_handlers_extract_test.go | 44 +++++++++++++ 7 files changed, 184 insertions(+), 14 deletions(-) diff --git a/weed/filer/leveldb3/leveldb3_store.go b/weed/filer/leveldb3/leveldb3_store.go index 3111e2a5ad..5484383713 100644 --- a/weed/filer/leveldb3/leveldb3_store.go +++ b/weed/filer/leveldb3/leveldb3_store.go @@ -8,6 +8,7 @@ import ( "io" "math" "os" + "path/filepath" "strings" "sync" @@ -167,9 +168,7 @@ func (store *LevelDB3Store) findDB(fullpath weed_util.FullPath, isForChildren bo shortPath = weed_util.FullPath(bucketAndObjectKey[t:]) } - // Dot-prefixed entries directly under /buckets (e.g. .system) are internal - // folders, not S3 buckets; keep them in the default DB by full path. - if strings.HasPrefix(bucket, ".") { + if !isBucketFolder(bucket) { store.dbsLock.RUnlock() return defaultDB, DEFAULT, fullpath, nil } @@ -207,6 +206,14 @@ func (store *LevelDB3Store) emptyBucketDB() (*leveldb.DB, error) { return store.emptyDB, nil } +// Dot-prefixed entries directly under /buckets (e.g. .system) are internal +// folders, not S3 buckets; they stay in the default DB by full path. A bucket +// also names a single folder under store.dir, so it cannot hold a separator; +// "\" is an ordinary name character on Unix and only rejected where it separates. +func isBucketFolder(bucket string) bool { + return !strings.HasPrefix(bucket, ".") && filepath.IsLocal(bucket) && !strings.ContainsAny(bucket, "/"+string(os.PathSeparator)) +} + func (store *LevelDB3Store) createDB(bucket string) (*leveldb.DB, error) { store.dbsLock.Lock() diff --git a/weed/filer/leveldb3/leveldb3_store_bucket.go b/weed/filer/leveldb3/leveldb3_store_bucket.go index feb2a4b049..2b56cad6ca 100644 --- a/weed/filer/leveldb3/leveldb3_store_bucket.go +++ b/weed/filer/leveldb3/leveldb3_store_bucket.go @@ -9,14 +9,18 @@ import ( var _ filer.BucketAware = (*LevelDB3Store)(nil) func (store *LevelDB3Store) OnBucketCreation(bucket string) { + if !isBucketFolder(bucket) { + return + } store.createDB(bucket) } func (store *LevelDB3Store) OnBucketDeletion(bucket string) { - store.closeDB(bucket) - if bucket != "" { // just to make sure - os.RemoveAll(store.dir + "/" + bucket) + if !isBucketFolder(bucket) || bucket == DEFAULT { + return } + store.closeDB(bucket) + os.RemoveAll(store.dir + "/" + bucket) } func (store *LevelDB3Store) CanDropWholeBucket() bool { diff --git a/weed/filer/leveldb3/leveldb3_store_test.go b/weed/filer/leveldb3/leveldb3_store_test.go index a9f3be10da..1f57acd4c1 100644 --- a/weed/filer/leveldb3/leveldb3_store_test.go +++ b/weed/filer/leveldb3/leveldb3_store_test.go @@ -3,6 +3,8 @@ package leveldb import ( "context" "math" + "os" + "path/filepath" "strings" "testing" @@ -207,3 +209,44 @@ func TestListDirectoryPrefixedEntries_StartBelowPrefix(t *testing.T) { t.Fatalf("want [item-b.versions], got %v", names) } } + +func TestBucketEventsStayInsideStoreDir(t *testing.T) { + parent := t.TempDir() + dir := filepath.Join(parent, "store") + store := &LevelDB3Store{} + if err := store.initialize(dir); err != nil { + t.Fatal(err) + } + defer store.Shutdown() + + sentinel := filepath.Join(parent, "keep") + if err := os.WriteFile(sentinel, []byte("keep"), 0644); err != nil { + t.Fatal(err) + } + // Checked after each create, before the matching delete could remove the + // evidence of a db opened in the wrong place. + assertNothingEscaped := func(bucket string) { + t.Helper() + for _, p := range []string{filepath.Join(parent, "outside"), filepath.Join(parent, "CURRENT"), filepath.Join(dir, "x", "y")} { + if _, err := os.Stat(p); !os.IsNotExist(err) { + t.Fatalf("bucket %q event created %s: %v", bucket, p, err) + } + } + } + store.OnBucketCreation("x") + for _, bucket := range []string{"..", ".", "../outside", "", DEFAULT, "x/../" + DEFAULT, "x/y"} { + store.OnBucketCreation(bucket) + assertNothingEscaped(bucket) + store.OnBucketDeletion(bucket) + } + if _, err := os.Stat(sentinel); err != nil { + t.Fatalf("bucket events removed %s: %v", sentinel, err) + } + if _, err := os.Stat(filepath.Join(dir, DEFAULT)); err != nil { + t.Fatalf("default db folder missing: %v", err) + } + ctx := context.Background() + if err := store.InsertEntry(ctx, &filer.Entry{FullPath: "/a/b"}); err != nil { + t.Fatalf("default db unusable after bucket events: %v", err) + } +} diff --git a/weed/server/filer_jwt_test.go b/weed/server/filer_jwt_test.go index 74f5bb9bcc..efd4b99c60 100644 --- a/weed/server/filer_jwt_test.go +++ b/weed/server/filer_jwt_test.go @@ -38,6 +38,7 @@ func TestFilerServer_maybeCheckJwtAuthorization_Scoped(t *testing.T) { token string method string path string + header map[string]string isWrite bool expectAuthorized bool }{ @@ -225,6 +226,63 @@ func TestFilerServer_maybeCheckJwtAuthorization_Scoped(t *testing.T) { isWrite: true, expectAuthorized: false, }, + { + name: "write at the prefix itself allowed", + token: genToken([]string{"/tenant1"}, nil), + method: "POST", + path: "/tenant1", + isWrite: true, + expectAuthorized: true, + }, + { + name: "extract into the prefix directory allowed", + token: genToken([]string{"/tenant1"}, nil), + method: "POST", + path: "/tenant1/?extract=zip", + isWrite: true, + expectAuthorized: true, + }, + { + name: "extract beside an archive inside the prefix allowed", + token: genToken([]string{"/tenant1"}, nil), + method: "PUT", + path: "/tenant1/pack.zip?extract=zip", + isWrite: true, + expectAuthorized: true, + }, + { + name: "extract at the prefix itself writes to its parent, denied", + token: genToken([]string{"/tenant1"}, nil), + method: "POST", + path: "/tenant1?extract=zip", + isWrite: true, + expectAuthorized: false, + }, + { + name: "extract header at the prefix itself denied", + token: genToken([]string{"/tenant1/"}, []string{"PUT"}), + method: "PUT", + path: "/tenant1", + header: map[string]string{"X-Seaweedfs-Extract": "zip"}, + isWrite: true, + expectAuthorized: false, + }, + { + name: "extract at a nested prefix into the sibling parent denied", + token: genToken([]string{"/buckets/bucket-a"}, nil), + method: "POST", + path: "/buckets/bucket-a?extract=zip", + isWrite: true, + expectAuthorized: false, + }, + { + name: "extract with a root prefix allowed", + token: genToken([]string{"/"}, nil), + method: "POST", + path: "/tenant1?extract=zip", + isWrite: true, + expectAuthorized: true, + }, } for _, tt := range tests { @@ -233,6 +291,9 @@ func TestFilerServer_maybeCheckJwtAuthorization_Scoped(t *testing.T) { if tt.token != "" { req.Header.Set("Authorization", "Bearer "+tt.token) } + for k, v := range tt.header { + req.Header.Set(k, v) + } if authorized := fs.maybeCheckJwtAuthorization(req, tt.isWrite); authorized != tt.expectAuthorized { t.Errorf("expected authorized=%v, got %v", tt.expectAuthorized, authorized) } diff --git a/weed/server/filer_server_handlers.go b/weed/server/filer_server_handlers.go index 6fd9bfbf3e..4bdd5d87f9 100644 --- a/weed/server/filer_server_handlers.go +++ b/weed/server/filer_server_handlers.go @@ -308,10 +308,14 @@ func authorizeFilerJwtPaths(r *http.Request, claims *security.SeaweedFilerClaims } // jwtScopedRequestPaths returns every filer path a request touches that must be -// covered by the JWT AllowedPrefixes: the write target (r.URL.Path) plus any -// copy/move source named by the cp.from / mv.from query parameters. +// covered by the JWT AllowedPrefixes: the write target (r.URL.Path), the +// directory a zip extract writes under, and any copy/move source named by the +// cp.from / mv.from query parameters. func jwtScopedRequestPaths(r *http.Request) []string { paths := []string{r.URL.Path} + if (r.Method == http.MethodPost || r.Method == http.MethodPut) && isExtractZip(r) { + paths = append(paths, extractZipRoot(r)+"/") + } if query := r.URL.Query(); query.Has("cp.from") || query.Has("mv.from") { if from := query.Get("cp.from"); from != "" { paths = append(paths, from) diff --git a/weed/server/filer_server_handlers_extract.go b/weed/server/filer_server_handlers_extract.go index c51d7b9c6f..97057ee3fc 100644 --- a/weed/server/filer_server_handlers_extract.go +++ b/weed/server/filer_server_handlers_extract.go @@ -35,6 +35,17 @@ func isExtractZip(r *http.Request) bool { r.Header.Get(s3_constants.SeaweedFSExtract) == "zip" } +// extractZipRoot is the directory members land under: the request path when it +// ends in "/", otherwise its parent, so the path can name the archive. It is +// cleaned so the JWT scope check and the writes agree on dot segments. +func extractZipRoot(r *http.Request) string { + root := r.URL.Path + if !strings.HasSuffix(root, "/") { + root = path.Dir(root) + } + return strings.TrimSuffix(path.Clean(root), "/") +} + // extractZipArchive writes a posted archive's members as individual entries // under the target directory, like AWS Snowball's auto-extract. The archive // itself is only stored when ?extract.keep=true. @@ -81,11 +92,7 @@ func (fs *FilerServer) extractZipArchive(ctx context.Context, w http.ResponseWri return } - root := r.URL.Path - if !strings.HasSuffix(root, "/") { - root = path.Dir(root) - } - root = strings.TrimSuffix(root, "/") + root := extractZipRoot(r) query := r.URL.Query() chunkSize := fs.requestChunkSize(query) @@ -153,7 +160,7 @@ func (fs *FilerServer) extractZipArchive(ctx context.Context, w http.ResponseWri } } - if query.Get("extract.keep") == "true" && archiveName != "." && archiveName != "/" { + if query.Get("extract.keep") == "true" && archiveName != "." && archiveName != ".." && archiveName != "/" { mode := os.FileMode(0660) if modeStr := query.Get("mode"); modeStr != "" { if parsed, parseErr := strconv.ParseUint(modeStr, 8, 32); parseErr == nil { diff --git a/weed/server/filer_server_handlers_extract_test.go b/weed/server/filer_server_handlers_extract_test.go index 9461e79e5a..3040a9ad4a 100644 --- a/weed/server/filer_server_handlers_extract_test.go +++ b/weed/server/filer_server_handlers_extract_test.go @@ -168,6 +168,28 @@ func TestExtractZipArchiveKeep(t *testing.T) { } } +func TestExtractZipArchiveKeepSkipsParentName(t *testing.T) { + fs, f := newExtractTestServer(t) + zipBytes := buildTestZip(t, map[string]string{"x.txt": "data"}, nil, nil) + + var body bytes.Buffer + mw := multipartWriter(t, &body, "..", zipBytes) + r := httptest.NewRequest(http.MethodPost, "/a/b/?extract=zip&extract.keep=true", &body) + r.Header.Set("Content-Type", mw) + w := httptest.NewRecorder() + fs.PostHandler(w, r, int64(body.Len())) + + if w.Code != http.StatusCreated { + t.Fatalf("status = %d, body: %s", w.Code, w.Body.String()) + } + if _, err := f.FindEntry(r.Context(), util.FullPath("/a/b/..")); err == nil { + t.Fatal("archive kept under a parent-directory name") + } + if _, err := f.FindEntry(r.Context(), util.FullPath("/a/b/x.txt")); err != nil { + t.Fatalf("member missing: %v", err) + } +} + func TestExtractZipArchiveNotAZip(t *testing.T) { fs, _ := newExtractTestServer(t) r := httptest.NewRequest(http.MethodPut, "/dst/notzip.bin?extract=zip", bytes.NewReader([]byte("not a zip"))) @@ -231,6 +253,28 @@ func TestExtractZipArchiveRawPost(t *testing.T) { } } +func TestExtractZipArchiveCleansDotSegments(t *testing.T) { + fs, f := newExtractTestServer(t) + zipBytes := buildTestZip(t, map[string]string{"raw.txt": "raw"}, nil, nil) + + // A percent-encoded ".." survives path cleaning in front of the mux, so the + // handler sees it decoded and must land members under the cleaned root. + r := httptest.NewRequest(http.MethodPost, "/other/%2e%2e/dst/?extract=zip", bytes.NewReader(zipBytes)) + r.Header.Set("Content-Type", "application/zip") + w := httptest.NewRecorder() + fs.PostHandler(w, r, int64(len(zipBytes))) + + if w.Code != http.StatusCreated { + t.Fatalf("status = %d, body: %s", w.Code, w.Body.String()) + } + if _, err := f.FindEntry(r.Context(), util.FullPath("/dst/raw.txt")); err != nil { + t.Fatalf("member missing under the cleaned root: %v", err) + } + if _, err := f.FindEntry(r.Context(), util.FullPath("/other/../dst/raw.txt")); err == nil { + t.Fatal("member written under the raw root") + } +} + func TestExtractZipArchiveMultipartPost(t *testing.T) { fs, f := newExtractTestServer(t) zipBytes := buildTestZip(t, map[string]string{"img/1.png": "png-bytes"}, nil, nil)