Skip to content

fix(query): fix fp for s3_bucket_access_to_any_principal#7564

Merged
cx-andre-pereira merged 16 commits into
Checkmarx:masterfrom
cx-andre-pereira:AST-94942--FP_S3_Bucket_Access_to_Any_Principal--cloudFormation/aws
Jul 30, 2025
Merged

fix(query): fix fp for s3_bucket_access_to_any_principal#7564
cx-andre-pereira merged 16 commits into
Checkmarx:masterfrom
cx-andre-pereira:AST-94942--FP_S3_Bucket_Access_to_Any_Principal--cloudFormation/aws

Conversation

@cx-andre-pereira

Copy link
Copy Markdown
Contributor

Reason for Proposed Changes

  • Currently the query s3_bucket_access_to_any_principal can only determine if a "Principal" has permissions "*" or "s3:*" from YAML files if it has no "!If" statements associated.
  • With "!If" statements the flag will trigger every time due to lack of specific case handling . The query does not take into consideration the existence of If´s.
  • The If structure is as follows : (YAML)
    - <CONDITION>
    - <THEN>
    - <ELSE>
  • The values for <THEN> and <ELSE> require scanning that is currently inexistent. Either of these values can have meaningful policy statements for this query.

Proposed Changes

  • Introduced two helper functions (handle_if_statements and handle_if_statement) to correctly traverse and extract valid policy statements from structures resulting from !If conditions.

  • This improves the query’s accuracy by avoiding false positives when Principal is not actually "*" in resolved policy logic, and also guarantees the verifications of each and every meaningful object in the if statement be it a <THEN> or <ELSE> associated object.

originalPR

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 16, 2025 11:40
@github-actions github-actions Bot added community Community contribution query New query feature labels Jul 16, 2025

@cx-artur-ribeiro cx-artur-ribeiro 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. We could improve the way the if statement is being handled by supporting this flow in the library instead of the function.
But since the scope of this PR is to only fix the query, we can add that as a later improment.

@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

@cx-andre-pereira cx-andre-pereira merged commit d4b6f71 into Checkmarx:master Jul 30, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community Community contribution query New query feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants