Fix (NC | NSFS) - list_buckets allowing unauthorized bucket access

Signed-off-by: Aayush Chouhan <achouhan@redhat.com>

Author: aayushchouhan09

src/sdk/bucketspace_fs.js

@@ -271,9 +271,9 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
                 if (err.rpc_code !== 'NO_SUCH_BUCKET') throw err;
             }
             if (!bucket) return;
-            const bucket_policy_accessible = await this.has_bucket_action_permission(bucket, account, 's3:ListBucket');
-            dbg.log2(`list_buckets: bucket_name ${bucket_name} bucket_policy_accessible`, bucket_policy_accessible);
-            if (!bucket_policy_accessible) return;
+
+            if (!await this.has_list_buckets_permission(bucket, account)) return;
+
             const fs_accessible = await this.validate_fs_bucket_access(bucket, object_sdk);
             if (!fs_accessible) return;
             return bucket;
@@ -921,23 +921,101 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
     ///// UTILS /////
     /////////////////
 
-    // TODO: move the following 3 functions - has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir()
+    // TODO: move the following 4 functions - is_bucket_owner(), has_list_buckets_permission(), has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir()
     // so they can be re-used
+
+    /**
+     * is_bucket_owner checks if the account is the direct owner of the bucket
+     * @param {Record<string, any>} bucket
+     * @param {Record<string, any>} account
+     * @returns {boolean}
+     */
+    is_bucket_owner(bucket, account) {
+        // Ensure both IDs exist to avoid undefined === undefined bug
+        const bucket_owner_id = bucket?.owner_account?.id;
+        const account_id = account?._id;
+        return bucket_owner_id !== undefined && bucket_owner_id === account_id;
+    }
+
+    /**
+     * has_iam_list_buckets_permission checks if the account has IAM policy granting s3:ListAllMyBuckets
+     * for buckets owned by their root account
+     * @param {Record<string, any>} bucket
+     * @param {Record<string, any>} account
+     * @returns {Promise<boolean>}
+     */
+    async has_iam_list_buckets_permission(bucket, account) {
+        // only IAM users can have IAM policies
+        if (!account?.owner) return false;
+
+        const iam_policies = account.iam_user_policies || [];
+        if (iam_policies.length === 0) return false;
+
+        // check each IAM policy for s3:ListAllMyBuckets permission
+        for (const iam_policy of iam_policies) {
+            const result = await bucket_policy_utils.has_bucket_policy_permission(
+                iam_policy.policy_document,
+                undefined, // no principal check for IAM policies
+                's3:listallmybuckets',
+                'arn:aws:s3:::*',
+                undefined,
+                { should_pass_principal: false } // skip principal check
+            );
+
+            if (result === 'DENY') return false;
+
+            // if policy allows, check if bucket is owned by the IAM user's root account
+            // IAM policy only grants access to buckets owned by the IAM user's root account
+            if (result === 'ALLOW') {
+                const bucket_owner_id = bucket?.owner_account?.id;
+                return bucket_owner_id !== undefined && bucket_owner_id === account.owner;
+            }
+        }
+
+        return false;
+    }
+
+    /**
+     * has_list_buckets_permission returns true if the account can list the bucket in ListBuckets operation
+     * only checks bucket ownership
+     *
+     * aws-compliant behavior:
+     * - Root accounts can list buckets they own
+     * - IAM users cannot inherit ListBuckets visibility from root (unlike bucket operations)
+     * - IAM users need explicit IAM policy with s3:ListAllMyBuckets to list buckets
+     *
+     * @param {Record<string, any>} bucket
+     * @param {Record<string, any>} account
+     * @returns {Promise<boolean>}
+     */
+    async has_list_buckets_permission(bucket, account) {
+        // check direct ownership (root accounts only)
+        if (this.is_bucket_owner(bucket, account)) return true;
+
+        // check IAM policy for s3:ListAllMyBuckets
+        // Note: IAM users need this policy to list buckets owned by their root account
+        if (await this.has_iam_list_buckets_permission(bucket, account)) return true;
+
+        return false;
+    }
+
     async has_bucket_action_permission(bucket, account, action, bucket_path = "") {
         dbg.log1('has_bucket_action_permission:', bucket.name.unwrap(), account.name.unwrap(), account._id, bucket.bucket_owner.unwrap());
 
-        const is_system_owner = Boolean(bucket.system_owner) && bucket.system_owner.unwrap() === account.name.unwrap();
+        // system_owner is deprecated since version 5.18, so we don't need to check it
+
+        // check ownership: direct owner or IAM user inheritance
+        const is_owner = this.is_bucket_owner(bucket, account);
 
-        // If the system owner account wants to access the bucket, allow it
-        if (is_system_owner) return true;
-        const is_owner = (bucket.owner_account && bucket.owner_account.id === account._id);
         const bucket_policy = bucket.s3_policy;
 
         if (!bucket_policy) {
             // in case we do not have bucket policy
             // we allow IAM account to access a bucket that that is owned by their root account
+            const bucket_owner_id = bucket.owner_account?.id;
             const is_iam_and_same_root_account_owner = account.owner !== undefined &&
-                account.owner === bucket.owner_account.id;
+                bucket_owner_id !== undefined &&
+                account.owner === bucket_owner_id;
             return is_owner || is_iam_and_same_root_account_owner;
         }
         if (!action) {

src/test/unit_tests/nsfs/test_bucketspace_fs.js

@@ -22,7 +22,7 @@ const nb_native = require('../../../util/nb_native');
 const SensitiveString = require('../../../util/sensitive_string');
 const NamespaceFS = require('../../../sdk/namespace_fs');
 const BucketSpaceFS = require('../../../sdk/bucketspace_fs');
-const { TMP_PATH, generate_s3_policy } = require('../../system_tests/test_utils');
+const { TMP_PATH } = require('../../system_tests/test_utils');
 const { CONFIG_SUBDIRS, JSON_SUFFIX } = require('../../../sdk/config_fs');
 const nc_mkm = require('../../../manage_nsfs/nc_master_key_manager').get_instance();
 
@@ -271,6 +271,17 @@ const account_iam_user1 = {
         gid: dummy_object_sdk.requesting_account.nsfs_account_config.gid,
         new_buckets_path: dummy_object_sdk.requesting_account.nsfs_account_config.new_buckets_path
     },
+    iam_user_policies: [{
+        policy_name: 'ListBucketsPolicy',
+        policy_document: {
+            Version: '2012-10-17',
+            Statement: [{
+                Effect: 'Allow',
+                Action: 's3:ListAllMyBuckets',
+                Resource: '*'
+            }]
+        }
+    }],
     creation_date: '2023-11-30T04:46:33.815Z',
 };
 
@@ -291,6 +302,17 @@ const account_iam_user2 = {
         gid: dummy_object_sdk.requesting_account.nsfs_account_config.gid,
         new_buckets_path: dummy_object_sdk.requesting_account.nsfs_account_config.new_buckets_path
     },
+    iam_user_policies: [{
+        policy_name: 'ListBucketsPolicy',
+        policy_document: {
+            Version: '2012-10-17',
+            Statement: [{
+                Effect: 'Allow',
+                Action: 's3:ListAllMyBuckets',
+                Resource: '*'
+            }]
+        }
+    }],
     creation_date: '2023-12-30T04:46:33.815Z',
 };
 
@@ -501,13 +523,13 @@ mocha.describe('bucketspace_fs', function() {
         });
         mocha.it('list buckets - iam accounts', async function() {
             // root account created a bucket
-            // account_iam_user2 can list the created bucket (the implicit policy - same root account)
+            // account_iam_user1 can list the created bucket (has s3:ListAllMyBuckets policy)
             const dummy_object_sdk_for_iam_account = make_dummy_object_sdk_for_account(dummy_object_sdk, account_iam_user1);
             const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account);
             assert.equal(res.buckets.length, 1);
             assert.equal(res.buckets[0].name.unwrap(), test_bucket);
 
-            // account_iam_user2 can list the created bucket (the implicit policy - same root account)
+            // account_iam_user2 can list the created bucket (has s3:ListAllMyBuckets policy)
             const dummy_object_sdk_for_iam_account2 = make_dummy_object_sdk_for_account(dummy_object_sdk, account_iam_user2);
             const res2 = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account2);
             assert.equal(res2.buckets.length, 1);
@@ -519,27 +541,10 @@ mocha.describe('bucketspace_fs', function() {
             const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account);
             assert.equal(res.buckets.length, 0);
         });
-        mocha.it('list buckets - different account with bucket policy (principal by name)', async function() {
-            // another user created a bucket
-            // with bucket policy account_user3 can list it
-            const policy = generate_s3_policy(account_user4.name, test_bucket, ['s3:*']).policy;
-            const param = { name: test_bucket, policy: policy };
-            await bucketspace_fs.put_bucket_policy(param);
-            const dummy_object_sdk_for_iam_account = make_dummy_object_sdk_for_account(dummy_object_sdk, account_user4);
-            const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account);
-            assert.equal(res.buckets.length, 1);
-            assert.equal(res.buckets[0].name.unwrap(), test_bucket);
-        });
-        mocha.it('list buckets - different account with bucket policy (principal by id)', async function() {
-            // another user created a bucket
-            // with bucket policy account_user3 can list it
-            const policy = generate_s3_policy(account_user4._id, test_bucket, ['s3:*']).policy;
-            const param = { name: test_bucket, policy: policy };
-            await bucketspace_fs.put_bucket_policy(param);
+        mocha.it('list buckets - different account cannot list buckets they do not own', async function() {
             const dummy_object_sdk_for_iam_account = make_dummy_object_sdk_for_account(dummy_object_sdk, account_user4);
             const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account);
-            assert.equal(res.buckets.length, 1);
-            assert.equal(res.buckets[0].name.unwrap(), test_bucket);
+            assert.equal(res.buckets.length, 0);
         });
         mocha.afterEach(async function() {
             await fs_utils.folder_delete(`${new_buckets_path}/${test_bucket}`);
@@ -626,7 +631,7 @@ mocha.describe('bucketspace_fs', function() {
             // root account created the bucket
             await create_bucket(test_bucket_iam_account);
 
-            // account_iam_user1 can see the bucket in the list
+            // account_iam_user1 can see the bucket in the list (has s3:ListAllMyBuckets policy)
             const dummy_object_sdk_for_account_iam_user1 = make_dummy_object_sdk_for_account(dummy_object_sdk, account_iam_user1);
             const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_account_iam_user1);
             assert.ok(res.buckets.length > 0);