From 2f24fdf77f25766c9f3736e2732c519a37e9b05d Mon Sep 17 00:00:00 2001 From: Aayush Chouhan Date: Wed, 26 Nov 2025 12:47:46 +0530 Subject: [PATCH] Fix (NC | NSFS) - list_buckets allowing unauthorized bucket access Signed-off-by: Aayush Chouhan --- src/sdk/bucketspace_fs.js | 223 +++++++++++------- src/sdk/nb.d.ts | 2 + .../unit_tests/nsfs/test_bucketspace_fs.js | 61 +++-- 3 files changed, 173 insertions(+), 113 deletions(-) diff --git a/src/sdk/bucketspace_fs.js b/src/sdk/bucketspace_fs.js index 252242d918..5f0dafc926 100644 --- a/src/sdk/bucketspace_fs.js +++ b/src/sdk/bucketspace_fs.js @@ -241,7 +241,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS { * permissions * a. First iteration - map_with_concurrency with concurrency rate of 10 entries at the same time. * a.1. if entry is dir file/entry is non json - we will return undefined - * a.2. if bucket is unaccessible by bucket policy - we will return undefined + * a.2. if bucket is not owned by the account (or their root account for IAM users) - we will return undefined * a.3. if underlying storage of the bucket is unaccessible by the account's uid/gid - we will return undefined * a.4. else - return the bucket config info. * b. Second iteration - filter empty entries - filter will remove undefined values produced by the map_with_concurrency(). @@ -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 (!this.has_bucket_ownership_permission(bucket, account)) return; + const fs_accessible = await this.validate_fs_bucket_access(bucket, object_sdk); if (!fs_accessible) return; return bucket; @@ -921,83 +921,6 @@ class BucketSpaceFS extends BucketSpaceSimpleFS { ///// UTILS ///// ///////////////// - // TODO: move the following 3 functions - has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir() - // so they can be re-used - 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(); - - // 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 is_iam_and_same_root_account_owner = account.owner !== undefined && - account.owner === bucket.owner_account.id; - return is_owner || is_iam_and_same_root_account_owner; - } - if (!action) { - throw new Error('has_bucket_action_permission: action is required'); - } - - let permission_by_name; - const permission_by_id = await bucket_policy_utils.has_bucket_policy_permission( - bucket_policy, - account._id, - action, - `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`, - undefined, - { disallow_public_access: bucket.public_access_block?.restrict_public_buckets } - ); - if (permission_by_id === "DENY") return false; - // we (currently) allow account identified to be both id and name, - // so if by-id failed, try also name - if (account.owner === undefined) { - permission_by_name = await bucket_policy_utils.has_bucket_policy_permission( - bucket_policy, - account.name.unwrap(), - action, - `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`, - undefined, - { disallow_public_access: bucket.public_access_block?.restrict_public_buckets } - ); - } - if (permission_by_name === 'DENY') return false; - return is_owner || (permission_by_id === 'ALLOW' || permission_by_name === 'ALLOW'); - } - - async validate_fs_bucket_access(bucket, object_sdk) { - dbg.log1('bucketspace_fs.validate_fs_bucket_access:', bucket.name.unwrap()); - const ns = bucket.namespace; - const is_nsfs_bucket = object_sdk.is_nsfs_bucket(ns); - const accessible = is_nsfs_bucket ? await this._has_access_to_nsfs_dir(ns, object_sdk) : false; - dbg.log1('bucketspace_fs.validate_fs_bucket_access:', bucket.name.unwrap(), is_nsfs_bucket, accessible); - return accessible; - } - - async _has_access_to_nsfs_dir(ns, object_sdk) { - const account = object_sdk.requesting_account; - dbg.log1('_has_access_to_nsfs_dir: nsr: ', ns, 'account.nsfs_account_config: ', account && account.nsfs_account_config); - // nsfs bucket - if (!account || !account.nsfs_account_config || (account.nsfs_account_config.uid === undefined) || - (account.nsfs_account_config.gid === undefined)) return false; - try { - dbg.log1('_has_access_to_nsfs_dir: checking access:', ns.write_resource, account.nsfs_account_config.uid, account.nsfs_account_config.gid); - const path_to_check = path.join(ns.write_resource.resource.fs_root_path, ns.write_resource.path || ''); - const fs_context = this.prepare_fs_context(object_sdk, ns.write_resource.resource.fs_backend); - await nb_native().fs.checkAccess(fs_context, path_to_check); - return true; - } catch (err) { - dbg.log0('_has_access_to_nsfs_dir: failed', err); - if (err.code === 'ENOENT' || err.code === 'EACCES' || (err.code === 'EPERM' && err.message === 'Operation not permitted')) return false; - throw err; - } - } - is_nsfs_containerized_user_anonymous(token) { return !token && !process.env.NC_NSFS_NO_DB_ENV; } @@ -1025,10 +948,10 @@ class BucketSpaceFS extends BucketSpaceSimpleFS { /** * _is_bucket_empty returns true if the given bucket is empty * - * @param {*} ns - * @param {*} params - * @param {string} name - * @param {nb.ObjectSDK} object_sdk + * @param {*} ns + * @param {*} params + * @param {string} name + * @param {nb.ObjectSDK} object_sdk * * @returns {Promise} */ @@ -1055,8 +978,8 @@ class BucketSpaceFS extends BucketSpaceSimpleFS { * _generate_reserved_tag_event_args returns the list of reserved * bucket tags which have been modified and also returns a variable * indicating if there has been any modifications at all. - * @param {Record} prev_tags_objectified - * @param {Array<{key: string, value: string}>} new_tags + * @param {Record} prev_tags_objectified + * @param {Array<{key: string, value: string}>} new_tags */ static _generate_reserved_tag_event_args(prev_tags_objectified, new_tags) { let reserved_tag_modified = false; @@ -1080,7 +1003,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS { } /** - * @param {Array<{key: string, value: string}>} tagging + * @param {Array<{key: string, value: string}>} tagging * @returns {Record} */ static _objectify_tagging_arr(tagging) { @@ -1089,7 +1012,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS { /** * _create_uls creates underylying storage at the given storage_path - * @param {*} fs_context - root fs_context + * @param {*} fs_context - root fs_context * @param {*} acc_fs_context - fs_context associated with the performing account * @param {*} name - bucket name * @param {*} storage_path - bucket's storage path @@ -1106,6 +1029,128 @@ class BucketSpaceFS extends BucketSpaceSimpleFS { throw translate_error_codes(error, entity_enum.BUCKET); } } + + ////////////////// + // SHARED UTILS // + ////////////////// + + // TODO: move the below functions to a shared utils file so they can be re-used + + /** + * is_bucket_owner checks if the account is the direct owner of the bucket + * @param {nb.Bucket} bucket + * @param {nb.Account} account + * @returns {boolean} + */ + is_bucket_owner(bucket, account) { + if (!bucket?.owner_account?.id || !account?._id) return false; + return bucket.owner_account.id === account._id; + } + + /** + * is_iam_and_same_root_account_owner checks if the account is an IAM user and the bucket is owned by their root account + * @param {nb.Account} account + * @param {nb.Bucket} bucket + * @returns {boolean} + */ + is_iam_and_same_root_account_owner(account, bucket) { + if (!account?.owner || !bucket?.owner_account?.id) return false; + return account.owner === bucket.owner_account.id; + } + + /** + * has_bucket_ownership_permission returns true if the account can list the bucket in ListBuckets operation + * + * aws-compliant behavior: + * - Root accounts can list buckets they own + * - IAM users can list their owner buckets + * + * @param {nb.Bucket} bucket + * @param {nb.Account} account + * @returns {boolean} + */ + has_bucket_ownership_permission(bucket, account) { + // check direct ownership + if (this.is_bucket_owner(bucket, account)) return true; + + // special case: iam user can list the buckets of their owner + if (this.is_iam_and_same_root_account_owner(account, bucket)) 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()); + + // 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); + + 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 + return is_owner || this.is_iam_and_same_root_account_owner(account, bucket); + } + if (!action) { + throw new Error('has_bucket_action_permission: action is required'); + } + + let permission_by_name; + const permission_by_id = await bucket_policy_utils.has_bucket_policy_permission( + bucket_policy, + account._id, + action, + `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`, + undefined, + { disallow_public_access: bucket.public_access_block?.restrict_public_buckets } + ); + if (permission_by_id === "DENY") return false; + // we (currently) allow account identified to be both id and name, + // so if by-id failed, try also name + if (account.owner === undefined) { + permission_by_name = await bucket_policy_utils.has_bucket_policy_permission( + bucket_policy, + account.name.unwrap(), + action, + `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`, + undefined, + { disallow_public_access: bucket.public_access_block?.restrict_public_buckets } + ); + } + if (permission_by_name === 'DENY') return false; + return is_owner || (permission_by_id === 'ALLOW' || permission_by_name === 'ALLOW'); + } + + async validate_fs_bucket_access(bucket, object_sdk) { + dbg.log1('bucketspace_fs.validate_fs_bucket_access:', bucket.name.unwrap()); + const ns = bucket.namespace; + const is_nsfs_bucket = object_sdk.is_nsfs_bucket(ns); + const accessible = is_nsfs_bucket ? await this._has_access_to_nsfs_dir(ns, object_sdk) : false; + dbg.log1('bucketspace_fs.validate_fs_bucket_access:', bucket.name.unwrap(), is_nsfs_bucket, accessible); + return accessible; + } + + async _has_access_to_nsfs_dir(ns, object_sdk) { + const account = object_sdk.requesting_account; + dbg.log1('_has_access_to_nsfs_dir: nsr: ', ns, 'account.nsfs_account_config: ', account && account.nsfs_account_config); + // nsfs bucket + if (!account || !account.nsfs_account_config || (account.nsfs_account_config.uid === undefined) || + (account.nsfs_account_config.gid === undefined)) return false; + try { + dbg.log1('_has_access_to_nsfs_dir: checking access:', ns.write_resource, account.nsfs_account_config.uid, account.nsfs_account_config.gid); + const path_to_check = path.join(ns.write_resource.resource.fs_root_path, ns.write_resource.path || ''); + const fs_context = this.prepare_fs_context(object_sdk, ns.write_resource.resource.fs_backend); + await nb_native().fs.checkAccess(fs_context, path_to_check); + return true; + } catch (err) { + dbg.log0('_has_access_to_nsfs_dir: failed', err); + if (err.code === 'ENOENT' || err.code === 'EACCES' || (err.code === 'EPERM' && err.message === 'Operation not permitted')) return false; + throw err; + } + } } module.exports = BucketSpaceFS; diff --git a/src/sdk/nb.d.ts b/src/sdk/nb.d.ts index 662ad17afc..b35f132547 100644 --- a/src/sdk/nb.d.ts +++ b/src/sdk/nb.d.ts @@ -63,6 +63,7 @@ interface System extends Base { interface Account extends Base { _id: ID; + owner?: ID; name: string; system: System; email: SensitiveString; @@ -179,6 +180,7 @@ interface MirrorStatus { interface Bucket extends Base { _id: ID; + owner_account?: ID; deleted?: Date; name: SensitiveString; system: System; diff --git a/src/test/unit_tests/nsfs/test_bucketspace_fs.js b/src/test/unit_tests/nsfs/test_bucketspace_fs.js index d785a65f6a..cc67ab89a1 100644 --- a/src/test/unit_tests/nsfs/test_bucketspace_fs.js +++ b/src/test/unit_tests/nsfs/test_bucketspace_fs.js @@ -519,28 +519,6 @@ 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); - 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.afterEach(async function() { await fs_utils.folder_delete(`${new_buckets_path}/${test_bucket}`); const file_path = get_config_file_path(CONFIG_SUBDIRS.BUCKETS, test_bucket); @@ -928,19 +906,54 @@ mocha.describe('bucketspace_fs', function() { }); mocha.describe('bucket tagging operations', function() { + const test_bucket_tagging = 'test-bucket-tagging'; + mocha.before(async function() { + await create_bucket(test_bucket_tagging); + }); + mocha.it('put_bucket_tagging', async function() { - const param = { name: test_bucket, tagging: [{ key: 'k1', value: 'v1' }] }; + const param = { name: test_bucket_tagging, tagging: [{ key: 'k1', value: 'v1' }] }; await bucketspace_fs.put_bucket_tagging(param, dummy_object_sdk); const tag = await bucketspace_fs.get_bucket_tagging(param); assert.deepEqual(tag, { tagging: param.tagging }); }); mocha.it('delete_bucket_tagging', async function() { - const param = { name: test_bucket }; + const param = { name: test_bucket_tagging }; await bucketspace_fs.delete_bucket_tagging(param); const tag = await bucketspace_fs.get_bucket_tagging(param); assert.deepEqual(tag, { tagging: [] }); }); + + mocha.it('put_bucket_tagging - different account with bucket policy (principal by name)', async function() { + const policy = generate_s3_policy(account_user4.name, test_bucket_tagging, ['s3:PutBucketTagging']).policy; + const param = { name: test_bucket_tagging, policy: policy }; + await bucketspace_fs.put_bucket_policy(param); + const dummy_object_sdk_for_account = make_dummy_object_sdk_for_account(dummy_object_sdk, account_user4); + const tagging_param = { name: test_bucket_tagging, tagging: [{ key: 'test-key', value: 'test-value' }] }; + await bucketspace_fs.put_bucket_tagging(tagging_param, dummy_object_sdk_for_account); + const tagging = await bucketspace_fs.get_bucket_tagging({ name: test_bucket_tagging }); + assert.equal(tagging.tagging.length, 1); + assert.equal(tagging.tagging[0].key, 'test-key'); + }); + + mocha.it('put_bucket_tagging - different account with bucket policy (principal by id)', async function() { + const policy = generate_s3_policy(account_user4._id, test_bucket_tagging, ['s3:PutBucketTagging']).policy; + const param = { name: test_bucket_tagging, policy: policy }; + await bucketspace_fs.put_bucket_policy(param); + const dummy_object_sdk_for_account = make_dummy_object_sdk_for_account(dummy_object_sdk, account_user4); + const tagging_param = { name: test_bucket_tagging, tagging: [{ key: 'test-key', value: 'test-value' }] }; + await bucketspace_fs.put_bucket_tagging(tagging_param, dummy_object_sdk_for_account); + const tagging = await bucketspace_fs.get_bucket_tagging({ name: test_bucket_tagging }); + assert.equal(tagging.tagging.length, 1); + assert.equal(tagging.tagging[0].key, 'test-key'); + }); + + mocha.after(async function() { + await fs_utils.folder_delete(`${new_buckets_path}/${test_bucket_tagging}`); + const file_path = get_config_file_path(CONFIG_SUBDIRS.BUCKETS, test_bucket_tagging); + await fs_utils.file_delete(file_path); + }); }); mocha.describe('bucket lifecycle operations', function() {