Update GenericResourceWildcardPolicyRule to use REGEX_CONTAINS_WILDCA…

…RD (#227) * create new regex to detect any wildcard * update GenericResourceWildcardPolicyRule to use REGEX_CONTAINS_WILDCARD * update version and changelog * fix moto version since 3.1.10 is giving issues * update reason of fixed dependency * update changelog Co-authored-by: Ramon <ramon.guimera@skyscanner.net>
Skyscanner · May 30, 2022 · 2154f25 · 2154f25
1 parent 1385c7a
commit 2154f25
Show file tree

Hide file tree

Showing 8 changed files with 84 additions and 15 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,13 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [1.11.0]
+### Additions
+- New regex `REGEX_CONTAINS_WILDCARD` to check for any wildcard
+### Updates
+- `GenericResourceWildcardPolicyRule` now uses `REGEX_CONTAINS_WILDCARD` instead of `REGEX_HAS_STAR_OR_STAR_AFTER_COLON`.
+- Bump dev dependency `moto` to `==3.1.9`.
+
 ## [1.10.0]
 ### Improvements
 - `GenericCrossAccountTrustRule` can now scan IAM Roles correctly as `CrossAccountTrustRule` does

diff --git a/cfripper/__version__.py b/cfripper/__version__.py
@@ -1,3 +1,3 @@
-VERSION = (1, 10, 0)
+VERSION = (1, 11, 0)
 
 __version__ = ".".join(map(str, VERSION))
diff --git a/cfripper/config/regex.py b/cfripper/config/regex.py
@@ -69,6 +69,23 @@
 """
 REGEX_CONTAINS_STAR = re.compile(r"^.*[*].*$")
 
+"""
+Check for an str where a wildcard (* or ?) is being used.
+Valid:
+- *
+- ?
+- abc*def
+- abc?def
+- abcdef*
+- abcdef?
+- *abcdef
+- ?abcdef
+Invalid:
+- arn:aws:iam::123456789012:not-root
+- potato
+"""
+REGEX_CONTAINS_WILDCARD = re.compile(r"^.*[*?].*$")
+
 
 """
 Check for root wildcard

diff --git a/cfripper/rules/wildcard_policies.py b/cfripper/rules/wildcard_policies.py
@@ -14,7 +14,7 @@
 from pycfmodel.model.resources.sns_topic_policy import SNSTopicPolicy
 from pycfmodel.model.resources.sqs_queue_policy import SQSQueuePolicy
 
-from cfripper.config.regex import REGEX_HAS_STAR_OR_STAR_AFTER_COLON
+from cfripper.config.regex import REGEX_CONTAINS_WILDCARD, REGEX_HAS_STAR_OR_STAR_AFTER_COLON
 from cfripper.model.enums import RuleGranularity
 from cfripper.model.result import Result
 from cfripper.rules.base_rules import Rule
@@ -61,10 +61,10 @@ def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
 
 class GenericResourceWildcardPolicyRule(GenericWildcardPolicyRule):
     """
-    Rule that checks for use of the wildcard `*` character in Actions of Policy Documents of Generic AWS Resources.
+    Rule that checks for use of a wildcard `*` or `?` character in Actions of Policy Documents of Generic AWS Resources.
     """
 
-    REASON = "{} should not allow a `*` action"
+    REASON = "{} should not allow a wildcard (`*` or `?`) action"
     GRANULARITY = RuleGranularity.RESOURCE
 
     def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
@@ -73,7 +73,7 @@ def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
             policy_documents = resource.policy_documents
             if policy_documents:
                 for document in policy_documents:
-                    if document.policy_document.allowed_actions_with(REGEX_HAS_STAR_OR_STAR_AFTER_COLON):
+                    if document.policy_document.allowed_actions_with(REGEX_CONTAINS_WILDCARD):
                         self.add_failure_to_result(
                             result,
                             self.REASON.format(logical_id),

diff --git a/setup.py b/setup.py
@@ -23,7 +23,7 @@
     "pytest>=3.6",
     "pytest-cov>=2.5.1",
     "pip-tools>=5.3.1",
-    "moto[cloudformation,s3]>=3.0.0",
+    "moto[cloudformation,s3]==3.1.9",  # coverage fails for 3.1.10, issue is https://github.com/spulec/moto/issues/5162
 ]
 
 docs_requires = [

diff --git a/tests/config/test_regex.py b/tests/config/test_regex.py
@@ -3,6 +3,7 @@
 from cfripper.config.regex import (
     REGEX_ARN,
     REGEX_CONTAINS_STAR,
+    REGEX_CONTAINS_WILDCARD,
     REGEX_CROSS_ACCOUNT_ROOT,
     REGEX_FULL_WILDCARD_PRINCIPAL,
     REGEX_HAS_STAR_OR_STAR_AFTER_COLON,
@@ -44,6 +45,16 @@
         (REGEX_CONTAINS_STAR, "*abcdef", True),
         (REGEX_CONTAINS_STAR, "arn:aws:iam::123456789012:not-root", False),
         (REGEX_CONTAINS_STAR, "potato", False),
+        (REGEX_CONTAINS_WILDCARD, "*", True),
+        (REGEX_CONTAINS_WILDCARD, "abc*def", True),
+        (REGEX_CONTAINS_WILDCARD, "abcdef*", True),
+        (REGEX_CONTAINS_WILDCARD, "*abcdef", True),
+        (REGEX_CONTAINS_WILDCARD, "?", True),
+        (REGEX_CONTAINS_WILDCARD, "abc?def", True),
+        (REGEX_CONTAINS_WILDCARD, "abcdef?", True),
+        (REGEX_CONTAINS_WILDCARD, "?abcdef", True),
+        (REGEX_CONTAINS_WILDCARD, "arn:aws:iam::123456789012:not-root", False),
+        (REGEX_CONTAINS_WILDCARD, "potato", False),
         (REGEX_IS_STAR, "*", True),
         (REGEX_IS_STAR, "abc*def", False),
         (REGEX_IS_STAR, "abcdef*", False),

diff --git a/tests/rules/test_GenericResourceWildcardPolicyRule.py b/tests/rules/test_GenericResourceWildcardPolicyRule.py
@@ -36,7 +36,7 @@ def sns_topic_with_wildcards_fixture():
             [
                 Failure(
                     granularity=RuleGranularity.RESOURCE,
-                    reason="mysnspolicy1 should not allow a `*` action",
+                    reason="mysnspolicy1 should not allow a wildcard (`*` or `?`) action",
                     risk_value=RuleRisk.MEDIUM,
                     rule="GenericResourceWildcardPolicyRule",
                     rule_mode=RuleMode.BLOCKING,
@@ -52,7 +52,7 @@ def sns_topic_with_wildcards_fixture():
             [
                 Failure(
                     granularity=RuleGranularity.RESOURCE,
-                    reason="mysqspolicy1 should not allow a `*` action",
+                    reason="mysqspolicy1 should not allow a wildcard (`*` or `?`) action",
                     risk_value=RuleRisk.MEDIUM,
                     rule="GenericResourceWildcardPolicyRule",
                     rule_mode=RuleMode.BLOCKING,
@@ -62,7 +62,7 @@ def sns_topic_with_wildcards_fixture():
                 ),
                 Failure(
                     granularity=RuleGranularity.RESOURCE,
-                    reason="mysqspolicy1b should not allow a `*` action",
+                    reason="mysqspolicy1b should not allow a wildcard (`*` or `?`) action",
                     risk_value=RuleRisk.MEDIUM,
                     rule="GenericResourceWildcardPolicyRule",
                     rule_mode=RuleMode.BLOCKING,
@@ -72,7 +72,7 @@ def sns_topic_with_wildcards_fixture():
                 ),
                 Failure(
                     granularity=RuleGranularity.RESOURCE,
-                    reason="mysqspolicy1c should not allow a `*` action",
+                    reason="mysqspolicy1c should not allow a wildcard (`*` or `?`) action",
                     risk_value=RuleRisk.MEDIUM,
                     rule="GenericResourceWildcardPolicyRule",
                     rule_mode=RuleMode.BLOCKING,
@@ -82,7 +82,7 @@ def sns_topic_with_wildcards_fixture():
                 ),
                 Failure(
                     granularity=RuleGranularity.RESOURCE,
-                    reason="mysqspolicy1d should not allow a `*` action",
+                    reason="mysqspolicy1d should not allow a wildcard (`*` or `?`) action",
                     risk_value=RuleRisk.MEDIUM,
                     rule="GenericResourceWildcardPolicyRule",
                     rule_mode=RuleMode.BLOCKING,
@@ -98,7 +98,7 @@ def sns_topic_with_wildcards_fixture():
             [
                 Failure(
                     granularity=RuleGranularity.RESOURCE,
-                    reason="S3BucketPolicy should not allow a `*` action",
+                    reason="S3BucketPolicy should not allow a wildcard (`*` or `?`) action",
                     risk_value=RuleRisk.MEDIUM,
                     rule="GenericResourceWildcardPolicyRule",
                     rule_mode=RuleMode.BLOCKING,
@@ -108,7 +108,7 @@ def sns_topic_with_wildcards_fixture():
                 ),
                 Failure(
                     granularity=RuleGranularity.RESOURCE,
-                    reason="S3BucketPolicy2 should not allow a `*` action",
+                    reason="S3BucketPolicy2 should not allow a wildcard (`*` or `?`) action",
                     risk_value=RuleRisk.MEDIUM,
                     rule="GenericResourceWildcardPolicyRule",
                     rule_mode=RuleMode.BLOCKING,
@@ -124,7 +124,7 @@ def sns_topic_with_wildcards_fixture():
             [
                 Failure(
                     granularity=RuleGranularity.RESOURCE,
-                    reason="NotMapped4 should not allow a `*` action",
+                    reason="NotMapped4 should not allow a wildcard (`*` or `?`) action",
                     risk_value=RuleRisk.MEDIUM,
                     rule="GenericResourceWildcardPolicyRule",
                     rule_mode=RuleMode.BLOCKING,
@@ -134,14 +134,24 @@ def sns_topic_with_wildcards_fixture():
                 ),
                 Failure(
                     granularity=RuleGranularity.RESOURCE,
-                    reason="NotMapped5 should not allow a `*` action",
+                    reason="NotMapped5 should not allow a wildcard (`*` or `?`) action",
                     risk_value=RuleRisk.MEDIUM,
                     rule="GenericResourceWildcardPolicyRule",
                     rule_mode=RuleMode.BLOCKING,
                     actions=None,
                     resource_ids={"NotMapped5"},
                     resource_types={"AWS::Not::Mapped"},
                 ),
+                Failure(
+                    granularity=RuleGranularity.RESOURCE,
+                    reason="NotMapped7 should not allow a wildcard (`*` or `?`) action",
+                    risk_value=RuleRisk.MEDIUM,
+                    rule="GenericResourceWildcardPolicyRule",
+                    rule_mode=RuleMode.BLOCKING,
+                    actions=None,
+                    resource_ids={"NotMapped7"},
+                    resource_types={"AWS::Not::Mapped"},
+                ),
             ],
         ),
     ],

diff --git a/tests/test_templates/rules/WildcardPoliciesRule/generic_with_wildcards.json b/tests/test_templates/rules/WildcardPoliciesRule/generic_with_wildcards.json
@@ -88,6 +88,30 @@
           ]
         }
       }
+    },
+    "NotMapped7": {
+      "Type": "AWS::Not::Mapped",
+      "Properties": {
+        "Bucket": {
+          "Ref": "S3Bucket"
+        },
+        "PolicyDocument": {
+          "Statement": [
+            {
+              "Action": [
+                "?"
+              ],
+              "Effect": "Allow",
+              "Resource": "arn:aws:s3:::fakebucketfakebucket/*",
+              "Principal": {
+                "AWS": [
+                  "156460612806"
+                ]
+              }
+            }
+          ]
+        }
+      }
     }
   }
 }