Add S3BucketPublicReadAclRule rule (#185)

* Add S3BucketPublicReadAclRule rule

* PR suggestions

Co-authored-by: Carles Lopez <carles.lopez@skyscanner.net>

CHANGELOG.md

@@ -1,6 +1,10 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [1.0.6] - 2021-07-28
+### Improvements
+- Add `S3BucketPublicReadAclRule` rule
+
 ## [1.0.5] - 2021-07-28
 ### Improvements
 - Add EKS permissions that accept wildcard resource only

cfripper/__version__.py

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

cfripper/rules/__init__.py

@@ -20,7 +20,11 @@
 from cfripper.rules.policy_on_user import PolicyOnUserRule
 from cfripper.rules.privilege_escalation import PrivilegeEscalationRule
 from cfripper.rules.s3_bucket_policy import S3BucketPolicyPrincipalRule
-from cfripper.rules.s3_public_access import S3BucketPublicReadAclAndListStatementRule, S3BucketPublicReadWriteAclRule
+from cfripper.rules.s3_public_access import (
+    S3BucketPublicReadAclAndListStatementRule,
+    S3BucketPublicReadAclRule,
+    S3BucketPublicReadWriteAclRule,
+)
 from cfripper.rules.sns_topic_policy import SNSTopicDangerousPolicyActionsRule, SNSTopicPolicyNotPrincipalRule
 from cfripper.rules.sqs_queue_policy import (
     SQSDangerousPolicyActionsRule,
@@ -58,6 +62,7 @@
         S3BucketPolicyWildcardActionRule,
         S3BucketPublicReadAclAndListStatementRule,
         S3BucketPublicReadWriteAclRule,
+        S3BucketPublicReadAclRule,
         S3CrossAccountTrustRule,
         SNSTopicDangerousPolicyActionsRule,
         SNSTopicPolicyNotPrincipalRule,

cfripper/rules/s3_public_access.py

@@ -1,4 +1,4 @@
-__all__ = ["S3BucketPublicReadAclAndListStatementRule", "S3BucketPublicReadWriteAclRule"]
+__all__ = ["S3BucketPublicReadAclAndListStatementRule", "S3BucketPublicReadWriteAclRule", "S3BucketPublicReadAclRule"]
 
 import logging
 import re
@@ -105,3 +105,40 @@ def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
                     context={"config": self._config, "extras": extras, "logical_id": logical_id, "resource": resource},
                 )
         return result
+
+
+class S3BucketPublicReadAclRule(Rule):
+    """
+    Checks if any S3 bucket policy has access control set to `PublicRead`.
+
+    Risk:
+        Unless the bucket is hosting static content, S3 buckets should not have Public Read available on a bucket.
+        This allows anyone to read any objects to your S3 bucket.
+
+    Fix:
+        Remove any configuration that looks like `"AccessControl": "PublicRead"` from your S3 bucket policy.
+
+    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 |
+        |`resource`     | `Resource`         | S3 Bucket that is being addressed                              |
+    """
+
+    GRANULARITY = RuleGranularity.RESOURCE
+    REASON = "S3 Bucket {} should not have a public-read acl"
+    RISK_VALUE = RuleRisk.HIGH
+
+    def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
+        result = Result()
+        for logical_id, resource in cfmodel.resources_filtered_by_type(("AWS::S3::Bucket",)).items():
+            if hasattr(resource, "Properties") and resource.Properties.get("AccessControl") == "PublicRead":
+                self.add_failure_to_result(
+                    result,
+                    self.REASON.format(logical_id),
+                    resource_ids={logical_id},
+                    context={"config": self._config, "extras": extras, "logical_id": logical_id, "resource": resource},
+                )
+        return result

tests/rules/test_S3BucketPublicReadAclRule.py

@@ -0,0 +1,41 @@
+from pytest import fixture
+
+from cfripper.config.config import Config
+from cfripper.model.enums import RuleGranularity, RuleMode, RuleRisk
+from cfripper.model.result import Failure
+from cfripper.rules import S3BucketPublicReadAclRule
+from tests.utils import compare_lists_of_failures, get_cfmodel_from
+
+
+@fixture()
+def bad_template():
+    return get_cfmodel_from("rules/S3BucketPublicReadAclRule/bad_template.json").resolve()
+
+
+def test_failures_are_raised(bad_template):
+    rule = S3BucketPublicReadAclRule(Config())
+    result = rule.invoke(bad_template)
+
+    assert not result.valid
+    assert compare_lists_of_failures(
+        result.failures,
+        [
+            Failure(
+                granularity=RuleGranularity.RESOURCE,
+                reason="S3 Bucket S3Bucket should not have a public-read acl",
+                risk_value=RuleRisk.HIGH,
+                rule="S3BucketPublicReadAclRule",
+                rule_mode=RuleMode.BLOCKING,
+                actions=None,
+                resource_ids={"S3Bucket"},
+            )
+        ],
+    )
+
+
+def test_rule_supports_filter_config(bad_template, default_allow_all_config):
+    rule = S3BucketPublicReadAclRule(default_allow_all_config)
+    result = rule.invoke(bad_template)
+
+    assert result.valid
+    assert compare_lists_of_failures(result.failures, [])

tests/test_templates/rules/S3BucketPublicReadAclRule/bad_template.json

@@ -0,0 +1,11 @@
+{
+  "Resources": {
+    "S3Bucket": {
+      "Type": "AWS::S3::Bucket",
+      "Properties": {
+        "BucketName": "fakebucketfakebucket",
+        "AccessControl": "PublicRead"
+      }
+    }
+  }
+}