[BUG] CIDR Netmasks are strings, not netmasks

[pacohope]

Describe the bug

In the example security ingress/egress rules, the only CIDR range that will flag as "open to the world" is the exact string of 0.0.0.0/0. For example, 1.0.0.0/0 is also open to the world, but if I process my template where that's the ingress CIDR, I see:

ec2-secgroup-inbound-outbound-access.guard/prevent_inbound_access_to_any_ip     PASS

To Reproduce

I am pretty sure if you change the ingress test case to 1.0.0.0/0 it will pass the test when it should fail. EC2/VPC are perfectly happy to accept that as a CIDR, because it is valid syntactically. They will create an ingress CIDR range that, when you do describe-security-groups will show as 0.0.0.0/0.

The same is probably true for IPv6 CIDR ranges (I haven't tested) because it looks like string matching on ::0.

Expected behavior

If we must treat IPv4 CIDRs as strings, can we just match strings that end with /0? That would be be a wee bit more robust.

Ideally we should parse CIDRs as data structures, extract the netmask as an integer, and complain on small numbers like 8 or less.

[dannyvassallo]

Hello @pacohope! Sorry for the very late reply. A lot has changed since 2021 so I figured I'd drop in with a suggestion to remedy this using regex:

let sg_resources = Resources.*[
    Type == "AWS::EC2::SecurityGroup"
]

rule prevent_outbound_access_to_any_ip when %sg_resources !empty {
    # select egress rules that are strings
    let egress = %sg_resources.Properties.SecurityGroupEgress[
        CidrIp      is_string or
        CidrIpv6    is_string
    ]

    when %egress !empty {
        %egress.CidrIp      != /^(?:\d{1,3}\.){3}\d{1,3}\/0$/ <<IPv4 address cannot be open to the world (e.g., any CIDR ending in /0)>> or
        %egress.CidrIpv6    != /^::\/0$/ <<IPv6 address cannot be open to the world (e.g., CIDR ::/0)>>
    }
}

rule prevent_inbound_access_to_any_ip when %sg_resources !empty {
    let ingress = %sg_resources.Properties.SecurityGroupIngress[
        CidrIp      is_string or
        CidrIpv6    is_string
    ]

    when %ingress !empty {
        %ingress.CidrIp      != /^(?:\d{1,3}\.){3}\d{1,3}\/0$/ <<IPv4 address cannot be open to the world (e.g., any CIDR ending in /0)>> or
        %ingress.CidrIpv6    != /^::\/0$/ <<IPv6 address cannot be open to the world (e.g., CIDR ::/0)>>
    }
}

When checking locally and applying your suggested reproduce steps I am able to have the test fail as expected. Please let me know if this is a viable solution for you. Thanks!

[pacohope]

This pattern match will accurately pluck off the netmask part of a CIDR range, so it won't be fooled by 1.1.1.1/0. It is still treating the netmask as a string, and it's only matching the string "0". It's not considering the netmask as a number. And if you consider how much IPv6 space is allowed when the CIDR range is ::/1, it's still pretty bad. This regular expression will catch all the CIDRs that are literally 0 in the netmask, but I'm arguing that that is insufficient. For an IPv4 netmask, less than 8 is probably worth alerting (heck, imagine someone tried to type /24 and they typoed /2). For an IPv6 range, I think less than 32 is probably worth alerting (again, imagine someone meant to type ::/64 and accidentally typed ::/6).

My point was that CIDR ranges and netmasks are numbers, not strings. So string matching doesn't get it done because string matching doesn't do math.