sftp: match permission paths on path-component boundaries (#10311)

* sftp: match permission paths on path-component boundaries

Permission checks compared paths with raw string prefixes, so a
permission entry for /tenants/alice also matched sibling paths such as
/tenants/alice-archive, letting a scoped user read and overwrite
another tenant's files. The home directory containment check had the
same flaw. Route both through pathWithin, which cleans the paths and
requires exact equality or a separator-delimited descendant.

* sftp: clean permission path into a local when ranking matches

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))
+}

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
   }
 ]
 

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
 		}
 	}

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")
+	}
+}