Skip to content

fix(query): fn for security_group_with_unrestricted_access_to_ssh - terraform/aws#7568

Merged
cx-andre-pereira merged 6 commits into
Checkmarx:masterfrom
cx-andre-pereira:AST-40778--FN_ECS_Cluster_not_encrypted_at_rest_missing_results--terraform/aws
Aug 12, 2025
Merged

fix(query): fn for security_group_with_unrestricted_access_to_ssh - terraform/aws#7568
cx-andre-pereira merged 6 commits into
Checkmarx:masterfrom
cx-andre-pereira:AST-40778--FN_ECS_Cluster_not_encrypted_at_rest_missing_results--terraform/aws

Conversation

@cx-andre-pereira

@cx-andre-pereira cx-andre-pereira commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

Reason for Proposed Changes

  • This query is meant to ensure that terraform/aws resource/module security groups do not have the ssh port (22) exposed on an "Ingress" block associated with that group; that is to say the cidr_blocks of a given Ingress cannot have the reserved IP adress "0.0.0.0/0" while exposing port 22.
  • There are, currently, 2 major flaws with this implementation, for starters it does not account for the fact that one security group might have more than 1 "Ingress", if this is the case the resulting payload will come in array form and there is currently no case handling in place for either resource or module blocks of this sort.
  • Furthermore it lacks support for ipv6_cidr_blocks which can represent the aforementioned IP address in 3 main ways:
   - "::/0"  (standard / canonical form)
   - "0:0:0:0:0:0:0:0/0"
   - "0000:0000:0000:0000:0000:0000:0000:0000/0" 
  • This representations are all very different from the equivalent ipv4 value and must be accounted for.

Proposed Changes

  • I implemented correct case handling for arrays of ingress blocks.

  • To support ipv6 cidr_block some improvements were made to the default terraform library´s check_cidr policy which is essential for the portOpenToInternet policy that the query depends on.

  • Finally the number of tests was greatly increased to account for the many use case scenarios. In the current implementation there are 2 variables considered: being a resource/module and having a cidr_blocks associated with a single value or 2+ values. Now we have to consider both those variables plus arrays of ingress blocks, ipv6_cidr_blocks and ipv6_cidre_blocks + (ipv4)cidr_blocks.

  • Given this the tests have been expanded from 2 negative to 8 and 4 positive to 22. Rundown of test´s purpose:

    • Negative 3 and 4 show more than 1 ingress for resource and module.
    • Negative 5 and 6 replace ipv4 cidr_blocks with ipv6 for resource and module.
    • Negative 7 and 8 replace ipv4 cidr_blocks with ipv6 for resource and module with more than 1 ingress.
    • Positive 5 to 8 show more than 1 ingress for resource and module and cidr_block associated with multiple/single_value(s).
    • Positive 9 to 12 show single ingress for ipv6_cidr_blocks (similar to 1 to 4).
    • Positive 13 to 16 show the same as 5 to 8 this time with ipv6_cidr_blocks .
    • Positive 17 to 20 show single ingress with both a ipv4 and ipv6 cidr_blocks simultaneously for resource and module.
    • Positive 21 is similar to 17 to 20 with a list of ingress blocks for a resource.
    • Positive 22 is similar to 17 to 20 with a list of ingress blocks for a module.
  • Spreadsheat of features tested (combinations):

image
  • Finally there was also a change to the line pointed to by the resulting "search_Key" of the query it now points to the Ingress as a whole. The previous logic was unnecessary and not informative to the user. It is now inline with the similar query "Network ACL With Unrestricted Access To SSH"

I submit this contribution under the Apache-2.0 license.

@cx-andre-pereira cx-andre-pereira requested a review from a team as a code owner July 17, 2025 13:34
@github-actions github-actions Bot added community Community contribution query New query feature terraform Terraform query aws PR related with AWS Cloud labels Jul 17, 2025

@cx-eduardo-semanas cx-eduardo-semanas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gitguardian

gitguardian Bot commented Aug 7, 2025

Copy link
Copy Markdown

⚠️ GitGuardian has uncovered 2 secrets 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.

Since your pull request originates from a forked repository, GitGuardian is not able to associate the secrets uncovered with secret incidents on your GitGuardian dashboard.
Skipping this check run and merging your pull request will create secret incidents on your GitGuardian dashboard.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
4266022 Triggered Generic Password 4c4d1e4 assets/queries/cloudFormation/aws/amplify_app_basic_auth_config_password_exposed/test/negative7.yaml View secret
9419039 Triggered Username Password 4c4d1e4 assets/queries/cloudFormation/aws/amplify_branch_basic_auth_config_password_exposed/test/positive5.yaml 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 secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  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


🦉 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-andre-pereira cx-andre-pereira merged commit d3bb7e4 into Checkmarx:master Aug 12, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aws PR related with AWS Cloud community Community contribution query New query feature terraform Terraform query

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants