Fix (containerized) - list_buckets allowing unauthorized bucket access

Signed-off-by: Aayush Chouhan <[email protected]>

Author: aayushchouhan09

src/server/common_services/auth_server.js

@@ -496,6 +496,10 @@ function _prepare_auth_request(req) {
     req.has_bucket_action_permission = async function(bucket, action, bucket_path, req_query) {
         return has_bucket_action_permission(bucket, req.account, action, req_query, bucket_path);
     };
+
+    req.has_list_bucket_permission = function(bucket) {
+        return has_list_bucket_permission(bucket, req.account, req.system);
+    };
 }
 
 function _get_auth_info(account, system, authorized_by, role, extra) {
@@ -520,6 +524,100 @@ function _get_auth_info(account, system, authorized_by, role, extra) {
     return response;
 }
 
+/**
+ * is_system_owner checks if the account is the system owner
+ * @param {Record<string, any>} system
+ * @param {Record<string, any>} account
+ * @returns {boolean}
+ */
+function is_system_owner(system, account) {
+    return system?.owner?.email?.unwrap() === account?.email?.unwrap();
+}
+
+/**
+ * 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}
+ */
+function is_bucket_owner(bucket, account) {
+    return bucket?.owner_account?.email?.unwrap() === account?.email?.unwrap();
+}
+
+/**
+ * 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 function has_iam_list_buckets_permission(bucket, account) {
+    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 s3_bucket_policy_utils.has_bucket_policy_permission(
+            iam_policy.policy_document,
+            undefined,
+            's3:ListAllMyBuckets',
+            'arn:aws:s3:::*',
+            undefined,
+            { should_pass_principal: false }
+        );
+
+        if (result === 'DENY') return false;
+
+        if (result === 'ALLOW') {
+            const bucket_owner_id = bucket.owner_account?._id;
+            const account_owner_id = account.owner?._id;
+            const ownership_match = account_owner_id !== undefined && bucket_owner_id !== undefined &&
+                                   String(bucket_owner_id) === String(account_owner_id);
+
+            // special case: check if root is OBC account and this is the claimed bucket
+            const root_claimed_bucket = account.owner?.bucket_claim_owner?.name?.unwrap();
+            const obc_claim_match = root_claimed_bucket && root_claimed_bucket === bucket.name?.unwrap();
+
+            if (ownership_match || obc_claim_match) return true;
+        }
+    }
+
+    return false;
+}
+
+/**
+ * has_list_bucket_permission returns true if the account can list the bucket in ListBuckets operation
+ *
+ * aws-compliant behavior:
+ * - System owner can list all the buckets
+ * - Root accounts can list buckets they own
+ * - IAM users CANNOT inherit ListBuckets visibility from root
+ * - IAM users need explicit IAM policy with s3:ListAllMyBuckets to list buckets
+ *
+ * @param {Record<string, any>} bucket
+ * @param {Record<string, any>} account
+ * @param {Record<string, any>} system
+ * @returns {Promise<boolean>}
+ */
+async function has_list_bucket_permission(bucket, account, system) {
+    // system owner can list all the buckets
+    if (is_system_owner(bucket.system, account)) return true;
+
+    // check direct ownership
+    if (is_bucket_owner(bucket, account)) return true;
+
+    // special case: check bucket claim ownership (OBC)
+    if (account.bucket_claim_owner?.name?.unwrap() === bucket.name.unwrap()) return true;
+
+    // check IAM policy for s3:ListAllMyBuckets
+    // Note: IAM users need this policy to list buckets owned by their root account
+    if (await has_iam_list_buckets_permission(bucket, account)) return true;
+
+    return false;
+}
+
 /**
  * has_bucket_action_permission returns true if the requesting account has permission to perform
  * the given action on the given bucket.
@@ -538,14 +636,22 @@ function _get_auth_info(account, system, authorized_by, role, extra) {
  */
 async function has_bucket_action_permission(bucket, account, action, req_query, bucket_path = "") {
     dbg.log1('has_bucket_action_permission:', bucket.name, account.email, bucket.owner_account.email);
-    // If the system owner account wants to access the bucket, allow it
-    if (bucket.system.owner.email.unwrap() === account.email.unwrap()) return true;
 
-    const is_owner = (bucket.owner_account.email.unwrap() === account.email.unwrap()) ||
-        (account.bucket_claim_owner && account.bucket_claim_owner.name.unwrap() === bucket.name.unwrap());
+    // system owner can access all buckets
+    if (is_system_owner(bucket.system, account)) return true;
+
+    // check ownership: direct owner, IAM user inheritance, or OBC
+    const is_owner = is_bucket_owner(bucket, account);
+    const bucket_owner_id = bucket.owner_account?._id;
+    const account_owner_id = account.owner?._id;
+    const has_iam_access = account_owner_id !== undefined && bucket_owner_id !== undefined &&
+                           String(bucket_owner_id) === String(account_owner_id);
+    const has_obc_access = account.bucket_claim_owner?.name?.unwrap() === bucket.name.unwrap();
+    const owner = is_owner || has_iam_access || has_obc_access;
+
     const bucket_policy = bucket.s3_policy;
 
-    if (!bucket_policy) return is_owner;
+    if (!bucket_policy) return owner;
     if (!action) {
         throw new Error('has_bucket_action_permission: action is required');
     }
@@ -560,7 +666,8 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
     );
 
     if (result === 'DENY') return false;
-    return is_owner || result === 'ALLOW';
+
+    return owner || result === 'ALLOW';
 }
 
 /**

src/server/system_services/bucket_server.js

@@ -1095,9 +1095,13 @@ async function list_buckets(req) {
     let continuation_token = req.rpc_params?.continuation_token;
     const max_buckets = req.rpc_params?.max_buckets;
 
-    const accessible_bucket_list = system_store.data.buckets.filter(
-        async bucket => await req.has_s3_bucket_permission(bucket, "s3:ListBucket", req) && !bucket.deleting
-    );
+    // filter buckets based on ownership (async to support IAM policy checks)
+    const bucket_permissions = await P.map(system_store.data.buckets, async bucket => {
+        if (bucket.deleting) return null;
+        const has_permission = await req.has_list_bucket_permission(bucket);
+        return has_permission ? bucket : null;
+    });
+    const accessible_bucket_list = bucket_permissions.filter(bucket => bucket !== null);
 
     accessible_bucket_list.sort((a, b) => a.name.unwrap().localeCompare(b.name.unwrap()));
 

src/test/integration_tests/api/s3/test_s3_list_buckets.js

@@ -127,5 +127,53 @@ mocha.describe('s3_ops', function() {
         });
     });
 
+    mocha.describe('list_buckets permissions', function() {
+        this.timeout(60000);
+        let s3_account_a;
+        let s3_account_b;
+
+        async function create_account_and_client(name) {
+            const account = await rpc_client.account.create_account({
+                name, email: name, has_login: false, s3_access: true,
+                default_resource: coretest.POOL_LIST[0].name
+            });
+            return new S3Client({
+                ...s3_client_params,
+                credentials: {
+                    accessKeyId: account.access_keys[0].access_key.unwrap(),
+                    secretAccessKey: account.access_keys[0].secret_key.unwrap(),
+                }
+            });
+        }
+
+        mocha.before(async function() {
+            s3_account_a = await create_account_and_client('account-a');
+            s3_account_b = await create_account_and_client('account-b');
+            await s3_account_a.send(new CreateBucketCommand({ Bucket: 'bucket-a' }));
+            await s3_account_b.send(new CreateBucketCommand({ Bucket: 'bucket-b' }));
+            await s3.send(new CreateBucketCommand({ Bucket: 'admin-buck' }));
+        });
+
+        mocha.after(async function() {
+            await s3_account_a.send(new DeleteBucketCommand({ Bucket: 'bucket-a' }));
+            await s3_account_b.send(new DeleteBucketCommand({ Bucket: 'bucket-b' }));
+            await s3.send(new DeleteBucketCommand({ Bucket: 'admin-buck' }));
+            await rpc_client.account.delete_account({ email: 'account-a' });
+            await rpc_client.account.delete_account({ email: 'account-b' });
+        });
+
+        mocha.it('accounts should list only owned buckets', async function() {
+            const buckets_a = (await s3_account_a.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
+            const buckets_b = (await s3_account_b.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
+            assert.deepStrictEqual(buckets_a, ['bucket-a']);
+            assert.deepStrictEqual(buckets_b, ['bucket-b']);
+        });
+
+        mocha.it('admin should lists all the buckets', async function() {
+            const buckets = (await s3.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
+            assert(buckets.includes('bucket-a') && buckets.includes('bucket-b') && buckets.includes('admin-buck'));
+        });
+    });
+
 });
 

src/test/integration_tests/api/sts/test_sts.js

@@ -457,6 +457,7 @@ function verify_session_token(session_token, access_key, secret_key, assumed_rol
 mocha.describe('Session token tests', function() {
     const { rpc_client } = coretest;
     const alice2 = 'alice2';
+    const alice2_buck = 'alice2-test-bucket';
     const bob2 = 'bob2';
     const charlie2 = 'charlie2';
     const accounts = [{ email: alice2 }, { email: bob2 }, { email: charlie2 }];
@@ -466,6 +467,8 @@ mocha.describe('Session token tests', function() {
     mocha.after(async function() {
         const self = this; // eslint-disable-line no-invalid-this
         self.timeout(60000);
+
+        await accounts[0].s3.deleteBucket({ Bucket: alice2_buck }).promise();
         for (const account of accounts) {
             await rpc_client.account.delete_account({ email: account.email });
         }
@@ -542,6 +545,10 @@ mocha.describe('Session token tests', function() {
             name: 'first.bucket',
             policy: s3accesspolicy,
         });
+
+        // create a bucket owned by alice2 for ListBuckets to work
+        // Note: bucket policy is not related to ListBuckets operation
+        await accounts[0].s3.createBucket({ Bucket: alice2_buck }).promise();
     });
 
     mocha.it('user b assume role of user a - default expiry - list s3 - should be allowed', async function() {
@@ -564,7 +571,7 @@ mocha.describe('Session token tests', function() {
         });
 
         const buckets1 = await temp_s3_with_session_token.listBuckets().promise();
-        assert.ok(buckets1.Buckets.length > 0);
+        assert.ok(buckets1.Buckets[0].Name === alice2_buck);
     });
 
     mocha.it('user b assume role of user a - valid expiry via durationSeconds - list s3 - should be allowed', async function() {
@@ -589,7 +596,7 @@ mocha.describe('Session token tests', function() {
         });
 
         const buckets1 = await temp_s3_with_session_token.listBuckets().promise();
-        assert.ok(buckets1.Buckets.length > 0);
+        assert.ok(buckets1.Buckets[0].Name === alice2_buck);
     });
 
     mocha.it('user b assume role of user a - invalid expiry via durationSeconds - should be rejected', async function() {