Repository: duo-labs/parliament

Issue:
Identify any star in a resource
In #72, I added a finding for whenever a policy has a Resource of `*` when the policy supports a more fine-grained resource.  This could be bypassed by specifying a resource such as `arn:aws:s3:::*` so I should alert on any use of `*`, although I may want to make the ARN parsing a bit smarter, because `arn:aws:s3:::mybucket/folder/*` seems reasonable for a policy, so warning on any `*` would be false positive prone.

Discussion:
raghavkaul: Isn't there a PR validator that runs tests?

Patch:
diff --git a/parliament/config.yaml b/parliament/config.yaml
index e56906a..e70c87b 100644
--- a/parliament/config.yaml
+++ b/parliament/config.yaml
@@ -115,6 +115,12 @@ RESOURCE_STAR:
   severity: LOW
   group: MALFORMED
 
+RESOURCE_EFFECTIVELY_STAR:
+  title: Unnecessary use of Resource * based on wildcards
+  description: "Resource block is functionally equivalent to Resource *"
+  severity: LOW
+  group: MALFORMED
+
 UNKNOWN_OPERATOR:
   title: The condition operator is unknown.
   severity: MEDIUM
diff --git a/parliament/statement.py b/parliament/statement.py
index eb05d5a..3f95e62 100644
--- a/parliament/statement.py
+++ b/parliament/statement.py
@@ -879,6 +879,9 @@ class Statement:
             actions_without_matching_resources = []
             # Ensure the required resources for each action exist
             # Iterate through each action
+
+            all_possible_resources_for_stmt = []
+
             for action_struct in expanded_actions:
                 privilege_info = get_privilege_info(
                     action_struct["service"], action_struct["action"]
@@ -889,6 +892,7 @@ class Statement:
                     len(privilege_info["resource_types"]) == 1
                     and privilege_info["resource_types"][0]["resource_type"] == ""
                 ):
+                    all_possible_resources_for_stmt.append("*")
                     match_found = False
                     for resource in resources:
                         if resource.value == "*":
@@ -915,6 +919,8 @@ class Statement:
                         resource_type, privilege_info["service_resources"]
                     )
 
+                    all_possible_resources_for_stmt.append(arn_format)
+
                     # At least one resource has to match the action's required resources
                     match_found = False
                     for resource in resources:
@@ -950,6 +956,23 @@ class Statement:
                     location=self.stmt
                 )
 
+            # If the Statement is applied to specific wildcarded Resources,
+            # but those Resources span all possible Resources that the Actions
+            # can touch, then the Statement is effectively unbounded, even without
+            # an explicit "*"
+            specified_resources = {r.value for r in resources}
+
+            # Only handle the cases where "*" is not needed (some Actions can
+            # only be scoped to "*") and it's not explcitly provided (already
+            # handled by RESOURCE_STAR). Otherwise, this check accidentally
+            # flags Actions that have - and need - Resource "*".
+            if (
+                "*" not in all_possible_resources_for_stmt
+                and "*" not in specified_resources
+                and specified_resources == set(all_possible_resources_for_stmt)
+            ):
+                self.add_finding("RESOURCE_EFFECTIVELY_STAR", location=self.stmt)
+
         # If conditions exist, it will be an element, which was previously made into a list
         if len(conditions) == 1:
             # Iterate through each condition, of something like:


Test patch:
diff --git a/tests/unit/test_patterns.py b/tests/unit/test_patterns.py
index 21c6f07..c3ec071 100644
--- a/tests/unit/test_patterns.py
+++ b/tests/unit/test_patterns.py
@@ -82,6 +82,57 @@ class TestPatterns(unittest.TestCase):
             "Resource policy privilege escalation across two statement",
         )
 
+    def test_resource_effectively_star(self):
+        policy = analyze_policy_string(
+            """{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Sid": "CloudtrailReadTrail",
+      "Effect": "Allow",
+      "Action": [
+        "cloudtrail:GetEventSelectors",
+        "cloudtrail:PutEventSelectors"
+      ],
+      "Resource": [
+        "arn:*:cloudtrail:*:*:trail/*"
+      ]
+    }
+  ]
+}"""
+        )
+
+        assert_equal(
+            policy.finding_ids,
+            set(["RESOURCE_EFFECTIVELY_STAR"]),
+            "Resource policy spans all Cloudtrails even without an asterisk.",
+        )
+
+        policy = analyze_policy_string(
+            """{
+      "Version": "2012-10-17",
+      "Statement": [
+        {
+          "Sid": "CloudtrailReadTrail",
+          "Effect": "Allow",
+          "Action": [
+            "cloudtrail:GetEventSelectors",
+            "cloudtrail:PutEventSelectors"
+          ],
+          "Resource": [
+            "arn:aws:cloudtrail:us-east-1:000000000000:trail/*"
+          ]
+        }
+      ]
+    }"""
+        )
+
+        assert_equal(
+            policy.finding_ids,
+            set(),
+            "Resource policy is scoped by AWS partition, account and region and is therefore not 'STAR'.",
+        )
+
     def test_resource_policy_privilege_escalation_with_deny(self):
         # This test ensures if we have an allow on a specific resource, but a Deny on *,
         # that it is denied.