fix(query): adding support for CloudFormation queries missing ingress/egress resources - Part 2

[cx-andre-pereira]

Reason for Proposed Changes

• Part 1 | Part 3

• Currently some CloudFormation queries do not support the newer "AWS::EC2::SecurityGroupIngress" and "AWS::EC2::SecurityGroupEgress" resources, only working for samples with the legacy "AWS::EC2::SecurityGroup" resource.

• This Pull Request will handle queries 6-10.

Query List

Query Name	Query ID	Query Logic
ELB With Security Group Without Outbound Rules	01d5a458-a6c4-452a-ac50-054d59275b7c	query 1
DB Security Group With Public Scope	9564406d-e761-4e61-b8d7-5926e3ab8e79	query 2
Default Security Groups With Unrestricted Traffic	ea33fcf7-394b-4d11-a228-985c5d08f205	query 3
EC2 Sensitive Port Is Publicly Exposed	494b03d3-bf40-4464-8524-7c56ad0700ed	query 4
ELB Sensitive Port Is Exposed To Entire Network	78055456-f670-4d2e-94d5-392d1cf4f5e4	query 5
HTTP Port Open To Internet	ddfc4eaa-af23-409f-b96c-bf5c45dc4daa	query 6*
Remote Desktop Port Open To Internet	c9846969-d066-431f-9b34-8c4abafe422a	query 7*
Security Groups Allows Unrestricted Outbound Traffic	66f2d8f9-a911-4ced-ae27-34f09690bb2c	query 8*
Security Group Unrestricted Access To RDP	3ae83918-7ec7-4cb8-80db-b91ef0f94002	query 9*
Security Groups With Exposed Admin Ports	cdbb0467-2957-4a77-9992-7b55b29df7b7	query 10*
Security Groups With Meta IP	adcd0082-e90b-4b63-862b-21899f6e6a48	query 11
Security Group With Unrestricted Access To SSH	6e856af2-62d7-4ba2-adc1-73b62cef9cc1	query 12
Unknown Port Exposed To Internet	829ce3b8-065c-41a3-ad57-e0accfea82d2	query 13
DB Security Group Open To Large Scope	0104165b-02d5-426f-abc9-91fb48189899	query 14

• While analyzing other relevant queries I realized the Fully Open Ingress query was not checking for inline ingresses, only the standalone ones; additionally it only check for ipv4 "CidrIp" but not the ipv6 equivalent "CidrIpv6".

Proposed Changes

• ✅HTTP Port Open To Internet and ✅Remote Desktop Port Open To Internet

  • These queries have near identical implementations differing only on the ports they check for : 80(HTTP) or 3389(RDP). Their implementation is similar to some queries from the Part 1 where i make use of the "search_for_standalone_ingress", "get_search_values" and "get_ingress_list_if_exists" to find every ingress rule declared and properly point to it.

• ✅Security Groups Allows Unrestricted Outbound Traffic

  • This query uses similar functions to the ones i just mention simply adapted for the "egress" outbound rules, then it is checked that the IpProtocol is set to "-1" instead of the "ALL" value; form my research this simply did not happen on valid configurations, even legacy aws versions that informally supported "all" did not support a caps locked version of it like the current implementation of this query does.
  • Aside from the missing support for "AWS::EC2::SecurityGroupEgress", ipv6 support was included.

• ❌Security Group Unrestricted Access To RDP

  • When I started to analyze this query i quickly realized it is just an incomplete version of the Remote Desktop Port Open To Internet detailed before. I believe it is best to remove this query altogether since it checks for the exact same scenario but only check that either the "FromPort" or "ToPort" values are explicitly set to 3389 (RDP port), completely missing any ports that define a relevant range.

• ✅Security Groups With Exposed Admin Ports

  • Once again the implementation is similar to many done before using the "search_for_standalone_ingress", "get_search_values" and "get_inline_ingress_list" functions.

• (EXTRA)✅Fully Open Ingress

  • Added support for ipv6 and inline ingress arrays.

• (EXTRA)✅Security Group Egress With All Protocols and ✅Security Group Ingress With All Protocols

  • Currently these queries check that the IpProtocol field is set to -1, instead of the string value "-1", this is incorrect since , although numeric values can be used they must be written as strings. Numeric values could be interpreted as intended but by writing them as strings we can be sure that is the case and choosing between supporting only numeric values as the query does current or the intended string values is a simple choice; the case could be made to support both types allowing -1 and "-1" (in this case).

• (EXTRA)✅Security Group Egress With Port Range and ✅Security Group Egress CIDR Open To World

  • Unlike their "Ingress" counterpart these queries were missing "searchLine" values, they have now been added to both queries.

• (EXTRA)✅Security Group Rule Without Description

  • This query was only checking for the "AWS::EC2::SecurityGroup" resource and not the outdated AWS::RDS::DBSecurityGroup. Support was added.

• On 0735b11 aside from some fixes and slight test changes for some queries I updated the "positive.yaml" sample from the e2e/fixtures/samples folder, the fact of the matter is i could not find a single source to support having a "fromPort" with a value lower that the "toPort", and the current file does so in 2 of the 3 from/to port ranges it defines. I did find 2 sources explicitly stating that this behavior is impossible for a valid range :

  • "AWS::NetworkFirewall::TLSInspectionConfiguration PortRange"
    • "FromPort The lower limit of the port range. This must be less than or equal to the ToPort specification." and "ToPort The upper limit of the port range. This must be greater than or equal to the FromPort specification."
  • Aws archive :
    • "FromPort (integer) -- [REQUIRED] The lower limit of the port range. This must be less than or equal to the ToPort specification." and "FromPort (integer) -- [REQUIRED] The lower limit of the port range. This must be less than or equal to the ToPort specification."

• These refer to the same fields on different resources but there is no reason to believe it works differently in the given context.

I submit this contribution under the Apache-2.0 license.

[github-actions]

KICS version: v2.1.13

Category Results CRITICAL 0 HIGH 0 MEDIUM 0 LOW 0 INFO 0 TRACE 0 TOTAL 0	Metric Values Files scanned 1 Files parsed 1 Files failed to scan 0 Total executed queries 47 Queries failed to execute 0 Execution time 0

[cx-andre-pereira]

Part 3 will be the only one to be merged, it will include all query fixes associated with this second part and part1.