From e6fa89aa5b3fe5ce2acc99a0090460ed08afef92 Mon Sep 17 00:00:00 2001 From: Matt Skillman Date: Tue, 4 Aug 2020 23:43:19 -0400 Subject: [PATCH 1/6] wip; working on unit tests and need to confirm intended changes to iamRolePolicies behavior --- plugins/aws/iam/iamRolePolicies.js | 48 +++++--- plugins/aws/iam/iamRolePolicies.spec.js | 144 ++++++++++++++++++++++++ 2 files changed, 174 insertions(+), 18 deletions(-) create mode 100644 plugins/aws/iam/iamRolePolicies.spec.js diff --git a/plugins/aws/iam/iamRolePolicies.js b/plugins/aws/iam/iamRolePolicies.js index 78ba4d659f..e6b1f2c243 100644 --- a/plugins/aws/iam/iamRolePolicies.js +++ b/plugins/aws/iam/iamRolePolicies.js @@ -18,19 +18,26 @@ module.exports = { description: 'Ignores roles that contain the provided exact-match path', regex: '^[0-9A-Za-z/._-]{3,512}$', default: false - } + }, + only_look_at_action_star_effect_allow: { + name: 'Only Look At Action Star Effect Allow', + description: 'enable this to consider only those roles which allow all actions', + regex: '^[0-1]$', + default: false + }, }, run: function(cache, settings, callback) { var config = { - iam_role_policies_ignore_path: settings.iam_role_policies_ignore_path || this.settings.iam_role_policies_ignore_path.default + iam_role_policies_ignore_path: settings.iam_role_policies_ignore_path || this.settings.iam_role_policies_ignore_path.default, + only_look_at_action_star_effect_allow: settings.only_look_at_action_star_effect_allow || this.settings.only_look_at_action_star_effect_allow }; var custom = helpers.isCustom(settings, this.settings); var results = []; var source = {}; - + var region = helpers.defaultRegion(settings); var listRoles = helpers.addSource(cache, source, @@ -109,7 +116,7 @@ module.exports = { var policyName = listRolePolicies.data.PolicyNames[p]; if (getRolePolicy && - getRolePolicy[policyName] && + getRolePolicy[policyName] && getRolePolicy[policyName].data && getRolePolicy[policyName].data.PolicyDocument) { @@ -124,23 +131,28 @@ module.exports = { if (statement.Effect === 'Allow' && !statement.Condition) { var failMsg; - if (statement.Action.indexOf('*') > -1 && - statement.Resource && - statement.Resource.indexOf('*') > -1) { - failMsg = 'Role inline policy allows all actions on all resources'; - } else if (statement.Action.indexOf('*') > -1) { - failMsg = 'Role inline policy allows all actions on selected resources'; - } else if (statement.Action && statement.Action.length) { - // Check each action for wildcards - var wildcards = []; - for (a in statement.Action) { - if (statement.Action[a].endsWith(':*')) { - wildcards.push(statement.Action[a]); + if (config.only_look_at_action_star_effect_allow) { + if (statement.Action === '*') { + failMsg = 'wildcard action allowed on all or selected resources'; // TODO this message seems like insufficient information. + } + } else { + if (statement.Action.indexOf('*') > -1 && + statement.Resource && + statement.Resource.indexOf('*') > -1) { + failMsg = 'Role inline policy allows all actions on all resources'; + } else if (statement.Action.indexOf('*') > -1) { + failMsg = 'Role inline policy allows all actions on selected resources'; + } else if (statement.Action && statement.Action.length) { + // Check each action for wildcards + var wildcards = []; + for (a in statement.Action) { + if (statement.Action[a].endsWith(':*')) { + wildcards.push(statement.Action[a]); + } } + if (wildcards.length) failMsg = 'Role inline policy allows wildcard actions: ' + wildcards.join(', '); } - if (wildcards.length) failMsg = 'Role inline policy allows wildcard actions: ' + wildcards.join(', '); } - if (failMsg && roleFailures.indexOf(failMsg) === -1) roleFailures.push(failMsg); } } diff --git a/plugins/aws/iam/iamRolePolicies.spec.js b/plugins/aws/iam/iamRolePolicies.spec.js new file mode 100644 index 0000000000..9286eafc10 --- /dev/null +++ b/plugins/aws/iam/iamRolePolicies.spec.js @@ -0,0 +1,144 @@ +var assert = require('assert'); +var expect = require('chai').expect; +var iamUserAdmins = require('./iamRolePolicies'); + +// apis needed in cache: +/* apis: ['IAM:listRoles', 'IAM:listRolePolicies', 'IAM:listAttachedRolePolicies', + 'IAM:getPolicy', 'IAM:getRolePolicy'], + */ +const cache = { // TODO these aren't right, just copied over + "iam": { + "listRoles": { + "us-east-1": { + "data": [ + { + "RoleName": "test1@turner.com", + }, + { + "RoleName": "test2@turner.com", + }, + ] + } + }, + "listRolePolicies": { + "us-east-1": { + "test1@turner.com": { + "data": { + "AttachedPolicies": [{"PolicyArn":"arn:aws:iam::aws:policy/AdministratorAccess"}], + } + }, + "test2@turner.com": { + "data": { + "AttachedPolicies": [{"PolicyArn":"arn:aws:iam::aws:policy/AdministratorAccess"}], + } + }, + } + }, + "listAttachedRolePolicies": { + "us-east-1": { + "test1@turner.com": { + "data": { + "PolicyNames": [], + } + }, + "test2@turner.com": { + "data": { + "PolicyNames": [], + } + }, + } + }, + "getPolicy": { + "us-east-1": { + "test1@turner.com": { + "data": { + "Groups": [], + } + }, + "test2@turner.com": { + "data": { + "Groups": [], + } + }, + } + }, + "getRolePolicy": { + "us-east-1": { + "test1@turner.com": { + "data": { + "Groups": [], + } + }, + "test2@turner.com": { + "data": { + "Groups": [], + } + }, + } + } + } + } + + +describe('iamUserAdmins', function () { + describe('run', function () { + it('should FAIL when no users are found', function (done) { + const settings = { + iam_admin_count_minimum: 2, + iam_admin_count_maximum: 2 + } + + const cache = [{}]; + + + const callback = (err, results) => { + expect(results.length).to.equal(0) + done() + } + + iamUserAdmins.run(cache, settings, callback) + }) + + it('should FAIL when there are not enough users', function (done) { + const settings = { + iam_admin_count_minimum: 3, + iam_admin_count_maximum: 3 + } + + const callback = (err, results) => { + expect(results[0].status).to.equal(1) + done() + } + + iamUserAdmins.run(cache, settings, callback) + }) + + it('should FAIL when there are too many users', function (done) { + const settings = { + iam_admin_count_minimum: 0, + iam_admin_count_maximum: 0 + } + + const callback = (err, results) => { + expect(results[0].status).to.equal(2) + done() + } + + iamUserAdmins.run(cache, settings, callback) + }) + + it('should PASS when users are found and fit within range', function (done) { + const settings = { + iam_admin_count_minimum: 1, + iam_admin_count_maximum: 8 + } + + const callback = (err, results) => { + expect(results[0].status).to.equal(0) + done() + } + + iamUserAdmins.run(cache, settings, callback) + }) + }) +}) From 0ef75e92a1200077bd623e43d8e3be14b6e0e387 Mon Sep 17 00:00:00 2001 From: Matt Skillman Date: Fri, 7 Aug 2020 01:15:03 -0400 Subject: [PATCH 2/6] addition of config parameter to optionally change behavior of this plugin. additionally test cases added for both existing and added functionality. --- plugins/aws/iam/iamRolePolicies.js | 37 ++- plugins/aws/iam/iamRolePolicies.spec.js | 304 +++++++++++++++--------- 2 files changed, 211 insertions(+), 130 deletions(-) diff --git a/plugins/aws/iam/iamRolePolicies.js b/plugins/aws/iam/iamRolePolicies.js index e6b1f2c243..dd5ce41356 100644 --- a/plugins/aws/iam/iamRolePolicies.js +++ b/plugins/aws/iam/iamRolePolicies.js @@ -10,8 +10,7 @@ module.exports = { more_info: 'Policies attached to IAM roles should be scoped to least-privileged access and avoid the use of wildcards.', link: 'https://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles.html', recommended_action: 'Ensure that all IAM roles are scoped to specific services and API calls.', - apis: ['IAM:listRoles', 'IAM:listRolePolicies', 'IAM:listAttachedRolePolicies', - 'IAM:getPolicy', 'IAM:getRolePolicy'], + apis: ['IAM:listRoles', 'IAM:listRolePolicies', 'IAM:listAttachedRolePolicies', 'IAM:getRolePolicy'], settings: { iam_role_policies_ignore_path: { name: 'IAM Role Policies Ignore Path', @@ -22,7 +21,7 @@ module.exports = { only_look_at_action_star_effect_allow: { name: 'Only Look At Action Star Effect Allow', description: 'enable this to consider only those roles which allow all actions', - regex: '^[0-1]$', + regex: '^[1]$', // set as 1 to enable, omit config to disable default: false }, }, @@ -30,7 +29,7 @@ module.exports = { run: function(cache, settings, callback) { var config = { iam_role_policies_ignore_path: settings.iam_role_policies_ignore_path || this.settings.iam_role_policies_ignore_path.default, - only_look_at_action_star_effect_allow: settings.only_look_at_action_star_effect_allow || this.settings.only_look_at_action_star_effect_allow + only_look_at_action_star_effect_allow: settings.only_look_at_action_star_effect_allow || this.settings.only_look_at_action_star_effect_allow.default }; var custom = helpers.isCustom(settings, this.settings); @@ -90,7 +89,7 @@ module.exports = { return cb(); } - var roleFailures = []; + let roleFailures = []; // See if role has admin managed policy if (listAttachedRolePolicies && @@ -114,12 +113,10 @@ module.exports = { for (var p in listRolePolicies.data.PolicyNames) { var policyName = listRolePolicies.data.PolicyNames[p]; - if (getRolePolicy && getRolePolicy[policyName] && getRolePolicy[policyName].data && getRolePolicy[policyName].data.PolicyDocument) { - var statements = helpers.normalizePolicyDocument( getRolePolicy[policyName].data.PolicyDocument); if (!statements) break; @@ -130,30 +127,32 @@ module.exports = { if (statement.Effect === 'Allow' && !statement.Condition) { - var failMsg; + let failMsg = false; if (config.only_look_at_action_star_effect_allow) { - if (statement.Action === '*') { - failMsg = 'wildcard action allowed on all or selected resources'; // TODO this message seems like insufficient information. + if (statement.Action.includes("*") || statement.Action.includes('*:*')) { + failMsg = 'wildcard action allowed on '.concat(statement.Resource.join(', ')); } } else { - if (statement.Action.indexOf('*') > -1 && + if (statement.Action.includes('*') && statement.Resource && - statement.Resource.indexOf('*') > -1) { + statement.Resource.includes('*')) { failMsg = 'Role inline policy allows all actions on all resources'; - } else if (statement.Action.indexOf('*') > -1) { + } else if (statement.Action.includes('*')) { failMsg = 'Role inline policy allows all actions on selected resources'; } else if (statement.Action && statement.Action.length) { // Check each action for wildcards - var wildcards = []; - for (a in statement.Action) { - if (statement.Action[a].endsWith(':*')) { - wildcards.push(statement.Action[a]); + let wildcards = []; + for (var i in statement.Action) { + if (statement.Action[i].endsWith(':*')) { + wildcards.push(statement.Action[i]); } } - if (wildcards.length) failMsg = 'Role inline policy allows wildcard actions: ' + wildcards.join(', '); + if (wildcards.length) { + failMsg = 'Role inline policy allows wildcard actions: '.concat(wildcards.join(', ')); + } } } - if (failMsg && roleFailures.indexOf(failMsg) === -1) roleFailures.push(failMsg); + if (failMsg) roleFailures.push(failMsg); } } } diff --git a/plugins/aws/iam/iamRolePolicies.spec.js b/plugins/aws/iam/iamRolePolicies.spec.js index 9286eafc10..19445d33ef 100644 --- a/plugins/aws/iam/iamRolePolicies.spec.js +++ b/plugins/aws/iam/iamRolePolicies.spec.js @@ -1,144 +1,226 @@ var assert = require('assert'); var expect = require('chai').expect; -var iamUserAdmins = require('./iamRolePolicies'); +var iamRolePolicies = require('./iamRolePolicies'); -// apis needed in cache: -/* apis: ['IAM:listRoles', 'IAM:listRolePolicies', 'IAM:listAttachedRolePolicies', - 'IAM:getPolicy', 'IAM:getRolePolicy'], - */ -const cache = { // TODO these aren't right, just copied over - "iam": { - "listRoles": { +// apis: ['IAM:listRoles', 'IAM:listRolePolicies', 'IAM:listAttachedRolePolicies','IAM:getRolePolicy'], +const cache = { + iam: { + listRoles: { "us-east-1": { - "data": [ + data: [ { - "RoleName": "test1@turner.com", + AssumeRolePolicyDocument: { + Version: "2012-10-17", + Statement: [ + { + Action: "sts:AssumeRole", + Principal: { + Service: "ec2.amazonaws.com" + }, + Effect: "Allow", + Sid: "" + } + ] + }, + RoleId: "AROAJ52OTH4H7LEXAMPLE", + CreateDate: "2013-05-11T00:02:27Z", + RoleName: "ExampleRole1", + Path: "/", + Arn: "arn:aws:iam::123456789012:role/ExampleRole1" }, { - "RoleName": "test2@turner.com", + AssumeRolePolicyDocument: { + Version: "2012-10-17", + Statement: [ + { + Action: "sts:AssumeRole", + Principal: { + Service: "elastictranscoder.amazonaws.com" + }, + Effect: "Allow", + Sid: "" + } + ] + }, + RoleId: "AROAI4QRP7UFT7EXAMPLE", + CreateDate: "2013-04-18T05:01:58Z", + RoleName: "emr-access", + Path: "/", + Arn: "arn:aws:iam::123456789012:role/emr-access" }, ] } }, - "listRolePolicies": { + listRolePolicies: { "us-east-1": { - "test1@turner.com": { - "data": { - "AttachedPolicies": [{"PolicyArn":"arn:aws:iam::aws:policy/AdministratorAccess"}], + ExampleRole1: { + data: { + PolicyNames: ["a", "b"] } }, - "test2@turner.com": { - "data": { - "AttachedPolicies": [{"PolicyArn":"arn:aws:iam::aws:policy/AdministratorAccess"}], + "emr-access": { + data: { + PolicyNames: ["c", "d"] } }, } }, - "listAttachedRolePolicies": { + listAttachedRolePolicies: { "us-east-1": { - "test1@turner.com": { - "data": { - "PolicyNames": [], + ExampleRole1: { + data: { + AttachedPolicies: [ + {PolicyName: "a", PolicyArn: "arn:aws:iam::aws:policy/a"}, + {PolicyName: "b", PolicyArn: "arn:aws:iam::aws:policy/b"} + ], + IsTruncated: false, } }, - "test2@turner.com": { - "data": { - "PolicyNames": [], + "emr-access": { + data: { + AttachedPolicies: [ + {PolicyName: "c", PolicyArn: "arn:aws:iam::aws:policy/c"}, + {PolicyName: "d", PolicyArn: "arn:aws:iam::aws:policy/d"} + ], + IsTruncated: false, } - }, + } } }, - "getPolicy": { + getRolePolicy: { "us-east-1": { - "test1@turner.com": { - "data": { - "Groups": [], - } - }, - "test2@turner.com": { - "data": { - "Groups": [], + ExampleRole1: { + a: { + data: { + RoleName: "ExampleRole1", + PolicyDocument: { + Statement: [ + { + Action: [ + "s3:ListBucket", + "s3:Put*", + "s3:Get*", + "s3:*MultipartUpload*" + ], + Resource: "*", + Effect: "Allow", + Sid: "1" + } + ] + }, + PolicyName: "a" + } + }, + b: { + data: { + RoleName: "ExampleRole1", + PolicyDocument: { + Statement: [ + { + Action: [ + "s3:ListBucket", + "s3:Put*", + "s3:Get*", + "s3:*MultipartUpload*" + ], + Resource: "*", + Effect: "Allow", + Sid: "1" + } + ] + }, + PolicyName: "b" + } } }, - } - }, - "getRolePolicy": { - "us-east-1": { - "test1@turner.com": { - "data": { - "Groups": [], - } - }, - "test2@turner.com": { - "data": { - "Groups": [], + "emr-access": { + c: { + data: { + RoleName: "emr-access", + PolicyDocument: { + Statement: [ + { + Action: [ + "s3:ListBucket", + "s3:Put*", + "s3:Get*", + "s3:*MultipartUpload*" + ], + Resource: "*", + Effect: "Allow", + Sid: "1" + } + ] + }, + PolicyName: "c" + } + }, + d: { + data: { + RoleName: "emr-access", + PolicyDocument: { + Statement: [ + { + Action: [ + "s3:*", + "s3:Put*", + "s3:Get*", + "s3:*MultipartUpload*" + ], + Resource: "*", + Effect: "Allow", + Sid: "1" + } + ] + }, + PolicyName: "d" + } } - }, - } + } + } } } - } + }; -describe('iamUserAdmins', function () { +describe('iamRolePolicies', function () { describe('run', function () { - it('should FAIL when no users are found', function (done) { - const settings = { - iam_admin_count_minimum: 2, - iam_admin_count_maximum: 2 - } - - const cache = [{}]; - - - const callback = (err, results) => { - expect(results.length).to.equal(0) - done() - } - - iamUserAdmins.run(cache, settings, callback) - }) - - it('should FAIL when there are not enough users', function (done) { - const settings = { - iam_admin_count_minimum: 3, - iam_admin_count_maximum: 3 - } - - const callback = (err, results) => { - expect(results[0].status).to.equal(1) - done() - } - - iamUserAdmins.run(cache, settings, callback) - }) - - it('should FAIL when there are too many users', function (done) { - const settings = { - iam_admin_count_minimum: 0, - iam_admin_count_maximum: 0 - } - - const callback = (err, results) => { - expect(results[0].status).to.equal(2) - done() - } - - iamUserAdmins.run(cache, settings, callback) - }) - - it('should PASS when users are found and fit within range', function (done) { - const settings = { - iam_admin_count_minimum: 1, - iam_admin_count_maximum: 8 - } - - const callback = (err, results) => { - expect(results[0].status).to.equal(0) - done() - } - - iamUserAdmins.run(cache, settings, callback) - }) - }) -}) + it('should PASS when actions end with wildcard but only_look_at_action_star_effect_allow is enabled', function (done) { + let settings = { + only_look_at_action_star_effect_allow: 1, + }; + + let callback = (err, results) => { + expect(results[0].status).to.equal(0); + done(); + }; + + iamRolePolicies.run(cache, settings, callback) + }); + + it('should FAIL when action is literally * and only_look_at_action_star_effect_allow is enabled', function (done) { + let settings = { + only_look_at_action_star_effect_allow: 1, + }; + + let copied_cache = JSON.parse(JSON.stringify(cache)); + copied_cache.iam.getRolePolicy["us-east-1"].ExampleRole1.a.data.PolicyDocument.Statement[0].Action = ["*"]; + let callback = (err, results) => { + expect(results[0].status).to.equal(2); + done(); + }; + + iamRolePolicies.run(copied_cache, settings, callback) + }); + + it('should FAIL when actions end with wildcard but only_look_at_action_star_effect_allow is disabled', function (done) { + let settings = {}; + let callback = (err, results) => { + expect(results[1].status).to.equal(2); + done(); + }; + + iamRolePolicies.run(cache, settings, callback) + }); + }); +}); \ No newline at end of file From f85d6b9f7076c3ea84ae029ab705817e4ef381c7 Mon Sep 17 00:00:00 2001 From: Matt Skillman Date: Wed, 12 Aug 2020 13:45:51 -0400 Subject: [PATCH 3/6] revision to logic of handling the ignore service specific wildcards setting; revision of test cases associated with this setting and associated plugin --- plugins/aws/iam/iamRolePolicies.js | 46 +++++++++++-------------- plugins/aws/iam/iamRolePolicies.spec.js | 31 ++++++++++++----- 2 files changed, 43 insertions(+), 34 deletions(-) diff --git a/plugins/aws/iam/iamRolePolicies.js b/plugins/aws/iam/iamRolePolicies.js index dd5ce41356..5a6235b8d9 100644 --- a/plugins/aws/iam/iamRolePolicies.js +++ b/plugins/aws/iam/iamRolePolicies.js @@ -18,10 +18,10 @@ module.exports = { regex: '^[0-9A-Za-z/._-]{3,512}$', default: false }, - only_look_at_action_star_effect_allow: { - name: 'Only Look At Action Star Effect Allow', + ignore_service_specific_wildcards: { + name: 'Ignore Service Specific Wildcards', description: 'enable this to consider only those roles which allow all actions', - regex: '^[1]$', // set as 1 to enable, omit config to disable + regex: '^[true|false]$', // string true or boolean true to enable, string false or boolean false to disable default: false }, }, @@ -29,9 +29,11 @@ module.exports = { run: function(cache, settings, callback) { var config = { iam_role_policies_ignore_path: settings.iam_role_policies_ignore_path || this.settings.iam_role_policies_ignore_path.default, - only_look_at_action_star_effect_allow: settings.only_look_at_action_star_effect_allow || this.settings.only_look_at_action_star_effect_allow.default + ignore_service_specific_wildcards: settings.ignore_service_specific_wildcards || this.settings.ignore_service_specific_wildcards.default }; + if (config.ignore_service_specific_wildcards === 'false') config.ignore_service_specific_wildcards = false; + var custom = helpers.isCustom(settings, this.settings); var results = []; @@ -128,29 +130,23 @@ module.exports = { if (statement.Effect === 'Allow' && !statement.Condition) { let failMsg = false; - if (config.only_look_at_action_star_effect_allow) { - if (statement.Action.includes("*") || statement.Action.includes('*:*')) { - failMsg = 'wildcard action allowed on '.concat(statement.Resource.join(', ')); - } - } else { - if (statement.Action.includes('*') && - statement.Resource && - statement.Resource.includes('*')) { - failMsg = 'Role inline policy allows all actions on all resources'; - } else if (statement.Action.includes('*')) { - failMsg = 'Role inline policy allows all actions on selected resources'; - } else if (statement.Action && statement.Action.length) { - // Check each action for wildcards - let wildcards = []; - for (var i in statement.Action) { - if (statement.Action[i].endsWith(':*')) { - wildcards.push(statement.Action[i]); - } - } - if (wildcards.length) { - failMsg = 'Role inline policy allows wildcard actions: '.concat(wildcards.join(', ')); + if (statement.Action.includes('*') || statement.Action.includes('*:*') && + statement.Resource && + statement.Resource.includes('*')) { + failMsg = 'Role inline policy allows all actions on all resources'; + } else if (statement.Action.includes('*') || statement.Action.includes('*:*')) { + failMsg = 'Role inline policy allows all actions on selected resources'; + } else if (statement.Action && statement.Action.length && !config.ignore_service_specific_wildcards) { + // Check each action for wildcards + let wildcards = []; + for (var i in statement.Action) { + if (statement.Action[i].endsWith(':*')) { + wildcards.push(statement.Action[i]); } } + if (wildcards.length) { + failMsg = 'Role inline policy allows wildcard actions: '.concat(wildcards.join(', ')); + } } if (failMsg) roleFailures.push(failMsg); } diff --git a/plugins/aws/iam/iamRolePolicies.spec.js b/plugins/aws/iam/iamRolePolicies.spec.js index 19445d33ef..a2363a5eeb 100644 --- a/plugins/aws/iam/iamRolePolicies.spec.js +++ b/plugins/aws/iam/iamRolePolicies.spec.js @@ -162,7 +162,7 @@ const cache = { Statement: [ { Action: [ - "s3:*", + "s3:*", //wildcard action "s3:Put*", "s3:Get*", "s3:*MultipartUpload*" @@ -185,23 +185,21 @@ const cache = { describe('iamRolePolicies', function () { describe('run', function () { - it('should PASS when actions end with wildcard but only_look_at_action_star_effect_allow is enabled', function (done) { + it('should PASS when actions end with wildcard but ignore_service_specific_wildcards is enabled', function (done) { let settings = { - only_look_at_action_star_effect_allow: 1, + ignore_service_specific_wildcards: true, }; let callback = (err, results) => { - expect(results[0].status).to.equal(0); + expect(results[1].status).to.equal(0); done(); }; iamRolePolicies.run(cache, settings, callback) }); - it('should FAIL when action is literally * and only_look_at_action_star_effect_allow is enabled', function (done) { - let settings = { - only_look_at_action_star_effect_allow: 1, - }; + it('should FAIL when action is literally *', function (done) { + let settings = {}; let copied_cache = JSON.parse(JSON.stringify(cache)); copied_cache.iam.getRolePolicy["us-east-1"].ExampleRole1.a.data.PolicyDocument.Statement[0].Action = ["*"]; @@ -213,8 +211,23 @@ describe('iamRolePolicies', function () { iamRolePolicies.run(copied_cache, settings, callback) }); - it('should FAIL when actions end with wildcard but only_look_at_action_star_effect_allow is disabled', function (done) { + it('should FAIL when action is literally *:*', function (done) { let settings = {}; + + let copied_cache = JSON.parse(JSON.stringify(cache)); + copied_cache.iam.getRolePolicy["us-east-1"].ExampleRole1.a.data.PolicyDocument.Statement[0].Action = ["*:*"]; + let callback = (err, results) => { + expect(results[0].status).to.equal(2); + done(); + }; + + iamRolePolicies.run(copied_cache, settings, callback) + }); + + it('should FAIL when actions end with wildcard but ignore_service_specific_wildcards is disabled', function (done) { + let settings = { + ignore_service_specific_wildcards: 'false', + }; let callback = (err, results) => { expect(results[1].status).to.equal(2); done(); From 0b5197e7adc99470207f7c155a312e3e46b6c1f6 Mon Sep 17 00:00:00 2001 From: Matt Skillman Date: Wed, 12 Aug 2020 14:13:21 -0400 Subject: [PATCH 4/6] correction to regex --- plugins/aws/iam/iamRolePolicies.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/aws/iam/iamRolePolicies.js b/plugins/aws/iam/iamRolePolicies.js index 5a6235b8d9..f278f447d7 100644 --- a/plugins/aws/iam/iamRolePolicies.js +++ b/plugins/aws/iam/iamRolePolicies.js @@ -21,7 +21,7 @@ module.exports = { ignore_service_specific_wildcards: { name: 'Ignore Service Specific Wildcards', description: 'enable this to consider only those roles which allow all actions', - regex: '^[true|false]$', // string true or boolean true to enable, string false or boolean false to disable + regex: '^(true|false)$', // string true or boolean true to enable, string false or boolean false to disable default: false }, }, From b8f2c362f163960f51564c648d6eebc16f7c4e3c Mon Sep 17 00:00:00 2001 From: Matt Skillman Date: Wed, 19 Aug 2020 08:53:25 -0400 Subject: [PATCH 5/6] addition of flag to exclude those roles which involve something to do with federated users --- plugins/aws/iam/iamRolePolicies.js | 18 +++++++++++++- plugins/aws/iam/iamRolePolicies.spec.js | 32 +++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/plugins/aws/iam/iamRolePolicies.js b/plugins/aws/iam/iamRolePolicies.js index f278f447d7..b2148030bd 100644 --- a/plugins/aws/iam/iamRolePolicies.js +++ b/plugins/aws/iam/iamRolePolicies.js @@ -3,6 +3,10 @@ var helpers = require('../../../helpers/aws'); var managedAdminPolicy = 'arn:aws:iam::aws:policy/AdministratorAccess'; +function hasFederatedUserRole(statement) { + return statement.Action.includes('sts:AssumeRoleWithSAML') || statement.Action.includes('sts:AssumeRoleWithWebIdentity'); +} + module.exports = { title: 'IAM Role Policies', category: 'IAM', @@ -24,15 +28,23 @@ module.exports = { regex: '^(true|false)$', // string true or boolean true to enable, string false or boolean false to disable default: false }, + ignore_identity_federation_roles: { + name: 'Ignore Identity Federation Roles', + description: 'enable this to ignore idp/saml trust roles', + regex: '^(true|false)$', // string true or boolean true to enable, string false or boolean false to disable + default: false + }, }, run: function(cache, settings, callback) { var config = { iam_role_policies_ignore_path: settings.iam_role_policies_ignore_path || this.settings.iam_role_policies_ignore_path.default, - ignore_service_specific_wildcards: settings.ignore_service_specific_wildcards || this.settings.ignore_service_specific_wildcards.default + ignore_service_specific_wildcards: settings.ignore_service_specific_wildcards || this.settings.ignore_service_specific_wildcards.default, + ignore_identity_federation_roles: settings.ignore_identity_federation_roles || this.settings.ignore_identity_federation_roles }; if (config.ignore_service_specific_wildcards === 'false') config.ignore_service_specific_wildcards = false; + if (config.ignore_identity_federation_roles === 'false') config.ignore_identity_federation_roles = false; var custom = helpers.isCustom(settings, this.settings); @@ -149,6 +161,10 @@ module.exports = { } } if (failMsg) roleFailures.push(failMsg); + if (hasFederatedUserRole(statement) && config.ignore_identity_federation_roles) { + roleFailures.length = 0; + break; // ignore this role and do not check other statements. + } } } } diff --git a/plugins/aws/iam/iamRolePolicies.spec.js b/plugins/aws/iam/iamRolePolicies.spec.js index a2363a5eeb..6b79847c81 100644 --- a/plugins/aws/iam/iamRolePolicies.spec.js +++ b/plugins/aws/iam/iamRolePolicies.spec.js @@ -224,6 +224,38 @@ describe('iamRolePolicies', function () { iamRolePolicies.run(copied_cache, settings, callback) }); + it('should PASS when action indicates federated identity role, has wildcards, and ignore_identity_federation_roles is enabled', function (done) { + let settings = { + ignore_identity_federation_roles: 'true' + }; + + let copied_cache = JSON.parse(JSON.stringify(cache)); + copied_cache.iam.getRolePolicy["us-east-1"].ExampleRole1.a.data.PolicyDocument.Statement[0].Action = ["*:*"]; + copied_cache.iam.getRolePolicy["us-east-1"].ExampleRole1.b.data.PolicyDocument.Statement[0].Action = ["sts:AssumeRoleWithWebIdentity"]; + let callback = (err, results) => { + expect(results[0].status).to.equal(0); + done(); + }; + + iamRolePolicies.run(copied_cache, settings, callback) + }); + + it('should FAIL when action indicates federated identity role, has wildcards, and ignore_identity_federation_roles is disabled', function (done) { + let settings = { + ignore_identity_federation_roles: 'false' + }; + + let copied_cache = JSON.parse(JSON.stringify(cache)); + copied_cache.iam.getRolePolicy["us-east-1"].ExampleRole1.a.data.PolicyDocument.Statement[0].Action = ["*:*"]; + copied_cache.iam.getRolePolicy["us-east-1"].ExampleRole1.b.data.PolicyDocument.Statement[0].Action = ["sts:AssumeRoleWithWebIdentity"]; + let callback = (err, results) => { + expect(results[0].status).to.equal(2); + done(); + }; + + iamRolePolicies.run(copied_cache, settings, callback) + }); + it('should FAIL when actions end with wildcard but ignore_service_specific_wildcards is disabled', function (done) { let settings = { ignore_service_specific_wildcards: 'false', From 90299715f001de2a2a9568b24fd8dfe7e6cc4466 Mon Sep 17 00:00:00 2001 From: Matt Skillman Date: Wed, 19 Aug 2020 11:16:27 -0400 Subject: [PATCH 6/6] correction of logic and associated test cases --- plugins/aws/iam/iamRolePolicies.js | 23 +++++++--- plugins/aws/iam/iamRolePolicies.spec.js | 60 +++++++++++++++---------- 2 files changed, 53 insertions(+), 30 deletions(-) diff --git a/plugins/aws/iam/iamRolePolicies.js b/plugins/aws/iam/iamRolePolicies.js index b2148030bd..e388c01cda 100644 --- a/plugins/aws/iam/iamRolePolicies.js +++ b/plugins/aws/iam/iamRolePolicies.js @@ -3,8 +3,15 @@ var helpers = require('../../../helpers/aws'); var managedAdminPolicy = 'arn:aws:iam::aws:policy/AdministratorAccess'; -function hasFederatedUserRole(statement) { - return statement.Action.includes('sts:AssumeRoleWithSAML') || statement.Action.includes('sts:AssumeRoleWithWebIdentity'); +function hasFederatedUserRole(policyDocument) { + // true iff every statement refers to federated user access + let statement; + for (statement of policyDocument.Statement) { + if (statement.Action !== 'sts:AssumeRoleWithSAML' && statement.Action !== 'sts:AssumeRoleWithWebIdentity'){ + return false; + } + } + return true; } module.exports = { @@ -71,7 +78,13 @@ module.exports = { async.each(listRoles.data, function(role, cb){ if (!role.RoleName) return cb(); - + if (hasFederatedUserRole(JSON.parse(decodeURIComponent(role.AssumeRolePolicyDocument))) && config.ignore_identity_federation_roles) { + helpers.addResult(results, 0, + 'Role is federated user role', + 'global', role.Arn, custom + ); + return cb(); + } // Skip roles with user-defined paths if (config.iam_role_policies_ignore_path && config.iam_role_policies_ignore_path.length && @@ -161,10 +174,6 @@ module.exports = { } } if (failMsg) roleFailures.push(failMsg); - if (hasFederatedUserRole(statement) && config.ignore_identity_federation_roles) { - roleFailures.length = 0; - break; // ignore this role and do not check other statements. - } } } } diff --git a/plugins/aws/iam/iamRolePolicies.spec.js b/plugins/aws/iam/iamRolePolicies.spec.js index 6b79847c81..57f05f7e87 100644 --- a/plugins/aws/iam/iamRolePolicies.spec.js +++ b/plugins/aws/iam/iamRolePolicies.spec.js @@ -2,6 +2,18 @@ var assert = require('assert'); var expect = require('chai').expect; var iamRolePolicies = require('./iamRolePolicies'); +function encodeCache(cache) { + let copied_cache = JSON.parse(JSON.stringify(cache)); + let x; + let data = []; + for (x of copied_cache.iam.listRoles['us-east-1'].data) { + x.AssumeRolePolicyDocument = encodeURIComponent(JSON.stringify(x.AssumeRolePolicyDocument)); + data.push(x); + } + copied_cache.iam.listRoles['us-east-1'].data = data; + return copied_cache; +} + // apis: ['IAM:listRoles', 'IAM:listRolePolicies', 'IAM:listAttachedRolePolicies','IAM:getRolePolicy'], const cache = { iam: { @@ -10,17 +22,17 @@ const cache = { data: [ { AssumeRolePolicyDocument: { - Version: "2012-10-17", - Statement: [ - { - Action: "sts:AssumeRole", - Principal: { - Service: "ec2.amazonaws.com" - }, - Effect: "Allow", - Sid: "" - } - ] + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "", + "Effect": "Allow", + "Principal": { + "AWS": "arn:aws:iam::000000000000:root" + }, + "Action": "sts:AssumeRole" + } + ] }, RoleId: "AROAJ52OTH4H7LEXAMPLE", CreateDate: "2013-05-11T00:02:27Z", @@ -195,7 +207,7 @@ describe('iamRolePolicies', function () { done(); }; - iamRolePolicies.run(cache, settings, callback) + iamRolePolicies.run(encodeCache(cache), settings, callback) }); it('should FAIL when action is literally *', function (done) { @@ -208,7 +220,7 @@ describe('iamRolePolicies', function () { done(); }; - iamRolePolicies.run(copied_cache, settings, callback) + iamRolePolicies.run(encodeCache(copied_cache), settings, callback) }); it('should FAIL when action is literally *:*', function (done) { @@ -221,39 +233,41 @@ describe('iamRolePolicies', function () { done(); }; - iamRolePolicies.run(copied_cache, settings, callback) + iamRolePolicies.run(encodeCache(copied_cache), settings, callback) }); - it('should PASS when action indicates federated identity role, has wildcards, and ignore_identity_federation_roles is enabled', function (done) { + it('should PASS when it is federated identity role, has wildcards, and ignore_identity_federation_roles is enabled', function (done) { let settings = { ignore_identity_federation_roles: 'true' }; - let copied_cache = JSON.parse(JSON.stringify(cache)); + copied_cache.iam.listRoles['us-east-1'].data[0].AssumeRolePolicyDocument.Statement[0].Action = 'sts:AssumeRoleWithSAML'; copied_cache.iam.getRolePolicy["us-east-1"].ExampleRole1.a.data.PolicyDocument.Statement[0].Action = ["*:*"]; - copied_cache.iam.getRolePolicy["us-east-1"].ExampleRole1.b.data.PolicyDocument.Statement[0].Action = ["sts:AssumeRoleWithWebIdentity"]; + copied_cache.iam.listRoles['us-east-1'].data[1].AssumeRolePolicyDocument.Statement[0].Action = 'sts:AssumeRoleWithWebIdentity'; let callback = (err, results) => { expect(results[0].status).to.equal(0); + expect(results[1].status).to.equal(0); done(); }; - iamRolePolicies.run(copied_cache, settings, callback) + iamRolePolicies.run(encodeCache(copied_cache), settings, callback) }); - it('should FAIL when action indicates federated identity role, has wildcards, and ignore_identity_federation_roles is disabled', function (done) { + it('should FAIL when it is federated identity role, has wildcards, and ignore_identity_federation_roles is disabled', function (done) { let settings = { ignore_identity_federation_roles: 'false' }; - let copied_cache = JSON.parse(JSON.stringify(cache)); + copied_cache.iam.listRoles['us-east-1'].data[0].AssumeRolePolicyDocument.Statement[0].Action = 'sts:AssumeRoleWithSAML'; copied_cache.iam.getRolePolicy["us-east-1"].ExampleRole1.a.data.PolicyDocument.Statement[0].Action = ["*:*"]; - copied_cache.iam.getRolePolicy["us-east-1"].ExampleRole1.b.data.PolicyDocument.Statement[0].Action = ["sts:AssumeRoleWithWebIdentity"]; + copied_cache.iam.listRoles['us-east-1'].data[1].AssumeRolePolicyDocument.Statement[0].Action = 'sts:AssumeRoleWithWebIdentity'; let callback = (err, results) => { expect(results[0].status).to.equal(2); + expect(results[1].status).to.equal(2); done(); }; - iamRolePolicies.run(copied_cache, settings, callback) + iamRolePolicies.run(encodeCache(copied_cache), settings, callback) }); it('should FAIL when actions end with wildcard but ignore_service_specific_wildcards is disabled', function (done) { @@ -265,7 +279,7 @@ describe('iamRolePolicies', function () { done(); }; - iamRolePolicies.run(cache, settings, callback) + iamRolePolicies.run(encodeCache(cache), settings, callback) }); }); }); \ No newline at end of file