From 37f3dff67740684e0a60c3a410863d4c46ba228d Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sat, 8 Aug 2026 09:25:57 -0700 Subject: [PATCH] volume: validate the file extension in CopyFile and ReceiveFile (#10644) * volume: validate the file extension in CopyFile and ReceiveFile CopyFile and ReceiveFile build an on-disk path from the client-supplied Ext. Both are intentionally ungated for cluster-internal peers, so a value like "/../../x" is joined onto the volume directory and, once path-cleaned, resolves outside it -- an EC-shard receive can then write, and CopyFile read, anywhere the process can reach. Constrain Ext to a real suffix (a leading dot followed by alphanumerics) before it is used to build any path, so it can no longer carry a separator or a parent reference. * test: use an alphanumeric missing-file extension in the copy variants The not-found and stop-offset-zero cases used ".definitely-missing" as a deliberately absent source. The extension is now validated, and the hyphen makes it invalid, so switch to ".missing" -- still a nonexistent file, but a real extension shape. * volume: validate the collection in CopyFile and ReceiveFile The client-supplied Collection is folded into the on-disk path as "_" by VolumeFileName and EcShardBaseFileName, both joined with path.Join / util.Join. A Collection carrying a separator, e.g. "../../x", therefore path-cleans to a target outside the volume directory, the same escape the extension check just closed. Reject a collection that is a bare parent reference or holds a separator; ordinary names ('.', '-' and all) still pass. --- .../grpc/copy_receive_variants_test.go | 6 +- weed/server/volume_grpc_copy.go | 55 +++++ .../server/volume_grpc_copy_traversal_test.go | 212 ++++++++++++++++++ 3 files changed, 270 insertions(+), 3 deletions(-) create mode 100644 weed/server/volume_grpc_copy_traversal_test.go diff --git a/test/volume_server/grpc/copy_receive_variants_test.go b/test/volume_server/grpc/copy_receive_variants_test.go index 3a82822b2..792ed3844 100644 --- a/test/volume_server/grpc/copy_receive_variants_test.go +++ b/test/volume_server/grpc/copy_receive_variants_test.go @@ -90,7 +90,7 @@ func TestCopyFileIgnoreNotFoundAndStopOffsetZeroPaths(t *testing.T) { missingNoIgnore, err := grpcClient.CopyFile(ctx, &volume_server_pb.CopyFileRequest{ VolumeId: volumeID, - Ext: ".definitely-missing", + Ext: ".missing", CompactionRevision: math.MaxUint32, StopOffset: 1, IgnoreSourceFileNotFound: false, @@ -104,7 +104,7 @@ func TestCopyFileIgnoreNotFoundAndStopOffsetZeroPaths(t *testing.T) { missingIgnored, err := grpcClient.CopyFile(ctx, &volume_server_pb.CopyFileRequest{ VolumeId: volumeID, - Ext: ".definitely-missing", + Ext: ".missing", CompactionRevision: math.MaxUint32, StopOffset: 1, IgnoreSourceFileNotFound: true, @@ -119,7 +119,7 @@ func TestCopyFileIgnoreNotFoundAndStopOffsetZeroPaths(t *testing.T) { stopZeroStream, err := grpcClient.CopyFile(ctx, &volume_server_pb.CopyFileRequest{ VolumeId: volumeID, - Ext: ".definitely-missing", + Ext: ".missing", CompactionRevision: math.MaxUint32, StopOffset: 0, IgnoreSourceFileNotFound: false, diff --git a/weed/server/volume_grpc_copy.go b/weed/server/volume_grpc_copy.go index 800399591..a33be5a42 100644 --- a/weed/server/volume_grpc_copy.go +++ b/weed/server/volume_grpc_copy.go @@ -6,6 +6,7 @@ import ( "io" "math" "os" + "strings" "time" "github.com/seaweedfs/seaweedfs/weed/pb/master_pb" @@ -480,11 +481,52 @@ func (vs *VolumeServer) ReadVolumeFileStatus(ctx context.Context, req *volume_se return resp, nil } +// checkVolumeFileExtension guards the client-supplied Ext that CopyFile and +// ReceiveFile turn into an on-disk path. Both RPCs are intentionally ungated +// for cluster-internal peers (see volume_grpc_admin_auth_coverage_test.go), so +// this is the only check standing between a peer request and the os.Open / +// os.Create target: without it an Ext like "/../../x" is joined onto the volume +// directory and, once path-cleaned, resolves outside it. A genuine extension is +// a leading dot followed by alphanumerics -- ".dat", ".idx", ".vif", ".ecx", +// ".ecj", ".ecsum", ".ec00".. -- and never contains a separator or "..". +func checkVolumeFileExtension(ext string) error { + if len(ext) < 2 || ext[0] != '.' { + return fmt.Errorf("invalid file extension %q", ext) + } + for _, r := range ext[1:] { + if r < '0' || (r > '9' && r < 'A') || (r > 'Z' && r < 'a') || r > 'z' { + return fmt.Errorf("invalid file extension %q", ext) + } + } + return nil +} + +// checkVolumeCollection guards the client-supplied Collection, which CopyFile +// and ReceiveFile fold into a path component ("_"). An empty +// collection is the default; any other value must be a single path element so a +// collection like "../../x" cannot climb out of the volume directory once +// path-cleaned. Collection names are user-facing and may hold '.' or '-', so +// this rejects only separators and bare parent references rather than the +// stricter alphanumeric rule used for extensions. +func checkVolumeCollection(collection string) error { + if collection == "." || collection == ".." || strings.ContainsAny(collection, `/\`) { + return fmt.Errorf("invalid collection %q", collection) + } + return nil +} + // CopyFile client pulls the volume related file from the source server. // if req.CompactionRevision != math.MaxUint32, it ensures the compact revision is as expected // The copying still stop at req.StopOffset, but you can set it to math.MaxUint64 in order to read all data. func (vs *VolumeServer) CopyFile(req *volume_server_pb.CopyFileRequest, stream volume_server_pb.VolumeServer_CopyFileServer) error { + if err := checkVolumeFileExtension(req.Ext); err != nil { + return err + } + if err := checkVolumeCollection(req.Collection); err != nil { + return err + } + var fileName string if !req.IsEcVolume { v := vs.store.GetVolume(needle.VolumeId(req.VolumeId)) @@ -652,6 +694,19 @@ func (vs *VolumeServer) ReceiveFile(stream volume_server_pb.VolumeServer_Receive glog.V(1).Infof("ReceiveFile: volume %d, ext %s, collection %s, shard %d, size %d", fileInfo.VolumeId, fileInfo.Ext, fileInfo.Collection, fileInfo.ShardId, fileInfo.FileSize) + if err := checkVolumeFileExtension(fileInfo.Ext); err != nil { + glog.Errorf("ReceiveFile: %v", err) + return stream.SendAndClose(&volume_server_pb.ReceiveFileResponse{ + Error: err.Error(), + }) + } + if err := checkVolumeCollection(fileInfo.Collection); err != nil { + glog.Errorf("ReceiveFile: %v", err) + return stream.SendAndClose(&volume_server_pb.ReceiveFileResponse{ + Error: err.Error(), + }) + } + if fileInfo.IsEcVolume { // os.Create below truncates in place; a mounted EcVolume // holds fds on the same inodes, so overwriting corrupts diff --git a/weed/server/volume_grpc_copy_traversal_test.go b/weed/server/volume_grpc_copy_traversal_test.go new file mode 100644 index 000000000..89ada05e6 --- /dev/null +++ b/weed/server/volume_grpc_copy_traversal_test.go @@ -0,0 +1,212 @@ +package weed_server + +import ( + "io" + "os" + "path/filepath" + "strings" + "testing" + + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" + + "github.com/seaweedfs/seaweedfs/weed/pb/volume_server_pb" + "github.com/seaweedfs/seaweedfs/weed/stats" + "github.com/seaweedfs/seaweedfs/weed/storage" + "github.com/seaweedfs/seaweedfs/weed/storage/types" + "github.com/seaweedfs/seaweedfs/weed/util" +) + +// newTraversalTestStore builds a single-location store rooted at dir. +func newTraversalTestStore(dir string) *storage.Store { + return storage.NewStore( + grpc.WithTransportCredentials(insecure.NewCredentials()), + "127.0.0.1", 0, 0, "", "test-store", + []string{dir}, []int32{10}, []util.MinFreeSpace{{}}, + dir, storage.NeedleMapInMemory, + []types.DiskType{types.HardDriveType}, [][]string{nil}, + 0, stats.DiskIOProbeConfig{}, + ) +} + +// fakeReceiveFileStream scripts a ReceiveFile request sequence and records the +// final response returned via SendAndClose. +type fakeReceiveFileStream struct { + grpc.ServerStream + reqs []*volume_server_pb.ReceiveFileRequest + index int + resp *volume_server_pb.ReceiveFileResponse +} + +func (s *fakeReceiveFileStream) Recv() (*volume_server_pb.ReceiveFileRequest, error) { + if s.index >= len(s.reqs) { + return nil, io.EOF + } + r := s.reqs[s.index] + s.index++ + return r, nil +} + +func (s *fakeReceiveFileStream) SendAndClose(resp *volume_server_pb.ReceiveFileResponse) error { + s.resp = resp + return nil +} + +func infoReq(info *volume_server_pb.ReceiveFileInfo) *volume_server_pb.ReceiveFileRequest { + return &volume_server_pb.ReceiveFileRequest{Data: &volume_server_pb.ReceiveFileRequest_Info{Info: info}} +} + +func contentReq(b []byte) *volume_server_pb.ReceiveFileRequest { + return &volume_server_pb.ReceiveFileRequest{Data: &volume_server_pb.ReceiveFileRequest_FileContent{FileContent: b}} +} + +// TestReceiveFile_RejectsTraversalExt ensures a client-supplied Ext with parent +// references cannot steer the EC-shard write outside the volume directory. The +// EC branch joins the Ext with util.Join, which path-cleans the ".." away +// before os.Create, so a bare concatenation check would miss it. +func TestReceiveFile_RejectsTraversalExt(t *testing.T) { + root := t.TempDir() + storeDir := filepath.Join(root, "a", "b", "store") + if err := os.MkdirAll(storeDir, 0o755); err != nil { + t.Fatal(err) + } + vs := &VolumeServer{store: newTraversalTestStore(storeDir)} + + stream := &fakeReceiveFileStream{reqs: []*volume_server_pb.ReceiveFileRequest{ + infoReq(&volume_server_pb.ReceiveFileInfo{ + VolumeId: 4, + Ext: "/../../pwned.ec00", + IsEcVolume: true, + FileSize: 5, + }), + contentReq([]byte("pwned")), + }} + + if err := vs.ReceiveFile(stream); err != nil { + t.Fatalf("ReceiveFile returned transport error: %v", err) + } + if stream.resp == nil || stream.resp.Error == "" { + t.Errorf("traversal ext was accepted; response = %+v", stream.resp) + } + + assertNoEscapedFile(t, root, storeDir, "pwned") +} + +// TestReceiveFile_RejectsTraversalCollection covers the sibling vector: the +// Collection is folded into the path as "_", so a value with +// a separator escapes the volume directory the same way a traversal ext does. +func TestReceiveFile_RejectsTraversalCollection(t *testing.T) { + root := t.TempDir() + storeDir := filepath.Join(root, "a", "b", "store") + if err := os.MkdirAll(storeDir, 0o755); err != nil { + t.Fatal(err) + } + vs := &VolumeServer{store: newTraversalTestStore(storeDir)} + + stream := &fakeReceiveFileStream{reqs: []*volume_server_pb.ReceiveFileRequest{ + infoReq(&volume_server_pb.ReceiveFileInfo{ + VolumeId: 4, + Ext: ".ec00", + Collection: "../../pwned", + IsEcVolume: true, + FileSize: 5, + }), + contentReq([]byte("pwned")), + }} + + if err := vs.ReceiveFile(stream); err != nil { + t.Fatalf("ReceiveFile returned transport error: %v", err) + } + if stream.resp == nil || stream.resp.Error == "" { + t.Errorf("traversal collection was accepted; response = %+v", stream.resp) + } + assertNoEscapedFile(t, root, storeDir, "pwned") +} + +// TestReceiveFile_AcceptsNormalExt is the positive control: a legitimate EC +// shard extension still lands inside the volume directory. +func TestReceiveFile_AcceptsNormalExt(t *testing.T) { + storeDir := t.TempDir() + vs := &VolumeServer{store: newTraversalTestStore(storeDir)} + + // A realistic collection name holding '.' and '-' must still be accepted. + stream := &fakeReceiveFileStream{reqs: []*volume_server_pb.ReceiveFileRequest{ + infoReq(&volume_server_pb.ReceiveFileInfo{ + VolumeId: 4, + Ext: ".ec00", + Collection: "my.bucket-1", + IsEcVolume: true, + FileSize: 5, + }), + contentReq([]byte("shard")), + }} + + if err := vs.ReceiveFile(stream); err != nil { + t.Fatalf("ReceiveFile: %v", err) + } + if stream.resp == nil || stream.resp.Error != "" { + t.Fatalf("normal ext rejected: %+v", stream.resp) + } + if got, err := os.ReadFile(filepath.Join(storeDir, "my.bucket-1_4.ec00")); err != nil || string(got) != "shard" { + t.Fatalf("expected shard written inside volume dir, got %q err %v", got, err) + } +} + +// fakeCopyFileServer records streamed content for CopyFile. +type fakeCopyFileServer struct { + grpc.ServerStream + sent []byte +} + +func (s *fakeCopyFileServer) Send(resp *volume_server_pb.CopyFileResponse) error { + s.sent = append(s.sent, resp.FileContent...) + return nil +} + +// TestCopyFile_RejectsTraversalExt ensures the read side cannot be steered to an +// arbitrary file with a traversal Ext. +func TestCopyFile_RejectsTraversalExt(t *testing.T) { + root := t.TempDir() + storeDir := filepath.Join(root, "a", "b", "store") + if err := os.MkdirAll(storeDir, 0o755); err != nil { + t.Fatal(err) + } + secret := filepath.Join(root, "a", "b", "secret.txt") + if err := os.WriteFile(secret, []byte("top secret"), 0o644); err != nil { + t.Fatal(err) + } + vs := &VolumeServer{store: newTraversalTestStore(storeDir)} + + // util.Join(storeDir, "4"+"/../../secret.txt") path-cleans to the secret. + req := &volume_server_pb.CopyFileRequest{ + VolumeId: 4, + Ext: "/../../secret.txt", + IsEcVolume: true, + StopOffset: 1 << 20, + } + server := &fakeCopyFileServer{} + err := vs.CopyFile(req, server) + if err == nil { + t.Errorf("CopyFile accepted a traversal ext") + } + if len(server.sent) != 0 { + t.Errorf("CopyFile leaked %d bytes for a traversal ext: %q", len(server.sent), server.sent) + } +} + +// assertNoEscapedFile fails if any file whose name contains needle exists under +// root but outside storeDir. +func assertNoEscapedFile(t *testing.T, root, storeDir, needle string) { + t.Helper() + _ = filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil || d.IsDir() { + return nil + } + rel, _ := filepath.Rel(storeDir, path) + outside := rel == "" || rel[0] == '.' + if outside && strings.Contains(d.Name(), needle) { + t.Errorf("path traversal: file written outside volume dir at %s", path) + } + return nil + }) +}