diff --git a/test/sftp/sibling_prefix_test.go b/test/sftp/sibling_prefix_test.go new file mode 100644 index 000000000..39df297b8 --- /dev/null +++ b/test/sftp/sibling_prefix_test.go @@ -0,0 +1,72 @@ +package sftp + +import ( + "io" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestScopedUserCannotAccessSiblingPrefix verifies that a permission entry +// for /tenants/alice does not leak onto sibling paths such as +// /tenants/alice-archive that merely share the string prefix. +func TestScopedUserCannotAccessSiblingPrefix(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + + config := DefaultTestConfig() + config.EnableDebug = testing.Verbose() + + fw := NewSftpTestFramework(t, config) + require.NoError(t, fw.Setup(config)) + defer fw.Cleanup() + + // admin seeds both tenants + admin, adminConn, err := fw.ConnectSFTP("admin", "adminpassword") + require.NoError(t, err) + defer adminConn.Close() + defer admin.Close() + + require.NoError(t, admin.MkdirAll("/tenants/alice")) + require.NoError(t, admin.MkdirAll("/tenants/alice-archive")) + secret, err := admin.Create("/tenants/alice-archive/secret.txt") + require.NoError(t, err) + _, err = secret.Write([]byte("SIBLING-SECRET")) + require.NoError(t, err) + require.NoError(t, secret.Close()) + + scoped, scopedConn, err := fw.ConnectSFTP("scoped", "scopedpassword") + require.NoError(t, err) + defer scopedConn.Close() + defer scoped.Close() + + // positive control: the scoped user can write within its own prefix + own, err := scoped.Create("/tenants/alice/own.txt") + require.NoError(t, err) + _, err = own.Write([]byte("ok")) + require.NoError(t, err) + require.NoError(t, own.Close()) + + // reading the sibling tenant's file must fail + if f, err := scoped.Open("/tenants/alice-archive/secret.txt"); err == nil { + data, _ := io.ReadAll(f) + f.Close() + t.Fatalf("ACL for /tenants/alice read sibling /tenants/alice-archive: %q", data) + } + + // overwriting the sibling tenant's file must fail + if f, err := scoped.Create("/tenants/alice-archive/secret.txt"); err == nil { + f.Write([]byte("OVERWRITTEN-BY-SCOPED-USER")) + f.Close() + t.Fatal("ACL for /tenants/alice overwrote sibling /tenants/alice-archive") + } + + // the sibling file is intact + f, err := admin.Open("/tenants/alice-archive/secret.txt") + require.NoError(t, err) + data, err := io.ReadAll(f) + require.NoError(t, err) + require.NoError(t, f.Close()) + require.Equal(t, "SIBLING-SECRET", string(data)) +} diff --git a/test/sftp/testdata/userstore.json b/test/sftp/testdata/userstore.json index 2ffa84539..5435264c1 100644 --- a/test/sftp/testdata/userstore.json +++ b/test/sftp/testdata/userstore.json @@ -31,6 +31,17 @@ }, "Uid": 1002, "Gid": 1002 + }, + { + "Username": "scoped", + "Password": "scopedpassword", + "PublicKeys": [], + "HomeDir": "/", + "Permissions": { + "/tenants/alice": ["read", "write", "list"] + }, + "Uid": 1003, + "Gid": 1003 } ] diff --git a/weed/sftpd/sftp_permissions.go b/weed/sftpd/sftp_permissions.go index 818de5a99..035785d76 100644 --- a/weed/sftpd/sftp_permissions.go +++ b/weed/sftpd/sftp_permissions.go @@ -114,9 +114,23 @@ func (fs *SftpServer) CheckFilePermission(path string, perm string) error { return os.ErrPermission } +// pathWithin reports whether candidate is base itself or a descendant of base, +// on path-component boundaries so /a does not match /a-sibling. +func pathWithin(base, candidate string) bool { + base = stdpath.Clean(base) + candidate = stdpath.Clean(candidate) + if base == "/" { + return strings.HasPrefix(candidate, "/") + } + return candidate == base || strings.HasPrefix(candidate, base+"/") +} + // isPathInHomeDirectory checks if a path is in the user's home directory func isPathInHomeDirectory(user *user.User, path string) bool { - return strings.HasPrefix(path, user.HomeDir) + if user.HomeDir == "" { + return true + } + return pathWithin(user.HomeDir, path) } // HasUnixPermission checks if the user has the required Unix permission @@ -182,9 +196,10 @@ func HasExplicitPermission(user *user.User, filepath, requiredPerm string, isDir var perms []string for p, userPerms := range user.Permissions { - // Check if the path is either the permission path exactly or is under that path - if strings.HasPrefix(filepath, p) && len(p) > len(bestMatch) { - bestMatch = p + // The path must be the permission path exactly or under that path + cleaned := stdpath.Clean(p) + if pathWithin(cleaned, filepath) && len(cleaned) > len(bestMatch) { + bestMatch = cleaned perms = userPerms } } diff --git a/weed/sftpd/sftp_permissions_test.go b/weed/sftpd/sftp_permissions_test.go new file mode 100644 index 000000000..739072894 --- /dev/null +++ b/weed/sftpd/sftp_permissions_test.go @@ -0,0 +1,64 @@ +package sftpd + +import ( + "testing" + + "github.com/seaweedfs/seaweedfs/weed/sftpd/user" +) + +func TestPathWithin(t *testing.T) { + tests := []struct { + base string + candidate string + want bool + }{ + {"/tenants/alice", "/tenants/alice", true}, + {"/tenants/alice", "/tenants/alice/file.txt", true}, + {"/tenants/alice", "/tenants/alice/sub/dir", true}, + {"/tenants/alice", "/tenants/alice-archive", false}, + {"/tenants/alice", "/tenants/alice-archive/secret.txt", false}, + {"/tenants/alice", "/tenants/alice2", false}, + {"/tenants/alice", "/tenants", false}, + {"/tenants/alice/", "/tenants/alice/file.txt", true}, + {"/tenants/alice", "/tenants/alice/../alice-archive", false}, + {"/tenants/alice", "/tenants/alice/..", false}, + {"/", "/", true}, + {"/", "/anything", true}, + } + for _, tt := range tests { + if got := pathWithin(tt.base, tt.candidate); got != tt.want { + t.Errorf("pathWithin(%q, %q) = %v, want %v", tt.base, tt.candidate, got, tt.want) + } + } +} + +func TestHasExplicitPermissionSiblingPrefix(t *testing.T) { + u := &user.User{ + Username: "scoped", + HomeDir: "/", + Permissions: map[string][]string{ + "/tenants/alice": {"read", "write", "list"}, + }, + } + + if !HasExplicitPermission(u, "/tenants/alice/file.txt", PermRead, false) { + t.Error("expected read permission under /tenants/alice") + } + if HasExplicitPermission(u, "/tenants/alice-archive/secret.txt", PermRead, false) { + t.Error("ACL for /tenants/alice must not grant read on sibling /tenants/alice-archive") + } + if HasExplicitPermission(u, "/tenants/alice-archive/secret.txt", PermWrite, false) { + t.Error("ACL for /tenants/alice must not grant write on sibling /tenants/alice-archive") + } +} + +func TestIsPathInHomeDirectorySiblingPrefix(t *testing.T) { + u := &user.User{Username: "alice", HomeDir: "/tenants/alice"} + + if !isPathInHomeDirectory(u, "/tenants/alice/file.txt") { + t.Error("expected /tenants/alice/file.txt to be within home /tenants/alice") + } + if isPathInHomeDirectory(u, "/tenants/alice-archive/secret.txt") { + t.Error("home /tenants/alice must not match sibling /tenants/alice-archive") + } +}