fix(query): fn for remote desktop port open to internet and other "security group" associated queries --terraform/aws--cloudformation/aws

[cx-andre-pereira]

Reason for Proposed Changes

• Currently the "Remote Desktop Port Open To Internet" terraform query will only flag if the exposed port is defined as an inline "ingress" rule of an "aws_security_group" resource.

• This leaves out any "aws_vpc_security_group_ingress_rule" that should also be flagged; in fact "Using aws_vpc_security_group_egress_rule and aws_vpc_security_group_ingress_rule resources is the current best practice" as stated in the official documentation.

• For a given "ingress" rule to be considered open it must be evaluated as "true" by the function "portOpenToInternet" from the terraform library.

• The auxiliary function checks that the "cidr_blocks" field contains "0.0.0.0/0" or equivilant IP, then the "protocol" field must be set to "tcp" and, finnaly, the port, in this case 3389, has to be included in the range determined by the "from_port" and "to_port" fields.

• This issue was raised followed by several identical ones that apply to other terraform queries. This sparked an investigation into the full scope of queries in need of updating for ingress/egress rules as resources. Both terraform and cloudformation queries require this update. Cloudformation equivalent resources:

  • "aws_security_group" - "AWS::EC2::SecurityGroup"
  • "aws_vpc_security_group_ingress_rule" - "AWS::EC2::SecurityGroupIngress"
  • "aws_vpc_security_group_egress_rule" - "AWS::EC2::SecurityGroupEgress"

• The cloudformation queries will be handled through a separate PR to facilitate review processes.

• Affected Terraform Queries :

  • "HTTP Port Open To Internet"
  • "Remote Desktop Port Open To Internet"
  • "Security Group Rule Without Description" + "Security Group Without Description"
  • "Security Group With Unrestricted Access To SSH"
  • "Sensitive Port Is Exposed To Entire Network"
  • "Sensitive Port Is Exposed To Small Public Network"
  • "Sensitive Port Is Exposed To Wide Private Network"
  • "SQL Analysis Services Port 2383 (TCP) Is Publicly Accessible"
  • "Unknown Port Exposed To Internet" - Query 10
  • "Unrestricted Security Group Ingress" - Query 11

Aside from the "aws_vpc_security_group_ingress_rule" resource the "aws_vpc_security_group_egress_rule" is also relevant and unsupported for some queries and for a large amount of them the "security-group" module is relevant but completely unsupported, if supported it is also missing Ipv6 support. IPv6 support is also lacking for the mentioned resources and the "aws_security_group" resource.

Proposed Changes

• assets\libraries

  • common.rego
    • Added ipv6 support for the "isPrivateIP" helper function and non-canonical forms of ipv6 "::/0" address to "is_unrestricted".
  • terraform.rego

    • Added "is_security_group_ingress" helper function to check that an aws security group "rule"(generic or explicit) is an ingress rule.

    • Added "cidr_is_unmasked" to check that the mask of an ip address is == "/0", made to support all possible cidr_block declarations associated with security groups.

    • Improved the "portOpenToInternet" function to support the naming scheme present in the aws_vpc_security_group_ingress_rule/aws_vpc_security_group_egress_rule resources which is as follows (compared to in-line aws_security_group ingress):

      • ingress.cidr_block == aws_vpc_security_group_ingress_rule.cidr_ipv4
      • ingress.ipv6_cidr_blocks == aws_vpc_security_group_ingress_rule.cidr_ipv6
      • ingress.protocol == aws_vpc_security_group_ingress_rule.ip_protocol

    • This equivalences are not one to one since the "aws_vpc_security_group_ingress_rule" cidr will always be a single string, it is not a string array like the Ingress's "cidr_blocks".

    • I updated the "portOpenToInternet" and "check_cidr" functions to support the new resource, note that for the "portOpenToInternet" function, when checking a "ip_protocol", "tcp" is no longer the only valid value; the documentation states a value of "-1" "translates to all protocols, all port ranges"; to furthur prove this point the documentation for the aws_security_group resource explicitly states "-1" is not a valid value for the Igress's "protocol" field which explains why it is not checked for in the current auxiliary function.

    • The generic security_group_rule allows "-1" values for a "protocol" field and the check for port range is not done when "-1" is set because setting this value ignores the port ranges altogether. The documentation states:

      • "Setting protocol = "all" or protocol = -1 with from_port and to_port will result in the EC2 API creating a security group rule with all ports open. This API behavior cannot be controlled by Terraform and may generate warnings in the future."

    • For security group ingress rules the documentation states:

      • "(Required) Protocol. If you select a protocol of -1 (semantically equivalent to all, which is not a valid value here), you must specify a from_port and to_port equal to 0."

    • It seems using "-1" and "all" can lead to unexpected results but given the context it sounds as though any protocol set to this value should be flagged regardless of port ranges.

    • Finally the new "get_ingress_list" function was created to simplify and unify case handling of "ingress" blocks so that a single CxPolicy can deal with either an individual "ingress" block and an array of them inside an "aws_security_group" resource.

• assets/queries

  • Group 1 - port open to internet queries
    • http_port_open : Current Implementation / Documentation
    • security_group_with_unrestricted_access_to_ssh : Current Implementation / Documentation
    • remote_desktop_port_open_to_internet : Current Implementation / Documentation
    • sql_analysis_services_port_2383_is_publicly_accessible : Current Implementation / Documentation

  These queries were grouped since their implementation is near identical and so were the adjustments made to them as a result. All of them rely on the auxiliar portOpenToInternet function to flag in case a specific port is exposed.

  Test files have been adjusted for all the new case scenarios and have been made as similar as possible to facilitate review and future alterations. The queries implementation itself was also altered to be as similar as possible between the 4 of them.

  For all these queries support for "aws_vpc_security_group_ingress_rule" and "aws_security_group_rule" was not present and was added through a single CxPolicy. Support for ipv6 addresses was also ensured. For module support only security_group_with_unrestricted_access_to_ssh had it and it did not take ipv6 into account.

  All the queries now support modules in a single CxPolicy (ssh query had 2 for array handling which is now delegated to helper functions). The original functions, with the exception of "ssh", only accounted for "aws_security_group" resources and did not have case handling in case there was more than a single ingress associated with said group and without ipv6 support.

  The queries use 3 CxPolicy to cover these scenarios relying on equivalent helper functions like "http_is_open" to return "search" and "key" values.

  • Group 2 - sensitive port queries
    • sensitive_port_is_exposed_to_entire_network : Current Implementation / Documentation
    • sensitive_port_is_exposed_to_small_public_network : Current Implementation / Documentation
    • sensitive_port_is_exposed_to_wide_private_network : Current Implementation / Documentation

  As their name implies this queries are also very similar in nature but their current implementations are varied. The simplest one is the "small public network" query, the implementation for it is very limited only being valid for "aws_security_group" resources with a single "ingress" block.

  For the "entire network" query, the "aws_security_group" resource is also the only one supported but there are 2 CxPolicy to handle multiple ingress blocks for a single resource.

  Finally the "wide private network" query once again only supports a single "ingress" block per "aws_security_group" resource, but has support for modules(ipv4).

  All the implementations are lacking support for ipv6 fields and "aws_vpc_security_group_ingress_rule" and "aws_security_group_rule" resources.

  The "isSmallPublicNetwork" helper function (sensitive_port_is_exposed_to_small_public_network) needed to include new "prefixes" to qualify as small network since, for an ipv4 address a /25 mask means 128 total addressable hosts but to an ipv6 network it means ~1.01 × 10³¹. The scales are completely different so, to correspond with the /25 to /29 values used for ipv4, ipv6 links are checked for masks ranging from /121 to /125.

  The "isPrivateNetwork" helper function (sensitive_port_is_exposed_to_wide_private_network) was also tweaked to check for all possible "cidr" field names; the "isPrivateIP" function from the common library was updated to allow ipv6 support in this query.

  • Test structure for Group 1 and 2

    • For all queries there is now support for:
      • Ipv4/ipv6 in all circumstances
      • "security-group" modules
      • "aws_vpc_security_group_ingress_rule" and "aws_security_group_rule" resources
      • Single ingress and array of ingress blocks
    • All the test files follow an identical structure as well, with 4 positive tests and 4 negative tests:
      • positive1/negative1 for all "aws_security_group" resource scenarios
      • positive2/negative2 for all "aws_vpc_security_group_ingress_rule" resource scenarios
      • positive3/negative3 for all "aws_security_group_rule" resource scenarios
      • positive4/negative4 for all "security-group" module scenarios

  • Group 3 - description queries

    • security_group_rules_without_description : Current Implementation / Documentation
    • security_group_without_description : Current Implementation / Documentation

  I started by updating the security_group_rules_without_description query and later realized the security_group_without_description was missing a check for modules too. This queries are structured so that the "group_rules" query checks for all "egress" and "ingress" blocks inside "aws_security_group" resources as well as all "aws_security_group_rule" resources and flags in case they are missing a description.

  This current implementation was missing the case of "aws_vpc_security_group_ingress_rule" and "aws_vpc_security_group_egress_rule" resources. Additionally it was not checking for the equivalent "rules" inside the relevant module those being "egress_with_cidr_blocks/egress_with_ipv6_cidr_blocks" and "ingress_with_cidr_blocks/ingress_with_ipv6_cidr_blocks".
  For the security_group_without_description i added the check for the description field of the "security-group" module itself.

  For testing a positive2/negative2 with a simple sample with a "module" was added to the security_group_without_description query; and for the security_group_rules_without_description the positive1-3 and negative1-3 testes were kept pretty much unchanged; new positive4/negative4 tests were added for "aws_vpc_security_group_ingress_rule" resources and positive5/negative5 tests for "security-group" modules.

  • Query 10 unknown_port_exposed_to_internet : Current Implementation / Documentation

  This query, currently, only works for "aws_security_group" ingress blocks. It checks that a port range (from x to x) includes unknown ports , as in any port not specified within the "tcpPortsMap" map of the common library, and that the cidr includes "0.0.0.0/0".

  The current implementation is shallow missing not only ipv6 handling but also "aws_security_group_rule", "aws_vpc_security_group_ingress_rule" and "security-group" module support. The "check_cidr" terraform library function will allow ipv6 open addresses to be detected. New CxPolicy(s) for modules and rules were added. The new comprehensive test files follow the 1-4 structure defined for Group 1 and 2.

  • Query 11 unrestricted_security_group_ingress : Current Implementation / Documentation

  The number of CxPolicy(s) for the current implementation of this query is very high; it handles a lot of scenarios : "aws_security_group_rule"(ipv4/ipv6), "aws_security_group"(single ingress and array + ipv4/ipv6), modules with "ingress_cidr_blocks" and "ingress_ipv6_cidr_blocks". It uses a new CxPolicy for every valid scenario (8).

  The new implementation has lowered the CxPolicy ammount to just 3 while relying on some big helper functions. This query stands out from the other due to the fact that the "cidr" fields can and should be pointed to by the "key" and "search" values in the results; this adds some complexity since the "cidr" fields have different names depending on context.

  A new case was also added to the modules since the original implementation was not taking into account "ingress_with_cidr_blocks" and "ingress_with_ipv6_cidr_blocks" which allow for a full ingress block to be defined inside the module and not just "cidr" values like the "ingress_cidr_blocks" and "ingress_ipv6_cidr_blocks" that were already accounted for.

  Original test files were adjusted to follow the standard 1-4 described in "Group 1 and 2".

Final Notes

• With the change a negative test for "Network ACL With Unrestricted Access To SSH" and identical test for "Network ACL With Unrestricted Access To RDP" started to flag since the test was for a "default_network_acl_ingress" resource with protocol set to "-1" which is allowed as per the documentation. The test was supposed to be negative since only the "tcp" protocol could flag before. They have been adjusted so the "cidr_block" is "0.0.0.1/0" making it a True Negative test once again.

Fixs Post Merger:

• test(query): better negative testing for "Unknown Port Exposed To Internet" - terraform/aws #7725
• test(query): fix for positive4 test to "Unknown Port Exposed To Internet" query #7726
• test(query): better testing for security group rules without description #7729
• test(query): test improvement for the "sensitive port" terraform queries #7731
• test(query): added new case for the several resources covered on the queries for terraform/aws #7734

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-artur-ribeiro]

LGTM

[cx-artur-ribeiro]

Hey André, insane job you did here. From the new cases to the auxiliary functions and search lines, you showed great judgment in making these changes. I left a few comments for possible improvements, take a look and see what you think/if you agree.

[cx-artur-ribeiro]

LGTM

[gitguardian]

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
20838717	Triggered	Generic Password	24678a5	assets/queries/azureResourceManager/sql_server_database_with_alerts_disabled/test/negative7.json	View secret

🛠 Guidelines to remediate hardcoded secrets

1. Understand the implications of revoking this secret by investigating where it is used in your code.
2. Replace and store your secret safely. Learn here the best practices.
3. Revoke and rotate this secret.
4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

• following these best practices for managing and storing secrets including API keys and other credentials
• install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

---

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

[cx-artur-ribeiro]

LGTM