Remove default exemption for ports 80 and 443 on Open Security Group …

…rules (#196) * Remove default exemption for ports 80 and 443 on Open Security Group rules * Update filter to perform equality check on the powerset of allowed ports Co-authored-by: Oliver Crawford <oliver.crawford@skyscanner.net>
Skyscanner · Nov 3, 2021 · 869eb58 · 869eb58
1 parent 1bc3ff4
commit 869eb58
Show file tree

Hide file tree

Showing 8 changed files with 129 additions and 10 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,10 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [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.
+
 ## [1.1.2] - 2021-10-06
 ### Fixes
 - Add a fix to the `KMSKeyEnabledKeyRotation` rule to be able to detect the `EnableKeyRotation` property properly.

diff --git a/cfripper/__version__.py b/cfripper/__version__.py
@@ -1,3 +1,3 @@
-VERSION = (1, 1, 2)
+VERSION = (1, 2, 0)
 
 __version__ = ".".join(map(str, VERSION))
diff --git a/cfripper/config/config.py b/cfripper/config/config.py
@@ -23,7 +23,6 @@
 
 
 class Config:
-    DEFAULT_ALLOWED_WORLD_OPEN_PORTS = [80, 443]
     DEFAULT_FORBIDDEN_MANAGED_POLICY_ARNS = [
         "arn:aws:iam::aws:policy/AdministratorAccess",
         "arn:aws:iam::aws:policy/IAMFullAccess",
@@ -120,8 +119,6 @@ def __init__(
         else:
             self.aws_service_accounts = aws_service_accounts
 
-        self.allowed_world_open_ports = list(self.DEFAULT_ALLOWED_WORLD_OPEN_PORTS)
-
         self.forbidden_managed_policy_arns = list(self.DEFAULT_FORBIDDEN_MANAGED_POLICY_ARNS)
 
         self.forbidden_resource_star_action_prefixes = list(self.DEFAULT_FORBIDDEN_RESOURCE_STAR_ACTION_PREFIXES)

diff --git a/cfripper/config/rule_configs/allow_http_ports_open_to_world.py b/cfripper/config/rule_configs/allow_http_ports_open_to_world.py
@@ -0,0 +1,46 @@
+from itertools import chain, combinations
+
+from cfripper.config.filter import Filter
+from cfripper.model.enums import RuleMode
+
+"""
+To use this Filter, or any Filter, make sure to include it in the `Config` instantiation.
+
+```python
+FILTERS = [allow_http_ports_open_to_world_rules_config_filter]
+
+config = Config(
+    ...
+    rules_filters=FILTERS,
+)
+```
+"""
+
+
+ALLOWED_PORTS = [80, 443]
+
+
+def powerset(iterable):
+    # https://docs.python.org/3/library/itertools.html#itertools-recipes
+    # powerset([1,2,3]) --> () (1,) (2,) (3,) (1,2) (1,3) (2,3) (1,2,3)
+    s = list(iterable)
+    return chain.from_iterable(combinations(s, r) for r in range(len(s) + 1))
+
+
+allow_http_ports_open_to_world_rules_config_filter = Filter(
+    reason="It can be acceptable to have Security Groups publicly available on ports 80 or 443.",
+    rule_mode=RuleMode.ALLOWED,
+    eval={
+        "and": [
+            {"exists": {"ref": "open_ports"}},
+            {
+                "or": [
+                    {"eq": [list(subset_allowed_ports), {"ref": "open_ports"}]}
+                    for subset_allowed_ports in powerset(ALLOWED_PORTS)
+                    if subset_allowed_ports
+                ]
+            },
+        ]
+    },
+    rules={"EC2SecurityGroupOpenToWorldRule", "EC2SecurityGroupIngressOpenToWorldRule"},
+)
diff --git a/cfripper/config/rule_configs/example_rules_config_for_cli.py b/cfripper/config/rule_configs/example_rules_config_for_cli.py
@@ -1,9 +1,12 @@
 from cfripper.config.rule_config import RuleConfig
+from cfripper.config.rule_configs.allow_http_ports_open_to_world import (
+    allow_http_ports_open_to_world_rules_config_filter,
+)
 from cfripper.config.rule_configs.firehose_ips import firehose_ips_rules_config_filter
 from cfripper.model.enums import RuleMode
 
 RULES_CONFIG = {
     "EC2SecurityGroupMissingEgressRule": RuleConfig(rule_mode=RuleMode.DISABLED),
 }
 
-FILTERS = [firehose_ips_rules_config_filter]
+FILTERS = [allow_http_ports_open_to_world_rules_config_filter, firehose_ips_rules_config_filter]
diff --git a/cfripper/rules/ec2_security_group.py b/cfripper/rules/ec2_security_group.py
@@ -37,7 +37,7 @@ def analyse_ingress(
     ):
         if self.non_compliant_ip_range(ingress=ingress):
             open_ports = list(range(ingress.FromPort, ingress.ToPort + 1))
-            non_allowed_open_ports = sorted(set(open_ports) - set(self._config.allowed_world_open_ports))
+            non_allowed_open_ports = sorted(set(open_ports))
 
             if non_allowed_open_ports:
                 ip_range = ingress.CidrIp or ingress.CidrIpv6

diff --git a/tests/rules/test_EC2SecurityGroupOpenToWorldRule.py b/tests/rules/test_EC2SecurityGroupOpenToWorldRule.py
@@ -2,6 +2,9 @@
 
 from cfripper.config.config import Config
 from cfripper.config.filter import Filter
+from cfripper.config.rule_configs.allow_http_ports_open_to_world import (
+    allow_http_ports_open_to_world_rules_config_filter,
+)
 from cfripper.model.enums import RuleGranularity, RuleMode, RuleRisk
 from cfripper.model.result import Failure
 from cfripper.rule_processor import RuleProcessor
@@ -46,6 +49,11 @@ def invalid_security_group_multiple_statements():
     ).resolve()
 
 
+@pytest.fixture()
+def invalid_security_group_port78_81():
+    return get_cfmodel_from("rules/EC2SecurityGroupOpenToWorldRule/invalid_security_group_port78_81.json").resolve()
+
+
 def test_security_group_type_slash0(security_group_type_slash0):
     rule = EC2SecurityGroupOpenToWorldRule(None)
     result = rule.invoke(security_group_type_slash0)
@@ -76,21 +84,63 @@ def test_valid_security_group_not_slash0(valid_security_group_not_slash0):
 
 
 def test_valid_security_group_port80(valid_security_group_port80):
-    rule = EC2SecurityGroupOpenToWorldRule(None)
+    rule = EC2SecurityGroupOpenToWorldRule(
+        Config(
+            rules=["EC2SecurityGroupOpenToWorldRule"],
+            aws_account_id="123456789",
+            stack_name="mockstack",
+            rules_filters=[allow_http_ports_open_to_world_rules_config_filter],
+        )
+    )
     result = rule.invoke(valid_security_group_port80)
 
     assert result.valid
     assert compare_lists_of_failures(result.failures, [])
 
 
 def test_valid_security_group_port443(valid_security_group_port443):
-    rule = EC2SecurityGroupOpenToWorldRule(None)
+    rule = EC2SecurityGroupOpenToWorldRule(
+        Config(
+            rules=["EC2SecurityGroupOpenToWorldRule"],
+            aws_account_id="123456789",
+            stack_name="mockstack",
+            rules_filters=[allow_http_ports_open_to_world_rules_config_filter],
+        )
+    )
     result = rule.invoke(valid_security_group_port443)
 
     assert result.valid
     assert compare_lists_of_failures(result.failures, [])
 
 
+def test_invalid_security_group_port78_81(invalid_security_group_port78_81):
+    rule = EC2SecurityGroupOpenToWorldRule(
+        Config(
+            rules=["EC2SecurityGroupOpenToWorldRule"],
+            aws_account_id="123456789",
+            stack_name="mockstack",
+            rules_filters=[allow_http_ports_open_to_world_rules_config_filter],
+        )
+    )
+    result = rule.invoke(invalid_security_group_port78_81)
+
+    assert not result.valid
+    assert compare_lists_of_failures(
+        result.failures,
+        [
+            Failure(
+                granularity=RuleGranularity.RESOURCE,
+                reason="Port(s) 78-81 open to public IPs: (0.0.0.0/0) in security group 'SecurityGroup'",
+                risk_value=RuleRisk.MEDIUM,
+                rule="EC2SecurityGroupOpenToWorldRule",
+                rule_mode=RuleMode.BLOCKING,
+                actions=None,
+                resource_ids={"SecurityGroup"},
+            )
+        ],
+    )
+
+
 def test_invalid_security_group_cidripv6(invalid_security_group_cidripv6):
     rule = EC2SecurityGroupOpenToWorldRule(None)
     result = rule.invoke(invalid_security_group_cidripv6)
@@ -122,7 +172,7 @@ def test_invalid_security_group_range(invalid_security_group_range):
         [
             Failure(
                 granularity=RuleGranularity.RESOURCE,
-                reason="Port(s) 0-79, 81-100 open to public IPs: (11.0.0.0/8) in security group 'SecurityGroup'",
+                reason="Port(s) 0-100 open to public IPs: (11.0.0.0/8) in security group 'SecurityGroup'",
                 risk_value=RuleRisk.MEDIUM,
                 rule="EC2SecurityGroupOpenToWorldRule",
                 rule_mode=RuleMode.BLOCKING,
@@ -203,7 +253,7 @@ def test_non_matching_filters_are_reported_normally(invalid_security_group_range
         [
             Failure(
                 granularity=RuleGranularity.RESOURCE,
-                reason="Port(s) 0-79, 81-100 open to public IPs: (11.0.0.0/8) in security group 'SecurityGroup'",
+                reason="Port(s) 0-100 open to public IPs: (11.0.0.0/8) in security group 'SecurityGroup'",
                 risk_value=RuleRisk.MEDIUM,
                 rule="EC2SecurityGroupOpenToWorldRule",
                 rule_mode=RuleMode.BLOCKING,

diff --git a/...est_templates/rules/EC2SecurityGroupOpenToWorldRule/invalid_security_group_port78_81.json b/...est_templates/rules/EC2SecurityGroupOpenToWorldRule/invalid_security_group_port78_81.json
@@ -0,0 +1,19 @@
+{
+  "AWSTemplateFormatVersion": "2010-09-09",
+  "Resources": {
+    "SecurityGroup": {
+      "Type": "AWS::EC2::SecurityGroup",
+      "Properties": {
+        "GroupDescription": "description",
+        "SecurityGroupIngress": [
+          {
+            "IpProtocol": "tcp",
+            "CidrIp": "0.0.0.0/0",
+            "FromPort": 78,
+            "ToPort": 81
+          }
+        ]
+      }
+    }
+  }
+}