diff --git a/cmd/erasure-metadata.go b/cmd/erasure-metadata.go
index f4b8798ba..e0ef25e4b 100644
--- a/cmd/erasure-metadata.go
+++ b/cmd/erasure-metadata.go
@@ -69,6 +69,26 @@ func (e ErasureInfo) ShardSize() int64 {
return ceilFrac(e.BlockSize, int64(e.DataBlocks))
}
+// HasNegativePartSize reports whether any part claims a negative size.
+//
+// Such metadata is never legitimate, and it is not merely cosmetic: a negative
+// length floors both terms of ShardFileSize to zero, and checkPart's only
+// integrity test is "st.Size() < expectedSize". A zero expectation is therefore
+// satisfied by every file that exists, including a truncated shard, so the part
+// is reported intact and a heal driven by the result skips the repair it should
+// have performed.
+//
+// Note this holds even when the erasure parameters are entirely valid, so
+// FileInfo.IsValid() - the check healing itself trusts - does not catch it.
+func (fi FileInfo) HasNegativePartSize() bool {
+ for _, p := range fi.Parts {
+ if p.Size < 0 {
+ return true
+ }
+ }
+ return false
+}
+
// IsValid - tells if erasure info fields are valid.
func (fi FileInfo) IsValid() bool {
if fi.Deleted {
diff --git a/cmd/storage-rest-server-guard.go b/cmd/storage-rest-server-guard.go
new file mode 100644
index 000000000..99efcb2fd
--- /dev/null
+++ b/cmd/storage-rest-server-guard.go
@@ -0,0 +1,417 @@
+// Copyright (c) 2015-2026 MinIO, Inc.
+//
+// This file is part of MinIO Object Storage stack
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program. If not, see .
+
+package cmd
+
+import (
+ "context"
+ "io"
+ "runtime"
+
+ "github.com/minio/madmin-go/v3"
+ xioutil "github.com/minio/minio/internal/ioutil"
+)
+
+// guardedStorage rejects filesystem paths that arrive from an internode
+// payload and would resolve outside the volume they name.
+//
+// Why here and not in each handler: the global HTTP middleware validates only
+// r.URL.Path and r.Form (query arguments), and r.Form is never populated from
+// a request body. Paths carried in a msgpack body, or in a grid RPC frame on
+// the long-lived /minio/grid/v1 websocket, therefore reach xlStorage
+// unvalidated. Every storage-REST and grid handler obtains its StorageAPI from
+// storageRESTServer.getStorage(), so wrapping that one call covers all of them
+// at once, sees the nested struct fields a per-handler check tends to miss,
+// and cannot drift out of sync as handlers are added.
+//
+// Scope, deliberately narrow:
+//
+// - Only *path* arguments are checked here. Volume arguments are validated
+// at the sink by xlStorage.getVolDir, which additionally covers callers
+// that bypass this wrapper entirely (the peer-S3 bucket RPCs). Do not
+// conclude from the absence of a volume check here that volumes are inert.
+//
+// - Local callers - the erasure layer talking to its own drives - are not
+// wrapped. Their object names already passed IsValidObjectName at the S3
+// boundary, so wrapping them would add cost and regression risk without
+// adding a control.
+type guardedStorage struct {
+ StorageAPI
+}
+
+// isVolumeRootAlias reports whether p addresses the volume directory itself
+// rather than something inside it, i.e. whether pathJoin(volumeDir, p)
+// collapses back to volumeDir. That happens only for the empty string and for
+// strings made up entirely of separators.
+//
+// Whitespace must NOT be treated as a separator here, even though
+// hasBadPathComponent trims it when comparing a segment against "."/"..".
+// path.Clean does not touch spaces, so pathJoin(volumeDir, " ") is
+// "volumeDir/ " - a real directory named two spaces, not the volume root. A
+// whitespace-only object key is legal in S3 (IsValidObjectName accepts it) and
+// is committed through RenameData on the PutObject path, so rejecting it here
+// would fail those writes on every remote drive at once. See
+// TestGuardAcceptsEveryLegalObjectName.
+//
+// Three characters collapse a component on Windows but not on Unix, so the
+// rule is platform-split and exercised from either platform through
+// isVolumeRootAliasOn:
+//
+// - Backslash is a separator on Windows only. path.Clean never treats it as
+// one, so on Unix "\\" names an ordinary file and is a legal S3 object key;
+// refusing it there would make a distributed cluster reject a write that a
+// single-node server accepts.
+//
+// - Space and period are stripped from the end of a path component by the
+// Win32 normalisation layer. A component made only of those characters
+// therefore disappears and the path resolves to its parent - the volume
+// root. Go does not add the \\?\ prefix that would suppress this for the
+// short paths used here, so " " and "..." reach the syscall as an empty
+// component. On Unix they are ordinary filenames and legal object keys.
+//
+// Note the second point is reasoned from documented Win32 behaviour, not from a
+// Windows test run: CI is Linux-only, so TestIsVolumeRootAliasIsPlatformCorrect
+// pins both branches of the predicate rather than the syscall behaviour itself.
+func isVolumeRootAlias(p string) bool {
+ return isVolumeRootAliasOn(p, runtime.GOOS == globalWindowsOSName)
+}
+
+func isVolumeRootAliasOn(p string, windows bool) bool {
+ for i := range len(p) {
+ if p[i] == SlashSeparatorChar {
+ continue
+ }
+ if windows && (p[i] == '\\' || p[i] == ' ' || p[i] == '.') {
+ continue
+ }
+ return false
+ }
+ return true
+}
+
+// guardErasureParams rejects a FileInfo from which no meaningful expected shard
+// size can be derived, because "no meaningful size" degrades to "everything
+// passes" rather than to an error.
+//
+// checkPart's only integrity test is "st.Size() < expectedSize". Whenever
+// ShardFileSize yields 0, that comparison is false for every file that exists -
+// including a truncated shard - so the part comes back reported healthy and a
+// heal driven by the result skips a shard that actually needs repair. Two
+// distinct inputs produce that 0:
+//
+// - Unusable erasure parameters with a part of positive size. ShardFileSize
+// returns 0 early so the division cannot panic.
+//
+// - A part of NEGATIVE size, with or without usable parameters: numShards
+// floors to 0 and ceilFrac of a negative numerator is 0, so the arithmetic
+// lands on 0 by itself. This one is easy to miss precisely because valid
+// erasure parameters do not save you from it.
+//
+// Parts of zero length are left alone - ShardFileSize legitimately returns 0
+// for them, so they say nothing about whether the metadata is sound.
+//
+// This is the boundary check; ShardFileSize's own zero-value guard stays as the
+// last line of defence against a panic.
+func guardErasureParams(fi FileInfo) error {
+ // Negative sizes share their rule with the storage layer, which also has to
+ // cope with metadata already on disk; keep the two from drifting apart by
+ // asking the same predicate.
+ if fi.HasNegativePartSize() {
+ return errFileCorrupt
+ }
+ usable := fi.Erasure.BlockSize > 0 && fi.Erasure.DataBlocks > 0
+ for _, p := range fi.Parts {
+ if p.Size > 0 && !usable {
+ return errFileCorrupt
+ }
+ }
+ return nil
+}
+
+// guardPaths rejects traversal in any of the supplied paths. The empty string
+// is allowed: it legitimately names the volume root for listing operations,
+// and an empty FileInfo.DataDir is normal for inline and transitioned objects.
+//
+// Callers pass the entire batch here before invoking storage, so a payload
+// mixing a valid target with a malicious one is rejected whole and performs no
+// partial work. (This returns on the first offending path; what matters is that
+// it runs to a verdict before any element has been acted on.)
+func guardPaths(paths ...string) error {
+ for _, p := range paths {
+ if hasBadPathComponent(p) {
+ return errFileAccessDenied
+ }
+ }
+ return nil
+}
+
+// guardObjectPaths is guardPaths plus a rejection of volume-root aliases. It
+// applies to the destructive verbs only - rename and bulk delete - because
+// those relocate or destroy the volume root when handed one, whereas reads and
+// writes of the volume root merely fail on their own.
+func guardObjectPaths(paths ...string) error {
+ for _, p := range paths {
+ if hasBadPathComponent(p) || isVolumeRootAlias(p) {
+ return errFileAccessDenied
+ }
+ }
+ return nil
+}
+
+// guardVersions validates every path-bearing field reachable through a
+// DeleteVersions payload: the per-object name, and the DataDir of each version.
+// DataDir is joined under the object directory, and may legitimately be empty
+// for inline and transitioned objects, so it gets guardPaths - never
+// guardObjectPaths.
+func guardVersions(versions []FileInfoVersions, opts DeleteOptions) error {
+ if err := guardPaths(opts.OldDataDir); err != nil {
+ return err
+ }
+ for _, v := range versions {
+ if err := guardPaths(v.Name); err != nil {
+ return err
+ }
+ for _, fi := range v.Versions {
+ if err := guardPaths(fi.DataDir); err != nil {
+ return err
+ }
+ }
+ }
+ return nil
+}
+
+// ---------------------------------------------------------------------------
+// Metadata operations
+// ---------------------------------------------------------------------------
+
+func (g guardedStorage) DeleteVersion(ctx context.Context, volume, path string, fi FileInfo, forceDelMarker bool, opts DeleteOptions) error {
+ if err := guardPaths(path, fi.DataDir, opts.OldDataDir); err != nil {
+ return err
+ }
+ return g.StorageAPI.DeleteVersion(ctx, volume, path, fi, forceDelMarker, opts)
+}
+
+func (g guardedStorage) DeleteVersions(ctx context.Context, volume string, versions []FileInfoVersions, opts DeleteOptions) []error {
+ if err := guardVersions(versions, opts); err != nil {
+ // The whole batch is refused: validating every element before acting on
+ // any of it is what stops a payload mixing a valid target with a
+ // malicious one from performing a partial delete.
+ errs := make([]error, len(versions))
+ for i := range errs {
+ errs[i] = err
+ }
+ return errs
+ }
+ return g.StorageAPI.DeleteVersions(ctx, volume, versions, opts)
+}
+
+func (g guardedStorage) DeleteBulk(ctx context.Context, volume string, paths ...string) error {
+ if err := guardObjectPaths(paths...); err != nil {
+ return err
+ }
+ return g.StorageAPI.DeleteBulk(ctx, volume, paths...)
+}
+
+func (g guardedStorage) WriteMetadata(ctx context.Context, origvolume, volume, path string, fi FileInfo) error {
+ if err := guardPaths(path, fi.DataDir); err != nil {
+ return err
+ }
+ return g.StorageAPI.WriteMetadata(ctx, origvolume, volume, path, fi)
+}
+
+func (g guardedStorage) UpdateMetadata(ctx context.Context, volume, path string, fi FileInfo, opts UpdateMetadataOpts) error {
+ if err := guardPaths(path, fi.DataDir); err != nil {
+ return err
+ }
+ return g.StorageAPI.UpdateMetadata(ctx, volume, path, fi, opts)
+}
+
+func (g guardedStorage) ReadVersion(ctx context.Context, origvolume, volume, path, versionID string, opts ReadOptions) (FileInfo, error) {
+ if err := guardPaths(path); err != nil {
+ return FileInfo{}, err
+ }
+ return g.StorageAPI.ReadVersion(ctx, origvolume, volume, path, versionID, opts)
+}
+
+func (g guardedStorage) ReadXL(ctx context.Context, volume, path string, readData bool) (RawFileInfo, error) {
+ if err := guardPaths(path); err != nil {
+ return RawFileInfo{}, err
+ }
+ return g.StorageAPI.ReadXL(ctx, volume, path, readData)
+}
+
+func (g guardedStorage) RenameData(ctx context.Context, srcVolume, srcPath string, fi FileInfo, dstVolume, dstPath string, opts RenameOptions) (RenameDataResp, error) {
+ if err := guardObjectPaths(srcPath, dstPath); err != nil {
+ return RenameDataResp{}, err
+ }
+ // DataDir is empty for inline and transitioned objects, so it must not be
+ // held to the non-empty rule above.
+ if err := guardPaths(fi.DataDir); err != nil {
+ return RenameDataResp{}, err
+ }
+ return g.StorageAPI.RenameData(ctx, srcVolume, srcPath, fi, dstVolume, dstPath, opts)
+}
+
+// ---------------------------------------------------------------------------
+// File operations
+// ---------------------------------------------------------------------------
+
+func (g guardedStorage) ListDir(ctx context.Context, origvolume, volume, dirPath string, count int) ([]string, error) {
+ if err := guardPaths(dirPath); err != nil {
+ return nil, err
+ }
+ return g.StorageAPI.ListDir(ctx, origvolume, volume, dirPath, count)
+}
+
+func (g guardedStorage) ReadFile(ctx context.Context, volume, path string, offset int64, buf []byte, verifier *BitrotVerifier) (int64, error) {
+ if err := guardPaths(path); err != nil {
+ return 0, err
+ }
+ return g.StorageAPI.ReadFile(ctx, volume, path, offset, buf, verifier)
+}
+
+func (g guardedStorage) AppendFile(ctx context.Context, volume, path string, buf []byte) error {
+ if err := guardPaths(path); err != nil {
+ return err
+ }
+ return g.StorageAPI.AppendFile(ctx, volume, path, buf)
+}
+
+func (g guardedStorage) CreateFile(ctx context.Context, origvolume, volume, path string, size int64, reader io.Reader) error {
+ if err := guardPaths(path); err != nil {
+ return err
+ }
+ return g.StorageAPI.CreateFile(ctx, origvolume, volume, path, size, reader)
+}
+
+func (g guardedStorage) ReadFileStream(ctx context.Context, volume, path string, offset, length int64) (io.ReadCloser, error) {
+ if err := guardPaths(path); err != nil {
+ return nil, err
+ }
+ return g.StorageAPI.ReadFileStream(ctx, volume, path, offset, length)
+}
+
+func (g guardedStorage) RenameFile(ctx context.Context, srcVolume, srcPath, dstVolume, dstPath string) error {
+ if err := guardObjectPaths(srcPath, dstPath); err != nil {
+ return err
+ }
+ return g.StorageAPI.RenameFile(ctx, srcVolume, srcPath, dstVolume, dstPath)
+}
+
+func (g guardedStorage) RenamePart(ctx context.Context, srcVolume, srcPath, dstVolume, dstPath string, meta []byte, skipParent string) error {
+ if err := guardObjectPaths(srcPath, dstPath); err != nil {
+ return err
+ }
+ if err := guardPaths(skipParent); err != nil {
+ return err
+ }
+ return g.StorageAPI.RenamePart(ctx, srcVolume, srcPath, dstVolume, dstPath, meta, skipParent)
+}
+
+func (g guardedStorage) CheckParts(ctx context.Context, volume, path string, fi FileInfo) (*CheckPartsResp, error) {
+ if err := guardPaths(path, fi.DataDir); err != nil {
+ return nil, err
+ }
+ if err := guardErasureParams(fi); err != nil {
+ return nil, err
+ }
+ return g.StorageAPI.CheckParts(ctx, volume, path, fi)
+}
+
+func (g guardedStorage) Delete(ctx context.Context, volume, path string, opts DeleteOptions) error {
+ if err := guardPaths(path, opts.OldDataDir); err != nil {
+ return err
+ }
+ return g.StorageAPI.Delete(ctx, volume, path, opts)
+}
+
+func (g guardedStorage) VerifyFile(ctx context.Context, volume, path string, fi FileInfo) (*CheckPartsResp, error) {
+ if err := guardPaths(path, fi.DataDir); err != nil {
+ return nil, err
+ }
+ if err := guardErasureParams(fi); err != nil {
+ return nil, err
+ }
+ return g.StorageAPI.VerifyFile(ctx, volume, path, fi)
+}
+
+func (g guardedStorage) StatInfoFile(ctx context.Context, volume, path string, glob bool) ([]StatInfo, error) {
+ if err := guardPaths(path); err != nil {
+ return nil, err
+ }
+ return g.StorageAPI.StatInfoFile(ctx, volume, path, glob)
+}
+
+// ReadParts is a read, so it gets guardPaths rather than guardObjectPaths: the
+// volume-root alias rule exists because rename and delete *relocate or destroy*
+// the root, whereas a read of it merely fails to find part.N. Applying the
+// stricter rule here would widen the false-positive surface for no gain.
+func (g guardedStorage) ReadParts(ctx context.Context, bucket string, partMetaPaths ...string) ([]*ObjectPartInfo, error) {
+ if err := guardPaths(partMetaPaths...); err != nil {
+ return nil, err
+ }
+ return g.StorageAPI.ReadParts(ctx, bucket, partMetaPaths...)
+}
+
+func (g guardedStorage) CleanAbandonedData(ctx context.Context, volume, path string) error {
+ if err := guardPaths(path); err != nil {
+ return err
+ }
+ return g.StorageAPI.CleanAbandonedData(ctx, volume, path)
+}
+
+func (g guardedStorage) WriteAll(ctx context.Context, volume, path string, b []byte) error {
+ if err := guardPaths(path); err != nil {
+ return err
+ }
+ return g.StorageAPI.WriteAll(ctx, volume, path, b)
+}
+
+func (g guardedStorage) ReadAll(ctx context.Context, volume, path string) ([]byte, error) {
+ if err := guardPaths(path); err != nil {
+ return nil, err
+ }
+ return g.StorageAPI.ReadAll(ctx, volume, path)
+}
+
+// ---------------------------------------------------------------------------
+// Directory walks and scanning
+// ---------------------------------------------------------------------------
+
+func (g guardedStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writer) error {
+ // FilterPrefix and ForwardTo are only string-compared against readDir
+ // output, they never reach a path join.
+ if err := guardPaths(opts.Bucket, opts.BaseDir); err != nil {
+ return err
+ }
+ return g.StorageAPI.WalkDir(ctx, opts, wr)
+}
+
+// NSScanner is the one StorageAPI method that reaches the filesystem without
+// passing through getVolDir: the scanner joins cache.Info.Name onto drivePath
+// directly (see scanFolder). It is therefore guarded here rather than at the
+// sink. Today an unregistered bucket name is also rejected further down by
+// globalBucketObjectLockSys, but that is a lookup in an unrelated subsystem,
+// not a containment boundary.
+func (g guardedStorage) NSScanner(ctx context.Context, cache dataUsageCache, updates chan<- dataUsageEntry, scanMode madmin.HealScanMode, shouldSleep func() bool) (dataUsageCache, error) {
+ if err := guardPaths(cache.Info.Name); err != nil {
+ // The caller blocks until updates is closed; NSScanner owns closing it.
+ xioutil.SafeClose(updates)
+ return cache, err
+ }
+ return g.StorageAPI.NSScanner(ctx, cache, updates, scanMode, shouldSleep)
+}
diff --git a/cmd/storage-rest-server-guard_test.go b/cmd/storage-rest-server-guard_test.go
new file mode 100644
index 000000000..152b2dcdf
--- /dev/null
+++ b/cmd/storage-rest-server-guard_test.go
@@ -0,0 +1,376 @@
+// Copyright (c) 2015-2026 MinIO, Inc.
+//
+// This file is part of MinIO Object Storage stack
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program. If not, see .
+
+package cmd
+
+import (
+ "bytes"
+ "context"
+ "errors"
+ "io"
+ "reflect"
+ "runtime"
+ "strings"
+ "testing"
+
+ "github.com/minio/minio-go/v7/pkg/s3utils"
+)
+
+// poisonStorage implements StorageAPI with a nil embedded interface, so any
+// call that reaches it panics. guardedStorage must never delegate a traversing
+// path to it.
+type poisonStorage struct {
+ StorageAPI
+}
+
+// volumeOnlyOrPathless lists the StorageAPI methods guardedStorage
+// deliberately does not override, with the reason each is safe.
+//
+// Adding a method to StorageAPI without either guarding it or adding it here
+// with a reason makes TestGuardedStorageCoversEveryPathMethod fail.
+var volumeOnlyOrPathless = map[string]string{
+ // No filesystem path in the signature at all.
+ "String": "identity", "IsOnline": "status", "LastConn": "status",
+ "IsLocal": "topology", "Hostname": "topology", "Endpoint": "topology",
+ "Close": "lifecycle", "GetDiskID": "identity", "SetDiskID": "identity",
+ "Healing": "status", "GetDiskLoc": "topology", "DiskInfo": "no path field",
+ "ListVols": "no argument",
+
+ // Volume-only. xlStorage.getVolDir validates the volume at the sink, which
+ // also covers the peer-S3 callers that never pass through this wrapper.
+ "MakeVol": "volume-only, guarded by getVolDir",
+ "MakeVolBulk": "volume-only, guarded by getVolDir",
+ "StatVol": "volume-only, guarded by getVolDir",
+ "DeleteVol": "volume-only, guarded by getVolDir",
+}
+
+// TestGuardedStorageCoversEveryPathMethod calls every StorageAPI method on a
+// guardedStorage whose embedded storage panics, passing "../evil" in every
+// string it can reach - including strings nested inside structs and slices,
+// which is where a hand-written check is most likely to miss one.
+//
+// A method that returns without panicking rejected the path. A method that
+// panics delegated it.
+func TestGuardedStorageCoversEveryPathMethod(t *testing.T) {
+ g := reflect.ValueOf(guardedStorage{poisonStorage{}})
+ iface := reflect.TypeOf((*StorageAPI)(nil)).Elem()
+
+ for i := range iface.NumMethod() {
+ name := iface.Method(i).Name
+ if reason, ok := volumeOnlyOrPathless[name]; ok {
+ t.Logf("skipping %s: %s", name, reason)
+ continue
+ }
+ m := g.MethodByName(name)
+ if !m.IsValid() {
+ t.Errorf("%s: not found on guardedStorage", name)
+ continue
+ }
+ mt := m.Type()
+ args := make([]reflect.Value, mt.NumIn())
+ for j := range args {
+ args[j] = poisonArg(t, mt.In(j))
+ }
+ t.Run(name, func(t *testing.T) {
+ defer func() {
+ if r := recover(); r != nil {
+ t.Errorf("%s delegated a traversing path to the underlying storage "+
+ "- it is not guarded. Add a guardedStorage override, or add it to "+
+ "volumeOnlyOrPathless with a reason. (%v)", name, r)
+ }
+ }()
+ if mt.IsVariadic() {
+ m.CallSlice(args)
+ } else {
+ m.Call(args)
+ }
+ })
+ }
+}
+
+// TestGuardChecksUnreachableFields pins the guard on path fields that exist on
+// the wire but that no handler currently reads, so an end-to-end test cannot
+// observe them. DeleteVersionHandler hardcodes `opts := DeleteOptions{}` and
+// discards p.Opts, which means DeleteOptions.OldDataDir - a value that reaches
+// renameAll() with no containment of its own - is unreachable by accident
+// rather than by design. If someone later plumbs p.Opts through, the guard is
+// already there; this test is what stops it from being removed as "dead".
+func TestGuardChecksUnreachableFields(t *testing.T) {
+ g := guardedStorage{poisonStorage{}}
+ ctx := context.Background()
+
+ if err := g.DeleteVersion(ctx, "foo", "obj", FileInfo{}, false,
+ DeleteOptions{OldDataDir: poisonPath}); !errors.Is(err, errFileAccessDenied) {
+ t.Errorf("DeleteVersion with a traversing OldDataDir: got %v, want %v", err, errFileAccessDenied)
+ }
+
+ errs := g.DeleteVersions(ctx, "foo", []FileInfoVersions{{Name: "obj"}},
+ DeleteOptions{OldDataDir: poisonPath})
+ if len(errs) != 1 || !errors.Is(errs[0], errFileAccessDenied) {
+ t.Errorf("DeleteVersions with a traversing OldDataDir: got %v, want %v", errs, errFileAccessDenied)
+ }
+
+ if err := g.Delete(ctx, "foo", "obj", DeleteOptions{OldDataDir: poisonPath}); !errors.Is(err, errFileAccessDenied) {
+ t.Errorf("Delete with a traversing OldDataDir: got %v, want %v", err, errFileAccessDenied)
+ }
+}
+
+const poisonPath = "../evil"
+
+func poisonArg(t *testing.T, typ reflect.Type) reflect.Value {
+ t.Helper()
+ switch typ {
+ case reflect.TypeOf((*context.Context)(nil)).Elem():
+ return reflect.ValueOf(context.Background())
+ case reflect.TypeOf((*io.Writer)(nil)).Elem():
+ return reflect.ValueOf(io.Discard)
+ case reflect.TypeOf((*io.Reader)(nil)).Elem():
+ return reflect.ValueOf(bytes.NewReader(nil))
+ }
+ if typ.Kind() == reflect.Chan {
+ // NSScanner's updates channel: the guard is required to close it.
+ return reflect.MakeChan(reflect.ChanOf(reflect.BothDir, typ.Elem()), 1).Convert(typ)
+ }
+ return poisonValue(typ, 0)
+}
+
+// poisonValue builds a value of typ with every settable string set to
+// poisonPath, recursing into structs, slices and pointers.
+func poisonValue(typ reflect.Type, depth int) reflect.Value {
+ v := reflect.New(typ).Elem()
+ if depth > 4 {
+ return v
+ }
+ switch typ.Kind() {
+ case reflect.String:
+ v.SetString(poisonPath)
+ case reflect.Struct:
+ for i := range typ.NumField() {
+ f := v.Field(i)
+ if !f.CanSet() {
+ continue // unexported
+ }
+ // Skip self-referential and container types we cannot poison
+ // meaningfully; the fields that matter are plain strings.
+ switch f.Kind() {
+ case reflect.Map, reflect.Chan, reflect.Func, reflect.Interface, reflect.UnsafePointer:
+ continue
+ }
+ f.Set(poisonValue(f.Type(), depth+1))
+ }
+ case reflect.Slice:
+ s := reflect.MakeSlice(typ, 1, 1)
+ s.Index(0).Set(poisonValue(typ.Elem(), depth+1))
+ v.Set(s)
+ case reflect.Pointer:
+ p := reflect.New(typ.Elem())
+ p.Elem().Set(poisonValue(typ.Elem(), depth+1))
+ v.Set(p)
+ }
+ return v
+}
+
+func TestIsVolumeRootAlias(t *testing.T) {
+ for _, tc := range []struct {
+ path string
+ want bool
+ }{
+ // Collapse back to the volume directory.
+ {"", true}, {"/", true}, {"//", true},
+ // Backslash is platform-dependent; see TestIsVolumeRootAliasIsPlatformCorrect.
+ // Whitespace is NOT a separator. path.Clean leaves it alone, so these
+ // name real directories and are legal S3 object keys.
+ {" ", false}, {" ", false}, {"\t", false}, {"\n", false},
+ {" / ", false}, {"/ ", false},
+ {"a", false}, {"/a", false}, {"..", false}, {" a ", false},
+ {"obj/part.1", false},
+ } {
+ if got := isVolumeRootAlias(tc.path); got != tc.want {
+ t.Errorf("isVolumeRootAlias(%q) = %v, want %v", tc.path, got, tc.want)
+ }
+ }
+}
+
+// guardMayRefuse reports whether the guards are permitted to refuse a name that
+// IsValidObjectName accepts. On Unix the answer is never: no legal object name
+// is made up entirely of '/', so the invariant below carries NO exceptions.
+//
+// Stated in terms of the separator set rather than by calling isVolumeRootAlias,
+// so that widening that function cannot silently widen what the test forgives.
+// Two regressions have already hidden in exactly that gap: whitespace was once
+// counted as a separator (refusing the legal key " "), and an earlier version
+// of this helper excused every backslash-only name on every platform, which is
+// why the fuzzer could not see that bug at all.
+func guardMayRefuse(name string) bool {
+ if runtime.GOOS != globalWindowsOSName {
+ return false
+ }
+ // Mirrors the Windows branch of isVolumeRootAliasOn: separators plus the
+ // characters Win32 strips from a component.
+ return strings.Trim(name, "/\\ .") == ""
+}
+
+func TestIsVolumeRootAliasIsPlatformCorrect(t *testing.T) {
+ for _, tc := range []struct {
+ path string
+ unix, windowsWant bool
+ }{
+ {"", true, true},
+ {"/", true, true},
+ {"//", true, true},
+ // Backslash: an ordinary filename on Unix, a separator on Windows.
+ {"\\", false, true},
+ {"\\\\", false, true},
+ {"/\\", false, true},
+ // Space and period: ordinary filename characters on Unix, but stripped
+ // from a component by Win32 normalisation, so a component made only of
+ // them vanishes and the path resolves to the volume root.
+ {" ", false, true}, {" ", false, true}, {" / ", false, true},
+ {"...", false, true}, {". .", false, true},
+ // Never an alias anywhere.
+ {"\t", false, false}, {"\n", false, false},
+ {"a", false, false}, {"/a", false, false}, {"a ", false, false},
+ {" a", false, false}, {"a.", false, false},
+ } {
+ if got := isVolumeRootAliasOn(tc.path, false); got != tc.unix {
+ t.Errorf("isVolumeRootAliasOn(%q, unix) = %v, want %v", tc.path, got, tc.unix)
+ }
+ if got := isVolumeRootAliasOn(tc.path, true); got != tc.windowsWant {
+ t.Errorf("isVolumeRootAliasOn(%q, windows) = %v, want %v", tc.path, got, tc.windowsWant)
+ }
+ }
+}
+
+// TestGuardAcceptsEveryLegalObjectName pins the invariant that actually matters
+// for availability: if S3 accepts a name, the guards must accept it too.
+//
+// A guard that rejects a legal object name breaks writes on every remote drive
+// simultaneously, which fails quorum - a worse outage than the vulnerability it
+// defends against. This caught a real regression: isVolumeRootAlias originally
+// treated whitespace as a separator, so a legal key of " " was refused on the
+// PutObject commit path.
+func TestGuardAcceptsEveryLegalObjectName(t *testing.T) {
+ names := []string{
+ // Whitespace, in every position. All legal in S3.
+ " ", " ", "\t", "\n", "a b", " a", "a ", " a ", " / ", "/ ",
+ // Dots that are not "." or ".." segments.
+ "..foo", "foo..", ".hidden", "a/..b", "a/b..", "a.b/c..d",
+ // "..." is a legal key on Unix and accepted; on Windows it is a volume-root
+ // alias (Win32 strips trailing periods) and guardMayRefuse excuses it there.
+ "part.1.meta", "...", "a/...",
+ // Ordinary shapes.
+ "a", "/a", "a/b/c", "obj/part.1", "__XLDIR__",
+ "unicode/ๆไปถ/ๅ", "emoji/๐", "pct%2e%2e/x",
+ // Long-ish and punctuation-heavy.
+ "a-b_c+d=e,f:g;h@i", "x!y'z(1)2*3",
+ // Backslashes: ordinary filename characters on Unix.
+ "\\", "\\\\", "/\\", "a\\b", "\\a", "a\\",
+ }
+ for _, name := range names {
+ if !IsValidObjectName(name) {
+ continue // S3 refuses it first; the guard is free to as well.
+ }
+ t.Run(name, func(t *testing.T) {
+ if err := guardPaths(name); err != nil {
+ t.Errorf("guardPaths(%q) = %v, but S3 accepts this object name", name, err)
+ }
+ if err := guardObjectPaths(name); err != nil {
+ if guardMayRefuse(name) {
+ t.Logf("allowed on this platform (%q): separator-only, addresses the volume root", name)
+ return
+ }
+ t.Errorf("guardObjectPaths(%q) = %v, but S3 accepts this object name. "+
+ "A guard that refuses a legal key fails writes on every drive at once.", name, err)
+ }
+ })
+ }
+}
+
+// FuzzGuardAcceptsLegalObjectNames searches for more of the above. The property
+// is one-directional on purpose: we assert the guards never refuse something S3
+// allows, and say nothing about names S3 already refuses.
+func FuzzGuardAcceptsLegalObjectNames(f *testing.F) {
+ for _, seed := range []string{" ", " ", "\t", "a b", "..foo", "a/..b", "/", "", "\\", "\\\\\\", "a/../b"} {
+ f.Add(seed)
+ }
+ f.Fuzz(func(t *testing.T, name string) {
+ if !IsValidObjectName(name) {
+ return
+ }
+ if guardMayRefuse(name) {
+ return
+ }
+ if err := guardPaths(name); err != nil {
+ t.Fatalf("guardPaths(%q) = %v, but IsValidObjectName accepts it", name, err)
+ }
+ if err := guardObjectPaths(name); err != nil {
+ t.Fatalf("guardObjectPaths(%q) = %v, but IsValidObjectName accepts it", name, err)
+ }
+ })
+}
+
+// FuzzGetVolDirAcceptsLegalBucketNames is the volume-axis twin of the object
+// invariant above. G-A adds hasBadPathComponent to getVolDir, so any bucket
+// name S3 accepts must survive it, or MakeBucket/HeadBucket/DeleteBucket start
+// failing for legitimate buckets across the whole cluster.
+func FuzzGetVolDirAcceptsLegalBucketNames(f *testing.F) {
+ for _, seed := range []string{"bucket", "my-bucket", "a.b.c", "1234", "x--y", "..", "a..b"} {
+ f.Add(seed)
+ }
+ f.Fuzz(func(t *testing.T, name string) {
+ if s3utils.CheckValidBucketName(name) != nil {
+ return // S3 refuses it first; getVolDir is free to as well.
+ }
+ if hasBadPathComponent(name) {
+ t.Fatalf("getVolDir would reject %q, but s3utils.CheckValidBucketName accepts it", name)
+ }
+ })
+}
+
+// TestGetVolDirAcceptsReservedVolumes covers the volume names the server uses
+// internally, which are not buckets and so are not covered by the fuzz above.
+func TestGetVolDirAcceptsReservedVolumes(t *testing.T) {
+ for _, vol := range []string{
+ minioMetaBucket, minioMetaTmpBucket, minioMetaTmpDeletedBucket,
+ minioMetaMultipartBucket, minioReservedBucket,
+ pathJoin(minioMetaBucket, bucketMetaPrefix),
+ pathJoin(minioMetaBucket, bucketMetaPrefix, deletedBucketsPrefix, "some-bucket"),
+ } {
+ if hasBadPathComponent(vol) {
+ t.Errorf("getVolDir would reject the reserved volume %q", vol)
+ }
+ }
+}
+
+func TestGuardPathsAcceptsReservedNames(t *testing.T) {
+ // Reserved volumes and directories contain dot-prefixed segments. Only an
+ // exact "." or ".." segment is a traversal.
+ for _, p := range []string{
+ "", minioMetaBucket, minioMetaTmpBucket, minioMetaTmpDeletedBucket,
+ minioMetaMultipartBucket, ".deleted", "obj/part.1.meta",
+ "a.b/c..d", "__XLDIR__", "bucket/.metacache/x.s2",
+ } {
+ if err := guardPaths(p); err != nil {
+ t.Errorf("guardPaths(%q) = %v, want nil", p, err)
+ }
+ }
+ for _, p := range traversalPaths {
+ if err := guardPaths(p); err == nil {
+ t.Errorf("guardPaths(%q) = nil, want %v", p, errFileAccessDenied)
+ }
+ }
+}
diff --git a/cmd/storage-rest-server.go b/cmd/storage-rest-server.go
index d7ee11493..dc7eb29bb 100644
--- a/cmd/storage-rest-server.go
+++ b/cmd/storage-rest-server.go
@@ -75,6 +75,10 @@ var (
storageListDirRPC = grid.NewStream[*grid.MSS, grid.NoPayload, *ListDirResult](grid.HandlerListDir, grid.NewMSS, nil, func() *ListDirResult { return &ListDirResult{} }).WithOutCapacity(1)
)
+// getStorageViaEndpoint returns the drive UNGUARDED. It is for local callers
+// only, whose arguments the server itself constructed. Anything serving a
+// remote peer must go through storageRESTServer.getStorage(), which wraps this
+// in guardedStorage to reject traversal in wire-supplied paths.
func getStorageViaEndpoint(endpoint Endpoint) StorageAPI {
globalLocalDrivesMu.RLock()
defer globalLocalDrivesMu.RUnlock()
@@ -85,7 +89,18 @@ func getStorageViaEndpoint(endpoint Endpoint) StorageAPI {
}
func (s *storageRESTServer) getStorage() StorageAPI {
- return getStorageViaEndpoint(s.endpoint)
+ st := getStorageViaEndpoint(s.endpoint)
+ if st == nil {
+ // Must stay an untyped nil. IsAuthValid and checkID compare the result
+ // against nil before authenticating, and a nil interface wrapped in a
+ // value struct does not compare equal to nil - that would turn an
+ // unauthenticated request against a drive that has not come up yet
+ // into a nil dereference.
+ return nil
+ }
+ // Reject traversal in paths carried by request bodies and grid RPC frames,
+ // neither of which the global HTTP middleware can see. See guardedStorage.
+ return guardedStorage{st}
}
func (s *storageRESTServer) writeErrorResponse(w http.ResponseWriter, err error) {
diff --git a/cmd/storage-rest-traversal_test.go b/cmd/storage-rest-traversal_test.go
new file mode 100644
index 000000000..49607c408
--- /dev/null
+++ b/cmd/storage-rest-traversal_test.go
@@ -0,0 +1,525 @@
+// Copyright (c) 2015-2026 MinIO, Inc.
+//
+// This file is part of MinIO Object Storage stack
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program. If not, see .
+
+package cmd
+
+import (
+ "errors"
+ "io"
+ "os"
+ "path/filepath"
+ "runtime"
+ "testing"
+
+ "github.com/minio/madmin-go/v3"
+)
+
+// Paths that must never be accepted from an internode payload. Each is a
+// distinct evasion of the segment scanner: forward slash, backslash (Windows
+// resolves these, path.Clean does not), whitespace padding, and single-dot.
+var traversalPaths = []string{
+ "../evil",
+ "../../evil",
+ "a/../../evil",
+ "..\\evil",
+ "a\\..\\..\\evil",
+ " .. /evil",
+ "../",
+ "./../evil",
+}
+
+// Paths that alias the volume root itself: pathJoin collapses each of these
+// back to the volume directory, so renaming or deleting one relocates or
+// destroys the whole volume, with no ".." required.
+//
+// Only the platform-independent forms live here. Whitespace does NOT belong:
+// path.Clean leaves spaces alone, so " " names a real directory inside the
+// volume and is a legal S3 object key. Backslash forms do not belong either:
+// they collapse on Windows but name an ordinary file on Unix. Both classes are
+// covered by TestIsVolumeRootAliasIsPlatformCorrect and, from the other
+// direction, by TestGuardAcceptsEveryLegalObjectName.
+var volumeRootAliases = []string{"", "/", "//"}
+
+// sentinels plants files we can prove were neither read nor removed.
+type sentinels struct {
+ drive string // drive root
+ outside string // file outside the drive root entirely
+ sibling string // file in a sibling volume
+ legit string // a legitimate object inside "foo"
+ partDir string // a readable part dir in the sibling volume
+ outsideRe string // path a rename/write would land on, outside the drive
+}
+
+func plantSentinels(t *testing.T, drive string) *sentinels {
+ t.Helper()
+ s := &sentinels{
+ drive: drive,
+ outside: filepath.Join(filepath.Dir(drive), "sentinel-outside.txt"),
+ sibling: filepath.Join(drive, "bar", "sentinel-sibling.txt"),
+ legit: filepath.Join(drive, "foo", "legit.txt"),
+ partDir: filepath.Join(drive, "bar", "obj"),
+ outsideRe: filepath.Join(filepath.Dir(drive), "sentinel-landing.txt"),
+ }
+ mustWrite(t, s.outside, "OUTSIDE-SECRET")
+ t.Cleanup(func() { os.Remove(s.outside); os.Remove(s.outsideRe) })
+ mustWrite(t, s.sibling, "SIBLING-SECRET")
+ mustWrite(t, s.legit, "LEGIT")
+
+ if err := os.MkdirAll(s.partDir, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ mustWrite(t, filepath.Join(s.partDir, "part.1"), "data")
+ blob, err := (&ObjectPartInfo{Number: 1, Size: 4, ETag: "SIBLING-ETAG"}).MarshalMsg(nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := os.WriteFile(filepath.Join(s.partDir, "part.1.meta"), blob, 0o644); err != nil {
+ t.Fatal(err)
+ }
+ return s
+}
+
+func mustWrite(t *testing.T, path, content string) {
+ t.Helper()
+ if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.WriteFile(path, []byte(content), 0o644); err != nil {
+ t.Fatal(err)
+ }
+}
+
+// assertIntact fails if any sentinel was removed, modified, or if a file
+// appeared where a traversal would have landed.
+func (s *sentinels) assertIntact(t *testing.T, op string) {
+ t.Helper()
+ for _, f := range []struct{ path, want string }{
+ {s.outside, "OUTSIDE-SECRET"},
+ {s.sibling, "SIBLING-SECRET"},
+ {s.legit, "LEGIT"},
+ } {
+ got, err := os.ReadFile(f.path)
+ if err != nil {
+ t.Errorf("%s: sentinel %s was destroyed: %v", op, f.path, err)
+ continue
+ }
+ if string(got) != f.want {
+ t.Errorf("%s: sentinel %s was modified: got %q want %q", op, f.path, got, f.want)
+ }
+ }
+ if _, err := os.Stat(s.outsideRe); err == nil {
+ t.Errorf("%s: a file was created outside the drive root at %s", op, s.outsideRe)
+ }
+ for _, vol := range []string{"foo", "bar"} {
+ st, err := os.Stat(filepath.Join(s.drive, vol))
+ if err != nil || !st.IsDir() {
+ t.Errorf("%s: volume %q no longer exists as a directory (err=%v)", op, vol, err)
+ }
+ }
+}
+
+// assertDenied requires the specific sentinel error, not merely "some error".
+// An operation on a nonexistent path fails anyway, so a bare err != nil check
+// passes even when the guard is absent.
+func assertDenied(t *testing.T, op string, err error) {
+ t.Helper()
+ if err == nil {
+ t.Errorf("%s: expected %v, got nil", op, errFileAccessDenied)
+ return
+ }
+ if !errors.Is(err, errFileAccessDenied) {
+ t.Errorf("%s: expected %v, got %v (%T)", op, errFileAccessDenied, err, err)
+ }
+}
+
+// TestStorageRESTTraversalRejected drives the real REST/grid client against a
+// real xlStorage and proves that no internode payload can read, write, move or
+// delete anything outside its own volume.
+//
+// NOTE: the grid.SetupTestGrid harness mounts a bare mux router with none of
+// globalMiddlewares, so a green run says nothing about the production HTTP
+// query-argument path (which setRequestValidityMiddleware already covers). It
+// measures the storage-layer guards and nothing else, which is the point.
+func TestStorageRESTTraversalRejected(t *testing.T) {
+ restClient := newStorageRESTHTTPServerClient(t)
+ drive := globalLocalSetDrives[0][0][0].Endpoint().Path
+ s := plantSentinels(t, drive)
+ ctx := t.Context()
+
+ badFI := func(dataDir string) FileInfo {
+ return FileInfo{Volume: "foo", Name: "obj", DataDir: dataDir, ModTime: UTCNow()}
+ }
+
+ // Every operation that accepts a path from the wire, driven with each
+ // traversal form. The op must be denied AND must not have touched disk.
+ for _, p := range traversalPaths {
+ ops := []struct {
+ name string
+ run func() error
+ }{
+ {"ReadAll", func() error { _, err := restClient.ReadAll(ctx, "foo", p); return err }},
+ {"WriteAll", func() error { return restClient.WriteAll(ctx, "foo", p, []byte("PWNED")) }},
+ {"ReadXL", func() error { _, err := restClient.ReadXL(ctx, "foo", p, true); return err }},
+ {"Delete", func() error {
+ return restClient.Delete(ctx, "foo", p, DeleteOptions{Recursive: true, Immediate: true})
+ }},
+ {"DeleteBulk", func() error { return restClient.DeleteBulk(ctx, "foo", p) }},
+ {"ReadParts", func() error { _, err := restClient.ReadParts(ctx, "foo", p); return err }},
+ {"StatInfoFile", func() error { _, err := restClient.StatInfoFile(ctx, "foo", p, false); return err }},
+ {"CleanAbandonedData", func() error { return restClient.CleanAbandonedData(ctx, "foo", p) }},
+ {"ListDir", func() error { _, err := restClient.ListDir(ctx, "", "foo", p, -1); return err }},
+ {"RenameFile-src", func() error { return restClient.RenameFile(ctx, "foo", p, "foo", "dst.txt") }},
+ {"RenameFile-dst", func() error { return restClient.RenameFile(ctx, "foo", "legit.txt", "foo", p) }},
+ {"RenamePart-src", func() error {
+ return restClient.RenamePart(ctx, "foo", p, "foo", "dst.txt", nil, "")
+ }},
+ {"RenamePart-dst", func() error {
+ return restClient.RenamePart(ctx, "foo", "legit.txt", "foo", p, nil, "")
+ }},
+ {"RenamePart-skipParent", func() error {
+ return restClient.RenamePart(ctx, "foo", "legit.txt", "foo", "dst.txt", nil, p)
+ }},
+ {"CheckParts", func() error { _, err := restClient.CheckParts(ctx, "foo", p, badFI("")); return err }},
+ {"CheckParts-DataDir", func() error {
+ _, err := restClient.CheckParts(ctx, "foo", "obj", badFI(p))
+ return err
+ }},
+ {"VerifyFile", func() error { _, err := restClient.VerifyFile(ctx, "foo", p, badFI("")); return err }},
+ {"VerifyFile-DataDir", func() error {
+ _, err := restClient.VerifyFile(ctx, "foo", "obj", badFI(p))
+ return err
+ }},
+ {"WriteMetadata", func() error { return restClient.WriteMetadata(ctx, "", "foo", p, badFI("")) }},
+ {"UpdateMetadata", func() error {
+ return restClient.UpdateMetadata(ctx, "foo", p, badFI(""), UpdateMetadataOpts{})
+ }},
+ {"DeleteVersion", func() error {
+ return restClient.DeleteVersion(ctx, "foo", p, badFI(""), false, DeleteOptions{})
+ }},
+
+ // Nested path-bearing fields, poisoned one at a time. A method that
+ // validates its `path` argument but forgets one of these still
+ // passes every check above, so each needs its own case.
+ {"WriteMetadata-DataDir", func() error {
+ return restClient.WriteMetadata(ctx, "", "foo", "obj", badFI(p))
+ }},
+ {"UpdateMetadata-DataDir", func() error {
+ return restClient.UpdateMetadata(ctx, "foo", "obj", badFI(p), UpdateMetadataOpts{})
+ }},
+ {"DeleteVersion-DataDir", func() error {
+ return restClient.DeleteVersion(ctx, "foo", "obj", badFI(p), false, DeleteOptions{})
+ }},
+ // DeleteOptions.OldDataDir is guarded too, but cannot be exercised
+ // from here: DeleteVersionHandler hardcodes `opts := DeleteOptions{}`
+ // and never reads the wire value, so the field is unreachable today.
+ // That is an accidental mitigation, not a control - see
+ // TestGuardChecksUnreachableFields for the unit-level assertion that
+ // the guard is ready if anyone ever plumbs it through.
+ {"RenameData-srcPath", func() error {
+ _, err := restClient.RenameData(ctx, "foo", p, badFI(""), "bar", "dst", RenameOptions{})
+ return err
+ }},
+ {"RenameData-dstPath", func() error {
+ _, err := restClient.RenameData(ctx, "foo", "src", badFI(""), "bar", p, RenameOptions{})
+ return err
+ }},
+ {"RenameData-DataDir", func() error {
+ _, err := restClient.RenameData(ctx, "foo", "src", badFI(p), "bar", "dst", RenameOptions{})
+ return err
+ }},
+ }
+ for _, op := range ops {
+ t.Run(op.name+"/"+p, func(t *testing.T) {
+ assertDenied(t, op.name, op.run())
+ s.assertIntact(t, op.name)
+ })
+ }
+
+ // The nested DataDir of each version is a separate field from the
+ // per-object Name; poison them independently so neither can regress
+ // behind the other.
+ t.Run("DeleteVersions-VersionDataDir/"+p, func(t *testing.T) {
+ errs := restClient.DeleteVersions(ctx, "foo",
+ []FileInfoVersions{{Name: "obj", Versions: []FileInfo{{Name: "obj", DataDir: p}}}},
+ DeleteOptions{})
+ if len(errs) != 1 {
+ t.Fatalf("expected 1 error, got %d", len(errs))
+ }
+ assertDenied(t, "DeleteVersions-VersionDataDir", errs[0])
+ s.assertIntact(t, "DeleteVersions-VersionDataDir")
+ })
+
+ t.Run("DeleteVersions/"+p, func(t *testing.T) {
+ errs := restClient.DeleteVersions(ctx, "foo",
+ []FileInfoVersions{{Name: p, Versions: []FileInfo{{Name: p}}}}, DeleteOptions{})
+ if len(errs) != 1 {
+ t.Fatalf("expected 1 error, got %d", len(errs))
+ }
+ assertDenied(t, "DeleteVersions", errs[0])
+ s.assertIntact(t, "DeleteVersions")
+ })
+
+ // WalkDir and NSScanner take their paths inside an options/cache struct
+ // rather than as arguments, which is exactly where a per-handler check
+ // tends to miss them.
+ t.Run("WalkDir/"+p, func(t *testing.T) {
+ for _, opts := range []WalkDirOptions{
+ {Bucket: p, BaseDir: "obj"},
+ {Bucket: "foo", BaseDir: p},
+ } {
+ if err := restClient.WalkDir(ctx, opts, io.Discard); err == nil {
+ t.Errorf("WalkDir(%+v) returned nil", opts)
+ }
+ }
+ s.assertIntact(t, "WalkDir")
+ })
+
+ t.Run("NSScanner/"+p, func(t *testing.T) {
+ cache := dataUsageCache{Info: dataUsageCacheInfo{Name: p}}
+ updates := make(chan dataUsageEntry, 1)
+ if _, err := restClient.NSScanner(ctx, cache, updates, madmin.HealNormalScan, nil); err == nil {
+ t.Errorf("NSScanner(cache.Info.Name=%q) returned nil", p)
+ }
+ s.assertIntact(t, "NSScanner")
+ })
+
+ t.Run("ReadAll-volume/"+p, func(t *testing.T) {
+ // Traversal smuggled through the volume argument rather than the path.
+ buf, err := restClient.ReadAll(ctx, p, "sentinel-outside.txt")
+ if err == nil {
+ t.Errorf("ReadAll with volume %q: expected error, got nil (read %d bytes)", p, len(buf))
+ }
+ if string(buf) == "OUTSIDE-SECRET" {
+ t.Errorf("ReadAll with volume %q leaked a file outside the drive root", p)
+ }
+ s.assertIntact(t, "ReadAll-volume")
+ })
+ }
+
+ // Volume-root aliases: no ".." involved, but a rename or bulk delete of the
+ // volume root relocates or destroys the entire volume.
+ for _, p := range volumeRootAliases {
+ t.Run("DeleteBulk-root/"+p, func(t *testing.T) {
+ assertDenied(t, "DeleteBulk", restClient.DeleteBulk(ctx, "foo", p))
+ s.assertIntact(t, "DeleteBulk-root")
+ })
+ t.Run("RenameFile-root/"+p, func(t *testing.T) {
+ assertDenied(t, "RenameFile", restClient.RenameFile(ctx, "foo", p, "bar", "captured"))
+ s.assertIntact(t, "RenameFile-root")
+ })
+ }
+
+ // A batch mixing a legitimate target with a malicious one must delete
+ // neither -- validate the whole set before acting on any of it.
+ t.Run("MixedBatch", func(t *testing.T) {
+ assertDenied(t, "DeleteBulk-mixed",
+ restClient.DeleteBulk(ctx, "foo", "legit.txt", "../bar/sentinel-sibling.txt"))
+ s.assertIntact(t, "DeleteBulk-mixed")
+ })
+
+ t.Run("ReadParts-mixed", func(t *testing.T) {
+ _, err := restClient.ReadParts(ctx, "foo", "legit.txt", "../bar/obj/part.1.meta")
+ assertDenied(t, "ReadParts-mixed", err)
+ s.assertIntact(t, "ReadParts-mixed")
+ })
+}
+
+// TestStorageRESTLegitimateOpsStillWork is the positive control: the guards must
+// not reject anything the cluster does in normal operation.
+func TestStorageRESTLegitimateOpsStillWork(t *testing.T) {
+ restClient := newStorageRESTHTTPServerClient(t)
+ drive := globalLocalSetDrives[0][0][0].Endpoint().Path
+ ctx := t.Context()
+
+ mustWrite(t, filepath.Join(drive, "foo", "a.txt"), "HELLO")
+ mustWrite(t, filepath.Join(drive, minioMetaBucket, "tmp", "sys.txt"), "SYS")
+
+ if err := restClient.WriteAll(ctx, "foo", "written.txt", []byte("DATA")); err != nil {
+ t.Fatalf("WriteAll on a legitimate path: %v", err)
+ }
+ if b, err := restClient.ReadAll(ctx, "foo", "written.txt"); err != nil || string(b) != "DATA" {
+ t.Fatalf("ReadAll on a legitimate path: %q %v", b, err)
+ }
+ // Reserved volumes contain dot-prefixed segments (".minio.sys", ".trash")
+ // which must remain acceptable -- only exact "." and ".." segments are bad.
+ if b, err := restClient.ReadAll(ctx, minioMetaBucket, "tmp/sys.txt"); err != nil || string(b) != "SYS" {
+ t.Fatalf("ReadAll on %s: %q %v", minioMetaBucket, b, err)
+ }
+ if err := restClient.RenameFile(ctx, "foo", "a.txt", "foo", "b.txt"); err != nil {
+ t.Fatalf("RenameFile on legitimate paths: %v", err)
+ }
+ if _, err := restClient.ListDir(ctx, "", "foo", "", -1); err != nil {
+ t.Fatalf("ListDir on the volume root is legitimate: %v", err)
+ }
+ if err := restClient.DeleteBulk(ctx, "foo", "b.txt"); err != nil {
+ t.Fatalf("DeleteBulk on a legitimate path: %v", err)
+ }
+ if _, err := os.Stat(filepath.Join(drive, "foo", "b.txt")); err == nil {
+ t.Fatal("DeleteBulk on a legitimate path did not delete")
+ }
+
+ // Object keys that look like separators or path components but are not.
+ // A whitespace-only key is legal in S3 and is committed through the rename
+ // path on PutObject, so a guard that refuses it fails the write on every
+ // remote drive at once and breaks quorum. This is a real regression that
+ // shipped in an earlier draft of the guard.
+ oddKeys := []string{" ", " ", "\t", "a b", " lead", "trail ", "..foo", "foo..", "a/..b"}
+ if runtime.GOOS != globalWindowsOSName {
+ // Backslash is an ordinary filename character off Windows.
+ oddKeys = append(oddKeys, "\\", "\\\\", "a\\b", "/\\")
+ }
+ for _, key := range oddKeys {
+ if !IsValidObjectName(key) {
+ t.Fatalf("test bug: %q is not a legal object name", key)
+ }
+ if err := restClient.WriteAll(ctx, "foo", key, []byte("ODD")); err != nil {
+ t.Errorf("WriteAll(%q) on a legal object key: %v", key, err)
+ continue
+ }
+ if b, err := restClient.ReadAll(ctx, "foo", key); err != nil || string(b) != "ODD" {
+ t.Errorf("ReadAll(%q) on a legal object key: %q %v", key, b, err)
+ }
+ // The rename path is what PutObject uses to commit.
+ if err := restClient.RenameFile(ctx, "foo", key, "foo", key+"-renamed"); err != nil {
+ t.Errorf("RenameFile(%q) on a legal object key: %v", key, err)
+ continue
+ }
+ if err := restClient.DeleteBulk(ctx, "foo", key+"-renamed"); err != nil {
+ t.Errorf("DeleteBulk(%q) on a legal object key: %v", key+"-renamed", err)
+ }
+ }
+}
+
+// TestPeerS3VolumeTraversalRejected covers the peer-S3 bucket RPCs, which reach
+// local drives through globalLocalDrivesMap and never pass through
+// storageRESTServer.getStorage(). Only the getVolDir guard protects them.
+func TestPeerS3VolumeTraversalRejected(t *testing.T) {
+ newStorageRESTHTTPServerClient(t)
+ drive := globalLocalSetDrives[0][0][0].Endpoint().Path
+ ctx := t.Context()
+
+ victim := filepath.Join(filepath.Dir(drive), "peer-victim")
+ mustWrite(t, filepath.Join(victim, "nested", "important.txt"), "IMPORTANT")
+ t.Cleanup(func() { os.RemoveAll(victim) })
+
+ for _, p := range traversalPaths {
+ t.Run("DeleteBucket/"+p, func(t *testing.T) {
+ // Force:true reaches moveToTrash(volumeDir, recursive, immediate).
+ if err := deleteBucketLocal(ctx, p+"/peer-victim", DeleteBucketOptions{Force: true}); err == nil {
+ t.Errorf("deleteBucketLocal(%q) returned nil", p)
+ }
+ if _, err := os.Stat(filepath.Join(victim, "nested", "important.txt")); err != nil {
+ t.Errorf("deleteBucketLocal(%q) destroyed a tree outside the drive root: %v", p, err)
+ }
+ })
+ t.Run("MakeBucket/"+p, func(t *testing.T) {
+ created := filepath.Join(filepath.Dir(drive), "peer-created")
+ t.Cleanup(func() { os.RemoveAll(created) })
+ if err := makeBucketLocal(ctx, p+"/peer-created", MakeBucketOptions{}); err == nil {
+ t.Errorf("makeBucketLocal(%q) returned nil", p)
+ }
+ if _, err := os.Stat(created); err == nil {
+ t.Errorf("makeBucketLocal(%q) created a directory outside the drive root", p)
+ }
+ })
+ }
+}
+
+// TestCheckPartsMalformedErasure covers a remote node kill that is not a
+// traversal: CheckParts hands a wire-supplied FileInfo to ShardFileSize, which
+// divided by ErasureInfo.BlockSize with no validity check. The call runs inside
+// xioutil.WithDeadline, i.e. a bare goroutine, so the resulting integer
+// divide-by-zero panic cannot be recovered by the grid or net/http handlers --
+// it terminates the process. If this test regresses it does not fail, it
+// crashes the whole run.
+func TestCheckPartsMalformedErasure(t *testing.T) {
+ restClient := newStorageRESTHTTPServerClient(t)
+ drive := globalLocalSetDrives[0][0][0].Endpoint().Path
+ ctx := t.Context()
+
+ // Not panicking is the floor, not the bar. ShardFileSize returns 0 for these,
+ // and checkPart's "st.Size() < expectedSize" then succeeds for *any* file
+ // that exists - including a truncated shard - so a request that got this far
+ // would come back reporting every part intact. The boundary must refuse it
+ // outright, which is what errFileCorrupt asserts here.
+ for _, fi := range []FileInfo{
+ {Volume: "foo", Name: "obj", Parts: []ObjectPartInfo{{Number: 1, Size: 4}}},
+ // Deleted short-circuits FileInfo.IsValid() to true, so an IsValid()
+ // guard would not have caught this one.
+ {Volume: "foo", Name: "obj", Deleted: true, Parts: []ObjectPartInfo{{Number: 1, Size: 4}}},
+ {Volume: "foo", Name: "obj", Parts: []ObjectPartInfo{{Number: 1, Size: 4}},
+ Erasure: ErasureInfo{DataBlocks: 4}}, // BlockSize still zero
+ {Volume: "foo", Name: "obj", Parts: []ObjectPartInfo{{Number: 1, Size: 4}},
+ Erasure: ErasureInfo{BlockSize: blockSizeV2}}, // DataBlocks still zero
+ } {
+ if _, err := restClient.CheckParts(ctx, "foo", "obj", fi); !errors.Is(err, errFileCorrupt) {
+ t.Errorf("CheckParts(%+v): got %v, want %v", fi.Erasure, err, errFileCorrupt)
+ }
+ if _, err := restClient.VerifyFile(ctx, "foo", "obj", fi); !errors.Is(err, errFileCorrupt) {
+ t.Errorf("VerifyFile(%+v): got %v, want %v", fi.Erasure, err, errFileCorrupt)
+ }
+ }
+
+ // A part of zero length says nothing about the erasure parameters, so it
+ // must not be swept up by the rule above.
+ zeroPart := FileInfo{Volume: "foo", Name: "obj", Parts: []ObjectPartInfo{{Number: 1, Size: 0}}}
+ if _, err := restClient.CheckParts(ctx, "foo", "obj", zeroPart); errors.Is(err, errFileCorrupt) {
+ t.Errorf("CheckParts with a zero-length part was rejected as corrupt")
+ }
+
+ // A NEGATIVE part size is the sharper case, and the one an ">0" test misses.
+ // ShardFileSize returns 0 for it whether or not the erasure parameters are
+ // usable (numShards and lastShardSize both floor to zero), so checkPart's
+ // "st.Size() < expectedSize" is false for any file that exists and the part
+ // is reported healthy. With a real short part planted on disk, a guard that
+ // only rejects Size > 0 lets malformed metadata launder a truncated shard
+ // into a clean bill of health.
+ partDir := filepath.Join(drive, "foo", "negobj")
+ if err := os.MkdirAll(partDir, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ mustWrite(t, filepath.Join(partDir, "part.1"), "tiny")
+
+ for _, e := range []ErasureInfo{
+ {}, // unusable parameters
+ // Fully valid parameters. This is the sharper case: the FileInfo passes
+ // FileInfo.IsValid(), the very check healing uses to decide the metadata
+ // is trustworthy, and the negative size still lands on a zero expected
+ // shard size. Valid erasure parameters do not save you here.
+ {DataBlocks: 2, ParityBlocks: 2, BlockSize: blockSizeV2, Index: 1, Distribution: []int{1, 2, 3, 4}},
+ } {
+ fi := FileInfo{
+ Volume: "foo", Name: "negobj", Erasure: e,
+ Parts: []ObjectPartInfo{{Number: 1, Size: -2}},
+ }
+ if e.DataBlocks > 0 && !fi.IsValid() {
+ t.Fatal("test bug: the second case must satisfy FileInfo.IsValid()")
+ }
+ resp, err := restClient.CheckParts(ctx, "foo", "negobj", fi)
+ if !errors.Is(err, errFileCorrupt) {
+ t.Errorf("CheckParts with a negative part size (erasure %+v): got err=%v resp=%v, want %v",
+ e, err, resp, errFileCorrupt)
+ if resp != nil && len(resp.Results) > 0 && resp.Results[0] == checkPartSuccess {
+ t.Errorf(" -> and it reported the part HEALTHY, which is the actual damage")
+ }
+ }
+ if _, err := restClient.VerifyFile(ctx, "foo", "negobj", fi); !errors.Is(err, errFileCorrupt) {
+ t.Errorf("VerifyFile with a negative part size (erasure %+v): got %v, want %v", e, err, errFileCorrupt)
+ }
+ }
+}
+
diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go
index 0e7803bcf..2937b47b9 100644
--- a/cmd/xl-storage.go
+++ b/cmd/xl-storage.go
@@ -788,6 +788,22 @@ func (s *xlStorage) getVolDir(volume string) (string, error) {
if volume == "" || volume == "." || volume == ".." {
return "", errVolumeNotFound
}
+ // Reject traversal smuggled inside the volume name itself, e.g. "../" or
+ // "..\", which the equality checks above do not catch.
+ //
+ // This must be evaluated on the raw argument and never on the joined
+ // result: pathJoin() below runs path.Clean() against an absolute
+ // drivePath, and Clean() *erases* leading ".." on an absolute path
+ // ("/drive/../../etc" becomes "/etc"), so a check placed after the join
+ // would silently accept an escaped path.
+ //
+ // This is also the only containment control covering callers that never
+ // reach storageRESTServer.getStorage() - notably the peer-S3 bucket RPCs
+ // (MakeBucket/HeadBucket/DeleteBucket/HealBucket), which drive
+ // MakeVol/StatVol/DeleteVol straight off globalLocalDrivesMap.
+ if hasBadPathComponent(volume) {
+ return "", errVolumeNotFound
+ }
volumeDir := pathJoin(s.drivePath, volume)
return volumeDir, nil
}