Skip to content

Commit d895b76

Browse files
Fix (NC | NSFS) - list_buckets allowing unauthorized bucket access
Signed-off-by: Aayush Chouhan <[email protected]>
1 parent 0d80b71 commit d895b76

File tree

1 file changed

+42
-8
lines changed

1 file changed

+42
-8
lines changed

src/sdk/bucketspace_fs.js

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -271,9 +271,9 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
271271
if (err.rpc_code !== 'NO_SUCH_BUCKET') throw err;
272272
}
273273
if (!bucket) return;
274-
const bucket_policy_accessible = await this.has_bucket_action_permission(bucket, account, 's3:ListBucket');
275-
dbg.log2(`list_buckets: bucket_name ${bucket_name} bucket_policy_accessible`, bucket_policy_accessible);
276-
if (!bucket_policy_accessible) return;
274+
275+
if (!await this.has_bucket_ownership_permission(bucket, account)) return;
276+
277277
const fs_accessible = await this.validate_fs_bucket_access(bucket, object_sdk);
278278
if (!fs_accessible) return;
279279
return bucket;
@@ -921,16 +921,50 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
921921
///// UTILS /////
922922
/////////////////
923923

924-
// TODO: move the following 3 functions - has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir()
924+
// TODO: move the following 4 functions - is_bucket_owner(), has_bucket_ownership_permission(), has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir()
925925
// so they can be re-used
926+
927+
/**
928+
* is_bucket_owner checks if the account is the direct owner of the bucket
929+
* @param {Record<string, any>} bucket
930+
* @param {Record<string, any>} account
931+
* @returns {boolean}
932+
*/
933+
is_bucket_owner(bucket, account) {
934+
if (!bucket?.owner_account?.id || !account?._id) return false;
935+
return bucket.owner_account.id === account._id;
936+
}
937+
938+
/**
939+
* has_bucket_ownership_permission returns true if the account can list the bucket in ListBuckets operation
940+
* only checks bucket ownership
941+
*
942+
* aws-compliant behavior:
943+
* - Root accounts can list buckets they own
944+
* - IAM users can list their owner buckets
945+
*
946+
* @param {Record<string, any>} bucket
947+
* @param {Record<string, any>} account
948+
* @returns {Promise<boolean>}
949+
*/
950+
async has_bucket_ownership_permission(bucket, account) {
951+
// check direct ownership (root accounts only)
952+
if (this.is_bucket_owner(bucket, account)) return true;
953+
954+
// special case: iam user can list the buckets of their owner
955+
// TODO: handle iam user
956+
957+
return false;
958+
}
959+
926960
async has_bucket_action_permission(bucket, account, action, bucket_path = "") {
927961
dbg.log1('has_bucket_action_permission:', bucket.name.unwrap(), account.name.unwrap(), account._id, bucket.bucket_owner.unwrap());
928962

929-
const is_system_owner = Boolean(bucket.system_owner) && bucket.system_owner.unwrap() === account.name.unwrap();
963+
// system_owner is deprecated since version 5.18, so we don't need to check it
964+
965+
// check ownership: direct owner or IAM user inheritance
966+
const is_owner = this.is_bucket_owner(bucket, account);
930967

931-
// If the system owner account wants to access the bucket, allow it
932-
if (is_system_owner) return true;
933-
const is_owner = (bucket.owner_account && bucket.owner_account.id === account._id);
934968
const bucket_policy = bucket.s3_policy;
935969

936970
if (!bucket_policy) {

0 commit comments

Comments
 (0)