From 2ea63bad488107c4c760eb36b21a3425115fef72 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 1 Nov 2026 18:33:07 -0800 Subject: [PATCH] filer.backup: keep local sink writes inside the sink directory (#12310) * localsink: skip keys that resolve outside the sink directory Co-Authored-By: Claude Opus 5.5 * filer: reject entry paths with dot segments Co-Authored-By: Claude Opus 5.5 * filer: test renames onto dot-segment targets Co-Authored-By: Claude Opus 5.5 * filer: keep the dot-segment rename test inside one bucket scope Co-Authored-By: Claude Opus 5.5 * localsink: resolve keys through an os.Root on the sink directory The lexical check alone lets a symlinked directory that already exists under the sink directory carry writes and deletes outside it. Create, rename and delete now go through an os.Root opened on the sink directory, so a symlink can only resolve inside it. Co-Authored-By: Claude Opus 5.5 * localsink: follow absolute symlinks that resolve inside the sink directory os.Root rejects every absolute symlink target, so a backup through an absolute link that points back into the sink failed, and a delete through it left the file behind. Resolve the parent of the key first and use the resolved path when it stays under the sink directory. Operations still go through os.Root, so a link swapped after the check cannot escape. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- weed/command/filer_sync_process_test.go | 35 +++++ weed/filer/filer.go | 7 + weed/replication/sink/localsink/local_sink.go | 79 ++++++++-- .../sink/localsink/local_sink_dir_test.go | 146 ++++++++++++++++++ .../filer_grpc_server_create_route_test.go | 37 +++++ weed/server/filer_grpc_server_rename_test.go | 29 ++++ weed/util/fullpath.go | 9 ++ 7 files changed, 326 insertions(+), 16 deletions(-) create mode 100644 weed/replication/sink/localsink/local_sink_dir_test.go diff --git a/weed/command/filer_sync_process_test.go b/weed/command/filer_sync_process_test.go index 132902134a..3e81e0d914 100644 --- a/weed/command/filer_sync_process_test.go +++ b/weed/command/filer_sync_process_test.go @@ -1,11 +1,14 @@ package command import ( + "os" + "path/filepath" "strings" "testing" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/replication/sink" + "github.com/seaweedfs/seaweedfs/weed/replication/sink/localsink" "github.com/seaweedfs/seaweedfs/weed/replication/source" "github.com/seaweedfs/seaweedfs/weed/util" ) @@ -343,3 +346,35 @@ func TestGenProcessFunctionRenameFromExcludedDirBecomesCreate(t *testing.T) { t.Fatalf("unexpected delete/update calls: deletes=%v updates=%v", dataSink.deleteKeys, dataSink.updateKeys) } } + +func TestGenProcessFunctionLocalSinkStaysInsideTarget(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "backup") + dataSink := &localsink.LocalSink{Dir: target} + dataSink.SetSourceFiler(&source.FilerSource{}) + processFn := genProcessFunction("/", target, nil, nil, nil, nil, dataSink, true, false) + + for _, dir := range []string{"/safe", "/../escape"} { + err := processFn(&filer_pb.SubscribeMetadataResponse{ + Directory: dir, + EventNotification: &filer_pb.EventNotification{ + NewParentPath: dir, + NewEntry: &filer_pb.Entry{ + Name: "marker.txt", + Content: []byte("marker"), + Attributes: &filer_pb.FuseAttributes{FileMode: 0644, FileSize: 6}, + }, + }, + }) + if err != nil { + t.Fatalf("processFn %s: %v", dir, err) + } + } + + if _, err := os.Stat(filepath.Join(target, "safe", "marker.txt")); err != nil { + t.Fatalf("inside file missing: %v", err) + } + if _, err := os.Stat(filepath.Join(root, "escape")); !os.IsNotExist(err) { + t.Fatalf("local sink wrote outside %s: %v", target, err) + } +} diff --git a/weed/filer/filer.go b/weed/filer/filer.go index bce66f5889..bae29d10bf 100644 --- a/weed/filer/filer.go +++ b/weed/filer/filer.go @@ -501,6 +501,10 @@ func (f *Filer) CreateEntry(ctx context.Context, entry *Entry, existing *Entry, return filer_pb.ErrEntryNameTooLong } + if entry.FullPath.HasDotSegment() { + return fmt.Errorf("invalid path %s", entry.FullPath) + } + if entry.IsDirectory() { entry.Attr.TtlSec = 0 } @@ -794,6 +798,9 @@ func (f *Filer) EnsureDirectoryEntry(ctx context.Context, dirPath util.FullPath, } func (f *Filer) UpdateEntry(ctx context.Context, oldEntry, entry *Entry, isFromOtherCluster bool) (err error) { + if entry.FullPath.HasDotSegment() { + return fmt.Errorf("invalid path %s", entry.FullPath) + } ctx = f.syncWriteContext(ctx, entry.FullPath) if oldEntry != nil { entry.Attr.Crtime = oldEntry.Attr.Crtime diff --git a/weed/replication/sink/localsink/local_sink.go b/weed/replication/sink/localsink/local_sink.go index 8debf7993a..b66e8abff4 100644 --- a/weed/replication/sink/localsink/local_sink.go +++ b/weed/replication/sink/localsink/local_sink.go @@ -3,6 +3,7 @@ package localsink import ( "context" "fmt" + "math/rand/v2" "os" "path/filepath" "strings" @@ -39,6 +40,34 @@ func (localsink *LocalSink) isMultiPartEntry(key string) bool { return strings.HasSuffix(key, ".part") && strings.Contains(key, "/"+s3_constants.MultipartUploadsFolder+"/") } +func (localsink *LocalSink) isInsideDir(key string) (string, bool) { + rel, err := filepath.Rel(filepath.Clean(localsink.Dir), filepath.Clean(key)) + if err == nil && filepath.IsLocal(rel) { + return rel, true + } + glog.Warningf("local sink: skip %s outside %s", key, localsink.Dir) + return "", false +} + +// resolveInsideDir resolves symlinks in the parent of rel while it stays inside the sink directory. +func (localsink *LocalSink) resolveInsideDir(rel string) string { + base, err := filepath.EvalSymlinks(localsink.Dir) + if err != nil { + return rel + } + dir, rest := filepath.Dir(rel), filepath.Base(rel) + for dir != "." { + if resolved, err := filepath.EvalSymlinks(filepath.Join(base, dir)); err == nil { + if inside, err := filepath.Rel(base, resolved); err == nil && filepath.IsLocal(inside) { + return filepath.Join(inside, rest) + } + return rel + } + dir, rest = filepath.Dir(dir), filepath.Join(filepath.Base(dir), rest) + } + return rel +} + func (localsink *LocalSink) initialize(dir string, isIncremental bool) error { localsink.Dir = dir localsink.isIncremental = isIncremental @@ -69,8 +98,17 @@ func (localsink *LocalSink) DeleteEntry(key string, isDirectory, deleteIncludeCh if localsink.isMultiPartEntry(key) { return nil } + rel, ok := localsink.isInsideDir(key) + if !ok { + return nil + } glog.V(4).Infof("Delete Entry key: %s", key) - if err := os.Remove(key); err != nil { + root, err := os.OpenRoot(localsink.Dir) + if err == nil { + err = root.Remove(localsink.resolveInsideDir(rel)) + root.Close() + } + if err != nil { glog.V(0).Infof("remove entry key %s: %s", key, err) } return nil @@ -81,6 +119,10 @@ func (localsink *LocalSink) CreateEntry(key string, entry *filer_pb.Entry, signa if entry.IsDirectory || localsink.isMultiPartEntry(key) { return nil } + rel, ok := localsink.isInsideDir(key) + if !ok { + return nil + } glog.V(4).Infof("Create Entry key: %s", key) totalSize := filer.FileSize(entry) @@ -89,35 +131,37 @@ func (localsink *LocalSink) CreateEntry(key string, entry *filer_pb.Entry, signa return fmt.Errorf("resolve chunks of %s: %w", key, err) } - dir := filepath.Dir(key) - - if _, err := os.Stat(dir); os.IsNotExist(err) { - glog.V(4).Infof("Create Directory key: %s", dir) - if err = os.MkdirAll(dir, 0755); err != nil { - return err - } + if err := os.MkdirAll(localsink.Dir, 0755); err != nil { + return err } + root, err := os.OpenRoot(localsink.Dir) + if err != nil { + return err + } + defer root.Close() - if entry.IsDirectory { - return os.Mkdir(key, os.FileMode(entry.Attributes.FileMode)) + rel = localsink.resolveInsideDir(rel) + dir := filepath.Dir(rel) + if err := root.MkdirAll(dir, 0755); err != nil { + return err } mode := os.FileMode(entry.Attributes.FileMode) - shortFileName := util.ToShortFileName(key) + shortFileName := util.ToShortFileName(rel) // Write to a temp file in the same directory, then atomically rename // on success. This prevents leaving a truncated/empty file if // decryption or chunk copy fails. - tmpFile, err := os.CreateTemp(dir, ".seaweedfs-tmp-*") + tmpName := filepath.Join(dir, fmt.Sprintf(".seaweedfs-tmp-%d", rand.Uint64())) + tmpFile, err := root.OpenFile(tmpName, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600) if err != nil { return err } - tmpName := tmpFile.Name() defer func() { // Clean up temp file on any error (rename removes it on success) if tmpFile != nil { tmpFile.Close() - os.Remove(tmpName) + root.Remove(tmpName) } }() @@ -151,8 +195,8 @@ func (localsink *LocalSink) CreateEntry(key string, entry *filer_pb.Entry, signa tmpFile = nil // prevent deferred cleanup // Atomic rename into final destination - if err := os.Rename(tmpName, shortFileName); err != nil { - os.Remove(tmpName) + if err := root.Rename(tmpName, shortFileName); err != nil { + root.Remove(tmpName) return err } @@ -164,6 +208,9 @@ func (localsink *LocalSink) UpdateEntry(key string, oldEntry *filer_pb.Entry, ne if localsink.isMultiPartEntry(key) { return true, nil } + if _, ok := localsink.isInsideDir(key); !ok { + return true, nil + } glog.V(4).Infof("Update Entry key: %s", key) // do delete and create foundExistingEntry = util.FileExists(key) diff --git a/weed/replication/sink/localsink/local_sink_dir_test.go b/weed/replication/sink/localsink/local_sink_dir_test.go new file mode 100644 index 0000000000..c044ca500a --- /dev/null +++ b/weed/replication/sink/localsink/local_sink_dir_test.go @@ -0,0 +1,146 @@ +package localsink + +import ( + "os" + "path/filepath" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/replication/source" +) + +func TestLocalSinkStaysInsideDir(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "backup") + sink := &LocalSink{} + sink.initialize(dir, false) + sink.SetSourceFiler(&source.FilerSource{}) + + entry := func() *filer_pb.Entry { + return &filer_pb.Entry{ + Name: "marker.txt", + Content: []byte("marker"), + Attributes: &filer_pb.FuseAttributes{FileMode: 0644, FileSize: 6}, + } + } + + inside := filepath.Join(dir, "safe", "marker.txt") + if err := sink.CreateEntry(inside, entry(), nil); err != nil { + t.Fatalf("create inside: %v", err) + } + if _, err := os.Stat(inside); err != nil { + t.Fatalf("inside file missing: %v", err) + } + + outside := filepath.Join(dir, "..", "escape", "marker.txt") + if err := sink.CreateEntry(outside, entry(), nil); err != nil { + t.Fatalf("create outside: %v", err) + } + if _, err := sink.UpdateEntry(outside, nil, "", entry(), false, nil); err != nil { + t.Fatalf("update outside: %v", err) + } + if _, err := os.Stat(filepath.Join(root, "escape")); !os.IsNotExist(err) { + t.Fatalf("sink wrote outside %s: %v", dir, err) + } + + victim := filepath.Join(root, "victim.txt") + if err := os.WriteFile(victim, []byte("keep"), 0644); err != nil { + t.Fatal(err) + } + if err := sink.DeleteEntry(filepath.Join(dir, "..", "victim.txt"), false, false, nil); err != nil { + t.Fatalf("delete outside: %v", err) + } + if _, err := os.Stat(victim); err != nil { + t.Fatalf("sink deleted a file outside %s: %v", dir, err) + } +} + +func TestLocalSinkDoesNotFollowSymlinksOutOfDir(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "backup") + outside := filepath.Join(root, "outside") + for _, d := range []string{dir, outside} { + if err := os.MkdirAll(d, 0755); err != nil { + t.Fatal(err) + } + } + if err := os.Symlink(outside, filepath.Join(dir, "link")); err != nil { + t.Skipf("symlink: %v", err) + } + sink := &LocalSink{} + sink.initialize(dir, false) + sink.SetSourceFiler(&source.FilerSource{}) + + entry := &filer_pb.Entry{ + Name: "marker.txt", + Content: []byte("marker"), + Attributes: &filer_pb.FuseAttributes{FileMode: 0644, FileSize: 6}, + } + if err := sink.CreateEntry(filepath.Join(dir, "link", "marker.txt"), entry, nil); err == nil { + t.Fatalf("create through a symlink out of %s succeeded", dir) + } + if _, err := os.Stat(filepath.Join(outside, "marker.txt")); !os.IsNotExist(err) { + t.Fatalf("sink wrote through a symlink out of %s: %v", dir, err) + } + + victim := filepath.Join(outside, "victim.txt") + if err := os.WriteFile(victim, []byte("keep"), 0644); err != nil { + t.Fatal(err) + } + if err := sink.DeleteEntry(filepath.Join(dir, "link", "victim.txt"), false, false, nil); err != nil { + t.Fatalf("delete through link: %v", err) + } + if _, err := os.Stat(victim); err != nil { + t.Fatalf("sink deleted through a symlink out of %s: %v", dir, err) + } + + if err := os.Mkdir(filepath.Join(dir, "real"), 0755); err != nil { + t.Fatal(err) + } + if err := os.Symlink("real", filepath.Join(dir, "inner")); err != nil { + t.Fatal(err) + } + if err := sink.CreateEntry(filepath.Join(dir, "inner", "marker.txt"), entry, nil); err != nil { + t.Fatalf("create through a symlink inside %s: %v", dir, err) + } + if _, err := os.Stat(filepath.Join(dir, "real", "marker.txt")); err != nil { + t.Fatalf("inside symlink target missing: %v", err) + } +} + +func TestLocalSinkFollowsAbsoluteSymlinksInsideDir(t *testing.T) { + dir := filepath.Join(t.TempDir(), "backup") + target := filepath.Join(dir, "real") + if err := os.MkdirAll(target, 0755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, filepath.Join(dir, "abs")); err != nil { + t.Skipf("symlink: %v", err) + } + sink := &LocalSink{} + sink.initialize(dir, false) + sink.SetSourceFiler(&source.FilerSource{}) + + entry := &filer_pb.Entry{ + Name: "marker.txt", + Content: []byte("marker"), + Attributes: &filer_pb.FuseAttributes{FileMode: 0644, FileSize: 6}, + } + for _, name := range []string{"marker.txt", filepath.Join("sub", "marker.txt")} { + if err := sink.CreateEntry(filepath.Join(dir, "abs", name), entry, nil); err != nil { + t.Fatalf("create %s through an absolute symlink inside %s: %v", name, dir, err) + } + if _, err := os.Stat(filepath.Join(target, name)); err != nil { + t.Fatalf("absolute symlink target %s missing: %v", name, err) + } + if err := sink.DeleteEntry(filepath.Join(dir, "abs", name), false, false, nil); err != nil { + t.Fatalf("delete %s through an absolute symlink: %v", name, err) + } + if _, err := os.Stat(filepath.Join(target, name)); !os.IsNotExist(err) { + t.Fatalf("delete through an absolute symlink left %s: %v", name, err) + } + } + if _, err := os.Lstat(filepath.Join(dir, "abs")); err != nil { + t.Fatalf("symlink itself removed: %v", err) + } +} diff --git a/weed/server/filer_grpc_server_create_route_test.go b/weed/server/filer_grpc_server_create_route_test.go index 0c266c5397..8bb4bbbc33 100644 --- a/weed/server/filer_grpc_server_create_route_test.go +++ b/weed/server/filer_grpc_server_create_route_test.go @@ -232,3 +232,40 @@ func TestCreateEntryFailsWhenPriorOwnerIsUnreachable(t *testing.T) { t.Fatal("a create routed away must not also write locally") } } + +func TestCreateEntryRejectsDotSegments(t *testing.T) { + fs, store := createRouteServer(t) + + for _, tc := range []struct{ dir, name string }{ + {"/../escape", "marker.txt"}, + {"/test/./sub", "marker.txt"}, + {"/test", ".."}, + } { + req := createReq(tc.name, false) + req.Directory = tc.dir + resp, err := fs.CreateEntry(context.Background(), req) + if err == nil && resp.Error == "" { + t.Fatalf("CreateEntry %s/%s must be rejected", tc.dir, tc.name) + } + if _, found := store.entries[string(util.NewFullPath(tc.dir, tc.name))]; found { + t.Fatalf("CreateEntry %s/%s stored the raw path", tc.dir, tc.name) + } + } + + resp, err := fs.CreateEntry(context.Background(), createReq("marker.txt", false)) + if err != nil || resp.Error != "" { + t.Fatalf("CreateEntry /test/marker.txt: err=%v resp=%v", err, resp) + } + + update := &filer_pb.UpdateEntryRequest{ + Directory: "/x/../test", + Entry: createReq("marker.txt", false).Entry, + } + update.Entry.Attributes.Mtime++ + if _, err := fs.UpdateEntry(context.Background(), update); err == nil { + t.Fatal("UpdateEntry with a dot segment must be rejected") + } + if _, found := store.entries["/x/../test/marker.txt"]; found { + t.Fatal("UpdateEntry stored the raw path") + } +} diff --git a/weed/server/filer_grpc_server_rename_test.go b/weed/server/filer_grpc_server_rename_test.go index 10d6941406..b793025472 100644 --- a/weed/server/filer_grpc_server_rename_test.go +++ b/weed/server/filer_grpc_server_rename_test.go @@ -559,3 +559,32 @@ func TestAtomicRenameReleasesQueuedStampsOnCommitFailure(t *testing.T) { t.Fatalf("LocalDeliveredThroughTsNs = %d, want %d: a leaked stamp still caps the claim", got, now) } } + +func TestAtomicRenameEntryRejectsDotSegmentTarget(t *testing.T) { + for _, tc := range []struct{ dir, name string }{ + {"/../escape", "dst.txt"}, + {"/", ".."}, + } { + store := newRenameTestStore() + store.entries["/src.txt"] = newFileEntry("/src.txt", 101) + f := newRenameTestFiler(t, store) + f.DirBucketsPath = "/buckets" + server := &FilerServer{filer: f, entryLockTable: util.NewLockTable[util.FullPath]()} + + _, err := server.AtomicRenameEntry(context.Background(), &filer_pb.AtomicRenameEntryRequest{ + OldDirectory: "/", + OldName: "src.txt", + NewDirectory: tc.dir, + NewName: tc.name, + }) + if err == nil { + t.Fatalf("rename to %s/%s must be rejected", tc.dir, tc.name) + } + if _, found := store.entries["/src.txt"]; !found { + t.Fatalf("rename to %s/%s removed the source", tc.dir, tc.name) + } + if len(store.entries) != 1 { + t.Fatalf("rename to %s/%s left entries %v", tc.dir, tc.name, store.entries) + } + } +} diff --git a/weed/util/fullpath.go b/weed/util/fullpath.go index 866592f95d..529344a548 100644 --- a/weed/util/fullpath.go +++ b/weed/util/fullpath.go @@ -76,6 +76,15 @@ func (fp FullPath) IsLongerFileName(maxFilenameLength uint32) bool { return uint32(len([]byte(fp.Name()))) > maxFilenameLength } +func (fp FullPath) HasDotSegment() bool { + for _, seg := range strings.Split(string(fp), "/") { + if seg == "." || seg == ".." { + return true + } + } + return false +} + func (fp FullPath) Child(name string) FullPath { dir := string(fp) noPrefix := name