From 4e128ba3be1c130b72ade2432913ecec68bef23b Mon Sep 17 00:00:00 2001 From: Ramon Date: Wed, 16 Mar 2022 10:09:20 +0100 Subject: [PATCH] Updat GenericWildcardPrincipalRule to be compatible with the Generic (#211) * updated GenericWildcardPrincipalRule to be compatible with the Generic resource * fix formatting * remove whitespace Co-authored-by: Ramon --- CHANGELOG.md | 6 ++ cfripper/__version__.py | 2 +- cfripper/rules/wildcard_principals.py | 73 +++++++++++++------- tests/rules/test_GenericWildcardPrincipal.py | 2 +- 4 files changed, 55 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cdb2194..8f2485f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,12 @@ # Changelog All notable changes to this project will be documented in this file. +## [1.5.3] +### Updates +- Updates `GenericWildcardPrincipalRule` to understand the `GenericResource`. +### Fixes +- Stopped using `_statement_as_list()` when retrieving statements in favor of `statement_as_list()`. + ## [1.5.2] ### Updates - Updates `WildcardResourceRule` for a better use with the `GenericResource`. diff --git a/cfripper/__version__.py b/cfripper/__version__.py index b2e822f6..197e1827 100644 --- a/cfripper/__version__.py +++ b/cfripper/__version__.py @@ -1,3 +1,3 @@ -VERSION = (1, 5, 2) +VERSION = (1, 5, 3) __version__ = ".".join(map(str, VERSION)) diff --git a/cfripper/rules/wildcard_principals.py b/cfripper/rules/wildcard_principals.py index 432379e8..ecf00578 100644 --- a/cfripper/rules/wildcard_principals.py +++ b/cfripper/rules/wildcard_principals.py @@ -4,14 +4,9 @@ from typing import Dict, Optional from pycfmodel.model.cf_model import CFModel -from pycfmodel.model.resources.iam_managed_policy import IAMManagedPolicy -from pycfmodel.model.resources.iam_policy import IAMPolicy from pycfmodel.model.resources.iam_role import IAMRole -from pycfmodel.model.resources.iam_user import IAMUser +from pycfmodel.model.resources.kms_key import KMSKey from pycfmodel.model.resources.properties.policy_document import PolicyDocument -from pycfmodel.model.resources.s3_bucket_policy import S3BucketPolicy -from pycfmodel.model.resources.sns_topic_policy import SNSTopicPolicy -from pycfmodel.model.resources.sqs_queue_policy import SQSQueuePolicy from cfripper.config.regex import REGEX_FULL_WILDCARD_PRINCIPAL, REGEX_PARTIAL_WILDCARD_PRINCIPAL from cfripper.model.enums import RuleGranularity, RuleRisk @@ -22,6 +17,31 @@ class GenericWildcardPrincipalRule(PrincipalCheckingRule): + """ + Checks for any wildcard principal defined in any statement. + To be inherited into more precise rules. + Ignores KMS Keys, since they have `KMSKeyWildcardPrincipalRule`. + For IAM Roles, it also checks `AssumeRolePolicyDocument`. + + Risk: + It might allow other AWS identities to escalate privileges. + + Fix: + Where possible, restrict the access to only the required resources. + For example, instead of `Principal: "*"`, include a list of the roles that need access. + + Filters context: + | Parameter | Type | Description | + |:-----------:|:------------------:|:--------------------------------------------------------------:| + |`config` | `str` | `config` variable available inside the rule | + |`extras` | `str` | `extras` variable available inside the rule | + |`logical_id` | `str` | ID used in CloudFormation to refer the resource being analysed | + |`resource` | `S3BucketPolicy` | Resource that is being addressed | + |`statement` | `Statement` | Statement being checked found in the Resource | + |`principal` | `str` | AWS Principal being checked found in the statement | + |`account_id` | `str` | Account ID found in the principal | + """ + REASON_WILDCARD_PRINCIPAL = "{} should not allow wildcards in principals (principal: '{}')" GRANULARITY = RuleGranularity.RESOURCE @@ -32,20 +52,21 @@ class GenericWildcardPrincipalRule(PrincipalCheckingRule): def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result: result = Result() for logical_id, resource in cfmodel.Resources.items(): - if isinstance(resource, (IAMManagedPolicy, IAMPolicy, S3BucketPolicy, SNSTopicPolicy, SQSQueuePolicy)): - self.check_for_wildcards(result, logical_id, resource.Properties.PolicyDocument, extras) - elif isinstance(resource, (IAMRole, IAMUser)): - if isinstance(resource, IAMRole): - self.check_for_wildcards(result, logical_id, resource.Properties.AssumeRolePolicyDocument, extras) - if resource.Properties and resource.Properties.Policies: - for policy in resource.Properties.Policies: - self.check_for_wildcards(result, logical_id, policy.PolicyDocument, extras) + if isinstance(resource, KMSKey): + # Ignoring KMSKey because there's already a rule for it `KMSKeyWildcardPrincipalRule` + continue + if isinstance(resource, IAMRole): + # Checking the `AssumeRolePolicyDocument` for IAM Roles + self.check_for_wildcards(result, logical_id, resource.Properties.AssumeRolePolicyDocument, extras) + for policy in resource.policy_documents: + self.check_for_wildcards(result, logical_id, policy.policy_document, extras) + return result def check_for_wildcards( self, result: Result, logical_id: str, resource: PolicyDocument, extras: Optional[Dict] = None ): - for statement in resource._statement_as_list(): + for statement in resource.statement_as_list(): if statement.Effect == "Allow" and statement.principals_with(self.FULL_REGEX): for principal in statement.get_principal_list(): account_id_match = self.IAM_PATTERN.match(principal) or self.AWS_ACCOUNT_ID_PATTERN.match(principal) @@ -92,13 +113,13 @@ class PartialWildcardPrincipalRule(GenericWildcardPrincipalRule): Filters context: | Parameter | Type | Description | |:-----------:|:------------------:|:--------------------------------------------------------------:| - |`config` | str | `config` variable available inside the rule | - |`extras` | str | `extras` variable available inside the rule | - |`logical_id` | str | ID used in Cloudformation to refer the resource being analysed | + |`config` | `str` | `config` variable available inside the rule | + |`extras` | `str` | `extras` variable available inside the rule | + |`logical_id` | `str` | ID used in CloudFormation to refer the resource being analysed | |`resource` | `S3BucketPolicy` | Resource that is being addressed | |`statement` | `Statement` | Statement being checked found in the Resource | - |`principal` | str | AWS Principal being checked found in the statement | - |`account_id` | str | Account ID found in the principal | + |`principal` | `str` | AWS Principal being checked found in the statement | + |`account_id` | `str` | Account ID found in the principal | """ REASON_WILDCARD_PRINCIPAL = ( @@ -110,7 +131,7 @@ class PartialWildcardPrincipalRule(GenericWildcardPrincipalRule): class FullWildcardPrincipalRule(GenericWildcardPrincipalRule): """ - Checks for any wildcard principals defined in any statements. + Checks for any wildcard principal defined in any statement. Risk: It might allow other AWS identities to escalate privileges. @@ -122,13 +143,13 @@ class FullWildcardPrincipalRule(GenericWildcardPrincipalRule): Filters context: | Parameter | Type | Description | |:-----------:|:------------------:|:--------------------------------------------------------------:| - |`config` | str | `config` variable available inside the rule | - |`extras` | str | `extras` variable available inside the rule | - |`logical_id` | str | ID used in Cloudformation to refer the resource being analysed | + |`config` | `str` | `config` variable available inside the rule | + |`extras` | `str` | `extras` variable available inside the rule | + |`logical_id` | `str` | ID used in CloudFormation to refer the resource being analysed | |`resource` | `S3BucketPolicy` | Resource that is being addressed | |`statement` | `Statement` | Statement being checked found in the Resource | - |`principal` | str | AWS Principal being checked found in the statement | - |`account_id` | str | Account ID found in the principal | + |`principal` | `str` | AWS Principal being checked found in the statement | + |`account_id` | `str` | Account ID found in the principal | """ RISK_VALUE = RuleRisk.HIGH diff --git a/tests/rules/test_GenericWildcardPrincipal.py b/tests/rules/test_GenericWildcardPrincipal.py index d95d5c38..daf5c13f 100644 --- a/tests/rules/test_GenericWildcardPrincipal.py +++ b/tests/rules/test_GenericWildcardPrincipal.py @@ -55,7 +55,7 @@ def test_failures_are_raised(bad_template): ) -def test_generic_wildcard_ignores_kms(): +def test_generic_wildcard_ignores_kms_keys_since_they_have_another_rule_for_them(): rule = GenericWildcardPrincipalRule(Config(aws_account_id="123456789", aws_principals=["999999999"])) model = get_cfmodel_from("rules/CrossAccountTrustRule/kms_basic.yml").resolve( extra_params={"Principal": "arn:aws:iam::*:*"}