Add support for policy documents that are of type string in WildcardR…

…esourceRule (#197)

* Add support for policy documents that are of type string in WildcardResourceRule

* Fix typo

* Apply PR comment on stack_info for logger

Co-authored-by: Oliver Crawford <oliver.crawford@skyscanner.net>

.gitignore

@@ -71,3 +71,6 @@ venv/
 ENV/
 env.bak/
 venv.bak/
+
+# Snyk Code
+.dccache

CHANGELOG.md

@@ -1,6 +1,10 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [1.2.1] - 2021-12-24
+### Fixes
+- The `WildcardResourceRule` would fail if it received a policy document that was a string. It was expecting all policy documents to be a dictionary. Some AWS services allow for string policies though (e.g. `AWS::Logs::ResourcePolicy`). The rule has been updated to handle string policies by attempting to convert it to a dictionary.
+
 ## [1.2.0] - 2021-11-03
 ### Updates
 - The rules `EC2SecurityGroupOpenToWorldRule` and `EC2SecurityGroupIngressOpenToWorldRule` were by default allowing ports 80 and 443. This has now been migrated to use a filter object, that can be optionally applied. See the README for further details. This means if the filter is not applied, Security Groups open to the world on ports 80 and 443 will start failing in CFRipper.

cfripper/__version__.py

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

cfripper/rules/wildcard_resource_rule.py

@@ -1,6 +1,7 @@
 __all__ = [
     "WildcardResourceRule",
 ]
+import json
 import logging
 from typing import Dict, Optional
 
@@ -60,7 +61,19 @@ def resource_invoke(self, resource: Resource, logical_id: str, extras: Optional[
             if hasattr(resource, "Properties"):
                 policy_document = resource.Properties.get("PolicyDocument")
                 if policy_document:
-                    self._check_policy_document(result, logical_id, PolicyDocument(**policy_document), None, extras)
+                    try:
+                        # PolicyDocument requires a dict. If we receive a string, attempt a conversion to dict.
+                        # If this conversion fails, show the appropriate warning and continue.
+                        formatted_policy_document = (
+                            json.loads(policy_document) if isinstance(policy_document, str) else policy_document
+                        )
+                        self._check_policy_document(
+                            result, logical_id, PolicyDocument(**formatted_policy_document), None, extras
+                        )
+                    except Exception:
+                        logger.warning(
+                            f"Could not process the PolicyDocument {policy_document} on {logical_id}", stack_info=True
+                        )
 
         return result
 

tests/rules/test_WildcardResourceRule.py

@@ -1,3 +1,5 @@
+from unittest.mock import patch
+
 import pydantic
 import pytest
 from pycfmodel.model.resources.iam_policy import IAMPolicy
@@ -43,6 +45,16 @@ def user_and_policy_with_wildcard_resource():
     return get_cfmodel_from("rules/WildcardResourceRule/multiple_resources_with_wildcard_resources.json").resolve()
 
 
+@pytest.fixture()
+def policy_with_string_policy_document():
+    return get_cfmodel_from("rules/WildcardResourceRule/policy_with_string_policy_document.json").resolve()
+
+
+@pytest.fixture()
+def policy_with_invalid_string_policy_document():
+    return get_cfmodel_from("rules/WildcardResourceRule/policy_with_invalid_string_policy_document.json").resolve()
+
+
 def test_user_with_inline_policy_with_wildcard_resource_is_detected(user_with_wildcard_resource):
     rule = WildcardResourceRule(None)
     rule._config.stack_name = "not_allowed_stack"
@@ -683,6 +695,37 @@ def test_policy_s3_wildcard_and_all_buckets(policy_with_s3_wildcard_and_all_buck
     assert 100 < len(result.failures)
 
 
+def test_policy_with_string_policy_document(policy_with_string_policy_document):
+    rule = WildcardResourceRule(None)
+    rule.all_cf_actions = set()
+    result = rule.invoke(policy_with_string_policy_document)
+
+    assert result.valid is False
+    assert result.failures == [
+        Failure(
+            granularity="ACTION",
+            reason='"GuardDutyResourcePolicy" is using a wildcard resource for "logs:CreateLogStream"',
+            risk_value="MEDIUM",
+            rule="WildcardResourceRule",
+            rule_mode="BLOCKING",
+            actions={"logs:CreateLogStream"},
+            resource_ids={"GuardDutyResourcePolicy"},
+        )
+    ]
+
+
+@patch("logging.Logger.warning")
+def test_policy_with_invalid_string_policy_document(patched_logger, policy_with_invalid_string_policy_document):
+    rule = WildcardResourceRule(None)
+    rule.all_cf_actions = set()
+    result = rule.invoke(policy_with_invalid_string_policy_document)
+
+    assert result.valid is True
+    patched_logger.assert_called_with(
+        "Could not process the PolicyDocument FOOBARFOOBAR on GuardDutyResourcePolicy", stack_info=True
+    )
+
+
 def test_policy_document_with_wildcard_resource_without_policy_name_is_detected():
     with pytest.raises(pydantic.ValidationError):
         get_cfmodel_from("rules/WildcardResourceRule/iam_policy_with_wildcard_resource_without_policy_name.json")

tests/test_templates/rules/WildcardResourceRule/policy_with_invalid_string_policy_document.json

@@ -0,0 +1,11 @@
+{
+  "Resources": {
+    "GuardDutyResourcePolicy": {
+      "Type": "AWS::Something::Broken",
+      "Properties": {
+        "PolicyName": "something-broken",
+        "PolicyDocument": "FOOBARFOOBAR"
+      }
+    }
+  }
+}

tests/test_templates/rules/WildcardResourceRule/policy_with_string_policy_document.json

@@ -0,0 +1,11 @@
+{
+  "Resources": {
+    "GuardDutyResourcePolicy": {
+      "Type": "AWS::Logs::ResourcePolicy",
+      "Properties": {
+        "PolicyName": "guardduty-resourcepolicy",
+        "PolicyDocument": "{\"Version\":\"2008-10-17\",\"Statement\":[{\"Sid\":\"GDAllowLogs\",\"Effect\":\"Allow\",\"Principal\":{\"Service\":[\"events.amazonaws.com\",\"delivery.logs.amazonaws.com\"]},\"Action\":[\"logs:CreateLogStream\"],\"Resource\":\"*\"}]}"
+      }
+    }
+  }
+}