Various improvements, version 0.22.0 (#137)

CHANGELOG.md

@@ -1,6 +1,14 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [0.22.0] - 2020-12-11
+### Breaking changes
+- Classes inheriting from `ResourceSpecificRule` now must allow an `extra` field in the `resource_invoke` function
+### Improvements
+- Improved context data for `BaseDangerousPolicyActions` and classes inheriting from it
+### Bugfix
+- `CrossAccountCheckingRule` did not check properly for calculated mock fields. 
+
 ## [0.21.1] - 2020-12-9
 ### Improvements
 - Add SNS actions that only allow wildcards

cfripper/__version__.py

@@ -1,3 +1,3 @@
-VERSION = (0, 21, 1)
+VERSION = (0, 22, 0)
 
 __version__ = ".".join(map(str, VERSION))

cfripper/rules/base_rules.py

@@ -110,11 +110,11 @@ def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
         result = Result()
         for logical_id, resource in cfmodel.Resources.items():
             if isinstance(resource, self.RESOURCE_TYPES):
-                result += self.resource_invoke(resource=resource, logical_id=logical_id)
+                result += self.resource_invoke(resource=resource, logical_id=logical_id, extras=extras)
         return result
 
     @abstractmethod
-    def resource_invoke(self, resource: Resource, logical_id: str) -> Result:
+    def resource_invoke(self, resource: Resource, logical_id: str, extras: Optional[Dict] = None) -> Result:
         pass
 
 
@@ -149,6 +149,17 @@ class BaseDangerousPolicyActions(ResourceSpecificRule, ABC):
     Base class for dangerous actions. Admits a DANGEROUS_ACTIONS class variable with a list of dangerous actions
     """
 
+    DEFAULT_FILTERS_CONTEXT = """\
+    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  |
+        |`policy_name` | `Optional[str]`  | If available, the policy name                                   |
+        |`action`      | `List[str]`      | List of dangerous actions contained within the policy           |
+    """
+
     REASON = "Resource {} should not include the following dangerous actions: {}"
     RISK_VALUE = RuleRisk.HIGH
     GRANULARITY = RuleGranularity.ACTION
@@ -158,9 +169,9 @@ class BaseDangerousPolicyActions(ResourceSpecificRule, ABC):
     @abstractmethod
     def DANGEROUS_ACTIONS(cls) -> List[str]:
         # This is designed to be overwritten as a class variable
-        return NotImplementedError
+        raise NotImplementedError
 
-    def resource_invoke(self, resource: Resource, logical_id: str) -> Result:
+    def resource_invoke(self, resource: Resource, logical_id: str, extras: Optional[Dict] = None) -> Result:
         result = Result()
         for policy in resource.policy_documents:
             actions = policy.policy_document.get_allowed_actions()
@@ -171,5 +182,12 @@ def resource_invoke(self, resource: Resource, logical_id: str) -> Result:
                     self.REASON.format(logical_id, sorted(dangerous_actions)),
                     resource_ids={logical_id},
                     actions=dangerous_actions,
+                    context={
+                        "config": self._config,
+                        "extras": extras,
+                        "logical_id": logical_id,
+                        "policy_name": policy.name,
+                        "actions": sorted(dangerous_actions),
+                    },
                 )
         return result

cfripper/rules/cross_account_trust.py

@@ -6,6 +6,7 @@
 ]
 
 import logging
+from abc import ABC
 from typing import Dict, Optional, Set
 
 from pycfmodel.model.cf_model import CFModel
@@ -23,7 +24,7 @@
 logger = logging.getLogger(__file__)
 
 
-class CrossAccountCheckingRule(PrincipalCheckingRule):
+class CrossAccountCheckingRule(PrincipalCheckingRule, ABC):
     """
     Base class not intended to be instantiated, but inherited from.
     This class provides common methods used to detect access permissions from other accounts.
@@ -84,7 +85,7 @@ def _do_statement_check(
                             f"Not adding {type(self).__name__} failure in {logical_id} "
                             f"because no AWS Account ID was found in the config."
                         )
-                    elif "GETATT" in principal or "UNDEFINED_" in principal:
+                    elif principal.startswith("GETATT") or principal.startswith("UNDEFINED_"):
                         self.add_failure_to_result(
                             result,
                             self.REASON.format(logical_id, principal),

cfripper/rules/privilege_escalation.py

@@ -7,14 +7,17 @@
 
 
 class PrivilegeEscalationRule(BaseDangerousPolicyActions):
-    """
+    f"""
     Checks for any dangerous IAM actions that could allow privilege escalation and potentially
     represent a large security risk.
-    See [current blacklisted IAM actions](https://github.com/Skyscanner/cfripper/blob/master/cfripper/rules/privilege_escalation.py#L29).
+    See [current blacklisted IAM actions]\
+(https://github.com/Skyscanner/cfripper/blob/master/cfripper/rules/privilege_escalation.py#L25).
 
     Fix:
         Unless strictly necessary, do not use actions in the IAM action blacklist. CloudFormation files that do require
         these actions should be added to the whitelist.
+
+    {BaseDangerousPolicyActions.DEFAULT_FILTERS_CONTEXT}
     """
 
     GRANULARITY = RuleGranularity.ACTION

cfripper/rules/s3_bucket_policy.py

@@ -1,5 +1,6 @@
 __all__ = ["S3BucketPolicyPrincipalRule"]
 import logging
+from typing import Dict, Optional
 
 from pycfmodel.model.resources.s3_bucket_policy import S3BucketPolicy
 
@@ -29,7 +30,7 @@ class S3BucketPolicyPrincipalRule(PrincipalCheckingRule, ResourceSpecificRule):
     RISK_VALUE = RuleRisk.HIGH
     RESOURCE_TYPES = (S3BucketPolicy,)
 
-    def resource_invoke(self, resource: S3BucketPolicy, logical_id: str) -> Result:
+    def resource_invoke(self, resource: S3BucketPolicy, logical_id: str, extras: Optional[Dict] = None) -> Result:
         result = Result()
 
         for statement in resource.Properties.PolicyDocument._statement_as_list():

cfripper/rules/sns_topic_policy.py

@@ -1,5 +1,6 @@
 __all__ = ["SNSTopicPolicyNotPrincipalRule", "SNSTopicDangerousPolicyActionsRule"]
 
+from typing import Dict, Optional
 
 from pycfmodel.model.resources.sns_topic_policy import SNSTopicPolicy
 
@@ -22,7 +23,7 @@ class SNSTopicPolicyNotPrincipalRule(ResourceSpecificRule):
     REASON = "SNS Topic policy {} should not allow Allow and NotPrincipal at the same time"
     RESOURCE_TYPES = (SNSTopicPolicy,)
 
-    def resource_invoke(self, resource: SNSTopicPolicy, logical_id: str) -> Result:
+    def resource_invoke(self, resource: SNSTopicPolicy, logical_id: str, extras: Optional[Dict] = None) -> Result:
         result = Result()
         for statement in resource.Properties.PolicyDocument._statement_as_list():
             if statement.NotPrincipal:
@@ -31,11 +32,13 @@ def resource_invoke(self, resource: SNSTopicPolicy, logical_id: str) -> Result:
 
 
 class SNSTopicDangerousPolicyActionsRule(BaseDangerousPolicyActions):
-    """
+    f"""
     Checks for dangerous permissions in Action statements in an SNS Topic Policy.
 
     Risk:
         This is deemed a potential security risk as it could allow privilege escalation.
+
+    {BaseDangerousPolicyActions.DEFAULT_FILTERS_CONTEXT}
     """
 
     REASON = "SNS Topic policy {} should not not include the following dangerous actions: {}"

cfripper/rules/sqs_queue_policy.py

@@ -1,6 +1,7 @@
 __all__ = ["SQSQueuePolicyNotPrincipalRule", "SQSQueuePolicyPublicRule", "SQSDangerousPolicyActionsRule"]
 
 import logging
+from typing import Dict, Optional
 
 from pycfmodel.model.resources.sqs_queue_policy import SQSQueuePolicy
 
@@ -26,7 +27,7 @@ class SQSQueuePolicyNotPrincipalRule(ResourceSpecificRule):
     RESOURCE_TYPES = (SQSQueuePolicy,)
     REASON = "SQS Queue policy {} should not allow Allow and NotPrincipal at the same time"
 
-    def resource_invoke(self, resource: SQSQueuePolicy, logical_id: str) -> Result:
+    def resource_invoke(self, resource: SQSQueuePolicy, logical_id: str, extras: Optional[Dict] = None) -> Result:
         result = Result()
         for statement in resource.Properties.PolicyDocument._statement_as_list():
             if statement.NotPrincipal:
@@ -47,7 +48,7 @@ class SQSQueuePolicyPublicRule(ResourceSpecificRule):
     GRANULARITY = RuleGranularity.RESOURCE
     RESOURCE_TYPES = (SQSQueuePolicy,)
 
-    def resource_invoke(self, resource: SQSQueuePolicy, logical_id: str) -> Result:
+    def resource_invoke(self, resource: SQSQueuePolicy, logical_id: str, extras: Optional[Dict] = None) -> Result:
         result = Result()
         if resource.Properties.PolicyDocument.allowed_principals_with(REGEX_HAS_STAR_OR_STAR_AFTER_COLON):
             for statement in resource.Properties.PolicyDocument._statement_as_list():
@@ -63,11 +64,13 @@ def resource_invoke(self, resource: SQSQueuePolicy, logical_id: str) -> Result:
 
 
 class SQSDangerousPolicyActionsRule(BaseDangerousPolicyActions):
-    """
+    f"""
     Checks for dangerous permissions in Action statements in an SQS Queue Policy.
 
     Risk:
         This is deemed a potential security risk as it'd allow various attacks to the queue.
+
+    {BaseDangerousPolicyActions.DEFAULT_FILTERS_CONTEXT}
     """
 
     REASON = "SQS Queue policy {} should not not include the following dangerous actions: {}"