Investigate and update non default rules (#97)

* promote CFauth rule and sec group egress rule to default rules * refactor more of the IAM rulesto be clearer and more concise * add tests for IAM Managed Policy refactor and delete old files * refactor S3 wildcard action on policy and SQS version to use new abstract general rule for wilcard actions on policy docs * move some test files to their new correct locations, update test files, and add a valid IAM role test for the IAMRoleWildcardActionOnPolicyRule * add some very basic documentaion to new WildcardPolicy rules * update CHANGELOG * apply PR comments, and add new test for SNS Topic Wildcard rule * Bump to version 0.13.0 as there may be breaking changes, and update CHANGELOG again * add breaking changes to the CHANGELOG
Skyscanner · Jan 22, 2020 · f874b04 · f874b04
1 parent 7d46c88
commit f874b04
Show file tree

Hide file tree

Showing 30 changed files with 693 additions and 360 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,7 +1,26 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
-## [0.12.2] - 2020-01-10
+## [0.13.0] - 2020-01-22
+### Changed
+- `CloudFormationAuthenticationRule` now in `MONITOR` mode and new test added
+- `IAMRoleWildcardActionOnPolicyRule` combines three previous unused rules in `IAMManagedPolicyWildcardActionRule`, `IAMRoleWildcardActionOnPermissionsPolicyRule`, and `IAMRoleWildcardActionOnTrustPolicyRule`
+- `IAMRoleWildcardActionOnPolicyRule` now in `DEBUG` mode
+- `S3BucketPolicyWildcardActionRule` has now been changed to be an instantiation of the new generic rule `GenericWildcardPolicyRule`. It is set in `DEBUG` mode
+- `S3BucketPolicyWildcardActionRule` has had updated regex filter to make it more aligned with both further rules to do with wildcards in actions, and the existing `SQSQueuePolicyWildcardActionRule`
+- `SQSQueuePolicyWildcardActionRule` has now been changed to be an instantiation of the new generic rule `GenericWildcardPolicyRule`. It is set in `DEBUG` mode
+- `SecurityGroupMissingEgressRule` now in `DEBUG` mode and a new test added
+- `SNSTopicPolicyWildcardActionRule` has beed added. It is an instantiation of the new generic rule `GenericWildcardPolicyRule`. It is set in `DEBUG` mode
+### Breaking changes
+- The following rules are no longer available:
+  - `IAMRoleWildcardActionOnPermissionsPolicyRule`
+  - `IAMRoleWildcardActionOnTrustPolicyRule`
+  - `IAMManagedPolicyWildcardActionRule`
+- The following rules have been moved:
+  - `S3BucketPolicyWildcardActionRule`
+  - `SQSQueuePolicyWildcardActionRule`
+
+## [0.12.2] - 2020-01-13
 ### Improvements
 - Documentation updated to show the risk of rules and possible fixes where available, 
 as well as a large set of updates to the content. The macros for parsing the documentation

diff --git a/cfripper/__version__.py b/cfripper/__version__.py
@@ -1,3 +1,3 @@
-VERSION = (0, 12, 2)
+VERSION = (0, 13, 0)
 
 __version__ = ".".join(map(str, VERSION))
diff --git a/cfripper/config/regex.py b/cfripper/config/regex.py
@@ -121,10 +121,12 @@
 Valid:
 - *
 - arn:aws:iam::437628376:*
+- sns:*
 Invalid:
 - arn:aws:s3:::my_corporate_bucket
 - arn:aws:s3:::my_corporate_bucket*
 - arn:aws:s3:::*my_corporate_bucket
 - potato
+- sns:Get*
 """
 REGEX_HAS_STAR_OR_STAR_AFTER_COLON = re.compile(r"^(\w*:)*\*$")
diff --git a/cfripper/rules/__init__.py b/cfripper/rules/__init__.py
@@ -22,70 +22,54 @@
 )
 from cfripper.rules.ebs_volume_has_sse import EBSVolumeHasSSERule
 from cfripper.rules.hardcoded_RDS_password import HardcodedRDSPasswordRule
-from cfripper.rules.iam_managed_policy_wildcard_action import IAMManagedPolicyWildcardActionRule
-from cfripper.rules.iam_roles import (
-    IAMRolesOverprivilegedRule,
-    IAMRoleWildcardActionOnPermissionsPolicyRule,
-    IAMRoleWildcardActionOnTrustPolicyRule,
-)
+from cfripper.rules.iam_roles import IAMRolesOverprivilegedRule, IAMRoleWildcardActionOnPolicyRule
 from cfripper.rules.kms_key_wildcard_principal import KMSKeyWildcardPrincipal
 from cfripper.rules.managed_policy_on_user import ManagedPolicyOnUserRule
 from cfripper.rules.policy_on_user import PolicyOnUserRule
 from cfripper.rules.privilege_escalation import PrivilegeEscalationRule
-from cfripper.rules.s3_bucket_policy import S3BucketPolicyPrincipalRule, S3BucketPolicyWildcardActionRule
+from cfripper.rules.s3_bucket_policy import S3BucketPolicyPrincipalRule
 from cfripper.rules.s3_public_access import S3BucketPublicReadAclAndListStatementRule, S3BucketPublicReadWriteAclRule
 from cfripper.rules.security_group import (
     SecurityGroupIngressOpenToWorld,
     SecurityGroupMissingEgressRule,
     SecurityGroupOpenToWorldRule,
 )
 from cfripper.rules.sns_topic_policy_not_principal import SNSTopicPolicyNotPrincipalRule
-from cfripper.rules.sqs_queue_policy import (
-    SQSQueuePolicyNotPrincipalRule,
-    SQSQueuePolicyPublicRule,
+from cfripper.rules.sqs_queue_policy import SQSQueuePolicyNotPrincipalRule, SQSQueuePolicyPublicRule
+from cfripper.rules.wildcard_policies import (
+    S3BucketPolicyWildcardActionRule,
+    SNSTopicPolicyWildcardActionRule,
     SQSQueuePolicyWildcardActionRule,
 )
-from cfripper.rules.wildcard_principals import (
-    FullWildcardPrincipalRule,
-    GenericWildcardPrincipalRule,
-    PartialWildcardPrincipalRule,
-)
+from cfripper.rules.wildcard_principals import FullWildcardPrincipalRule, PartialWildcardPrincipalRule
 
 DEFAULT_RULES = {
+    "CloudFormationAuthenticationRule": CloudFormationAuthenticationRule,
     "CrossAccountTrustRule": CrossAccountTrustRule,
     "EBSVolumeHasSSERule": EBSVolumeHasSSERule,
     "FullWildcardPrincipal": FullWildcardPrincipalRule,
     "HardcodedRDSPasswordRule": HardcodedRDSPasswordRule,
     "IAMRolesOverprivilegedRule": IAMRolesOverprivilegedRule,
+    "IAMRoleWildcardActionOnPolicyRule": IAMRoleWildcardActionOnPolicyRule,
     "KMSKeyCrossAccountTrustRule": KMSKeyCrossAccountTrustRule,
     "KMSKeyWildcardPrincipal": KMSKeyWildcardPrincipal,
     "ManagedPolicyOnUserRule": ManagedPolicyOnUserRule,
     "PartialWildcardPrincipal": PartialWildcardPrincipalRule,
     "PolicyOnUserRule": PolicyOnUserRule,
     "PrivilegeEscalationRule": PrivilegeEscalationRule,
     "S3BucketPolicyPrincipalRule": S3BucketPolicyPrincipalRule,
+    "S3BucketPolicyWildcardActionRule": S3BucketPolicyWildcardActionRule,
     "S3BucketPublicReadAclAndListStatementRule": S3BucketPublicReadAclAndListStatementRule,
     "S3BucketPublicReadWriteAclRule": S3BucketPublicReadWriteAclRule,
     "S3CrossAccountTrustRule": S3CrossAccountTrustRule,
     "SecurityGroupIngressOpenToWorld": SecurityGroupIngressOpenToWorld,
+    "SecurityGroupMissingEgressRule": SecurityGroupMissingEgressRule,
     "SecurityGroupOpenToWorldRule": SecurityGroupOpenToWorldRule,
     "SNSTopicPolicyNotPrincipalRule": SNSTopicPolicyNotPrincipalRule,
+    "SNSTopicPolicyWildcardActionRule": SNSTopicPolicyWildcardActionRule,
     "SQSQueuePolicyNotPrincipalRule": SQSQueuePolicyNotPrincipalRule,
     "SQSQueuePolicyPublicRule": SQSQueuePolicyPublicRule,
-}
-
-NON_DEFAULT_RULES = {
-    "CloudFormationAuthenticationRule": CloudFormationAuthenticationRule,
-    "GenericWildcardPrincipalRule": GenericWildcardPrincipalRule,
-    "IAMManagedPolicyWildcardActionRule": IAMManagedPolicyWildcardActionRule,
-    "IAMRoleWildcardActionOnPermissionsPolicyRule": IAMRoleWildcardActionOnPermissionsPolicyRule,
-    "IAMRoleWildcardActionOnTrustPolicyRule": IAMRoleWildcardActionOnTrustPolicyRule,
-    "S3BucketPolicyWildcardActionRule": S3BucketPolicyWildcardActionRule,
-    "SecurityGroupMissingEgressRule": SecurityGroupMissingEgressRule,
     "SQSQueuePolicyWildcardActionRule": SQSQueuePolicyWildcardActionRule,
 }
 
-BASE_CLASSES = {
-    "CrossAccountCheckingRule": CrossAccountCheckingRule,
-    "PrincipalCheckingRule": PrincipalCheckingRule,
-}
+BASE_CLASSES = {"CrossAccountCheckingRule": CrossAccountCheckingRule, "PrincipalCheckingRule": PrincipalCheckingRule}
diff --git a/cfripper/rules/cloudformation_authentication.py b/cfripper/rules/cloudformation_authentication.py
@@ -14,7 +14,7 @@
 """
 __all__ = ["CloudFormationAuthenticationRule"]
 
-from cfripper.model.enums import RuleGranularity
+from cfripper.model.enums import RuleGranularity, RuleMode
 from cfripper.model.rule import Rule
 
 
@@ -49,6 +49,7 @@ class CloudFormationAuthenticationRule(Rule):
     """
 
     REASON = "Hardcoded credentials in {}"
+    RULE_MODE = RuleMode.MONITOR
     GRANULARITY = RuleGranularity.RESOURCE
 
     def invoke(self, cfmodel):

diff --git a/cfripper/rules/iam_managed_policy_wildcard_action.py b/cfripper/rules/iam_managed_policy_wildcard_action.py
diff --git a/cfripper/rules/iam_roles.py b/cfripper/rules/iam_roles.py
@@ -12,16 +12,13 @@
 CONDITIONS OF ANY KIND, either express or implied. See the License for the
 specific language governing permissions and limitations under the License.
 """
-__all__ = [
-    "IAMRolesOverprivilegedRule",
-    "IAMRoleWildcardActionOnPermissionsPolicyRule",
-    "IAMRoleWildcardActionOnTrustPolicyRule",
-]
+__all__ = ["IAMRolesOverprivilegedRule", "IAMRoleWildcardActionOnPolicyRule"]
+from pycfmodel.model.resources.iam_managed_policy import IAMManagedPolicy
 from pycfmodel.model.resources.iam_role import IAMRole
 
 from cfripper.config.regex import REGEX_IS_STAR, REGEX_WILDCARD_POLICY_ACTION
 from cfripper.model.enums import RuleGranularity
-from cfripper.model.rule import Rule
+from cfripper.model.rule import Rule, RuleMode
 
 
 class IAMRolesOverprivilegedRule(Rule):
@@ -69,45 +66,44 @@ def check_inline_policies(self, logical_id, role):
                                 )
 
 
-class IAMRoleWildcardActionOnPermissionsPolicyRule(Rule):
+class IAMRoleWildcardActionOnPolicyRule(Rule):
     """
-    Checks if any IAM role is using a `*` character in any of its permissions policy.
-
-    Risk:
-        The principle of least privilege (POLP), an important concept in computer security, is the practice of
-        limiting access rights for users to the bare minimum permissions they need to perform their work.
+    Checks for use of wildcard characters in all IAM Role policies (including AssumeRolePolicyDocument)
+    and AWS Managed Policies
+    (https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies_managed-vs-inline.html).
     """
 
     GRANULARITY = RuleGranularity.RESOURCE
-    REASON = "IAM role {} should not allow any * action on its permissions policy {}"
+    REASON = "IAM role {} should not allow a `*` action on its {}"
+    RULE_MODE = RuleMode.DEBUG
 
     def invoke(self, cfmodel):
         for logical_id, resource in cfmodel.Resources.items():
             if isinstance(resource, IAMRole):
-                for policy in resource.Properties.Policies:
-                    if policy.PolicyDocument.allowed_actions_with(REGEX_WILDCARD_POLICY_ACTION):
-                        self.add_failure(
-                            type(self).__name__,
-                            self.REASON.format(logical_id, policy.PolicyName),
-                            resource_ids={logical_id},
-                        )
-
-
-class IAMRoleWildcardActionOnTrustPolicyRule(Rule):
-    """
-    Checks if any IAM role is using a `*` character in its trust policy.
-
-    Risk:
-        Do not allow any principal to assume your role.
-        This is a security risk as it allow a third party account to assume your role and escalate privileges in our account.
-    """
-
-    GRANULARITY = RuleGranularity.RESOURCE
-    REASON = "IAM role {} should not allow any * action on its trust policy"
-
-    def invoke(self, cfmodel):
-        for logical_id, resource in cfmodel.Resources.items():
-            if isinstance(resource, IAMRole) and resource.Properties.AssumeRolePolicyDocument.allowed_actions_with(
+                # check AssumeRolePolicyDocument.
+                if resource.Properties.AssumeRolePolicyDocument.allowed_actions_with(REGEX_WILDCARD_POLICY_ACTION):
+                    self.add_failure(
+                        type(self).__name__,
+                        self.REASON.format(logical_id, "AssumeRolePolicy"),
+                        resource_ids={logical_id},
+                    )
+
+                # check other policies of the IAM role.
+                if resource.Properties.Policies:
+                    for policy in resource.Properties.Policies:
+                        if policy.PolicyDocument.allowed_actions_with(REGEX_WILDCARD_POLICY_ACTION):
+                            self.add_failure(
+                                type(self).__name__,
+                                self.REASON.format(logical_id, f"{policy.PolicyName} policy"),
+                                resource_ids={logical_id},
+                            )
+
+            # check AWS::IAM::ManagedPolicy.
+            elif isinstance(resource, IAMManagedPolicy) and resource.Properties.PolicyDocument.allowed_actions_with(
                 REGEX_WILDCARD_POLICY_ACTION
             ):
-                self.add_failure(type(self).__name__, self.REASON.format(logical_id), resource_ids={logical_id})
+                self.add_failure(
+                    type(self).__name__,
+                    self.REASON.format(logical_id, "AWS::IAM::ManagedPolicy"),
+                    resource_ids={logical_id},
+                )
diff --git a/cfripper/rules/s3_bucket_policy.py b/cfripper/rules/s3_bucket_policy.py
@@ -12,14 +12,12 @@
 CONDITIONS OF ANY KIND, either express or implied. See the License for the
 specific language governing permissions and limitations under the License.
 """
-__all__ = ["S3BucketPolicyPrincipalRule", "S3BucketPolicyWildcardActionRule"]
+__all__ = ["S3BucketPolicyPrincipalRule"]
 import logging
-import re
 
 from pycfmodel.model.resources.s3_bucket_policy import S3BucketPolicy
 
 from cfripper.model.enums import RuleGranularity, RuleMode, RuleRisk
-from cfripper.model.rule import Rule
 from cfripper.model.utils import get_account_id_from_principal
 from cfripper.rules.base_rules import PrincipalCheckingRule
 
@@ -63,20 +61,3 @@ def invoke(self, cfmodel):
                                     self.REASON.format(logical_id, account_id),
                                     resource_ids={logical_id},
                                 )
-
-
-class S3BucketPolicyWildcardActionRule(Rule):
-    """
-    Checks if any S3 bucket policies contain wildcard actions.
-    """
-
-    GRANULARITY = RuleGranularity.RESOURCE
-    REASON = "S3 Bucket policy {} should not allow * action"
-    REGEX = re.compile(r"^(\w*:){0,1}\*$")
-
-    def invoke(self, cfmodel):
-        for logical_id, resource in cfmodel.Resources.items():
-            if isinstance(resource, S3BucketPolicy) and resource.Properties.PolicyDocument.allowed_actions_with(
-                self.REGEX
-            ):
-                self.add_failure(type(self).__name__, self.REASON.format(logical_id), resource_ids={logical_id})
diff --git a/cfripper/rules/security_group.py b/cfripper/rules/security_group.py
@@ -103,20 +103,60 @@ def invoke(self, cfmodel):
 
 class SecurityGroupMissingEgressRule(Rule):
     """
-    Checks if a security group has a missing egress rule.
+    Checks that Security Groups are defined with an egress policy, even if this is still allowing all
+    outbound traffic.
 
     Risk:
-        Missing egress rule in a security group means all traffic is allowed outbound.
-        Filtering egress traffic is a security best practice that will help to detect an attacker exfiltrating
-        data or performing an intrusion.
+        If no egress rule is specified, the default is to open all outbound traffic to the world. Whilst
+        some services may need this, it is usually the case that the security group can be locked down
+        more. A NAT instance for example may require a completely open egress policy.
+
+        Allowing unrestricted (0.0.0.0/0 or ::/0) outbound/egress access can increase opportunities for
+        malicious activity such as such as Denial of Service (DoS) attacks or Distributed Denial of Service (DDoS)
+        attacks.
+
+    Fix:
+        Explicitly defining the egress policy for the security group.
+
+    Code for fix:
+        Even in the example below, the egress rule added will allow HTTP traffic out to the world. However,
+        this will pass the rule.
+
+        ````json
+        // example from https://stelligent.com/2016/04/07/finding-security-problems-early-in-the-development-process-of-a-cloudformation-template-with-cfn-nag/
+        {
+            "Resources": {
+                "sg": {
+                    "Type": "AWS::EC2::SecurityGroup",
+                    "Properties": {
+                        "GroupDescription": "some_group_desc",
+                        "SecurityGroupIngress": {
+                            "CidrIp": "10.1.2.3/32",
+                            "FromPort": 34,
+                            "ToPort": 34,
+                            "IpProtocol": "tcp"
+                        },
+                        // addition of egress to the `sg` resource
+                        "SecurityGroupEgress": {
+                            "CidrIp": "0.0.0.0/0",
+                            "FromPort": 80,
+                            "ToPort": 80,
+                            "IpProtocol": "tcp"
+                        },
+                        "VpcId": "vpc-12345678"
+                    }
+                }
+            }
+        }
+        ````
     """
 
     GRANULARITY = RuleGranularity.RESOURCE
     REASON = (
         "Missing egress rule in {} means all traffic is allowed outbound. Make this explicit if it is desired "
         "configuration"
     )
-    RULE_MODE = RuleMode.MONITOR
+    RULE_MODE = RuleMode.DEBUG
 
     def invoke(self, cfmodel):
         for logical_id, resource in cfmodel.Resources.items():