Skip to content

Improve CQL Injection Query #200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

jeongsoolee09
Copy link
Contributor

@jeongsoolee09 jeongsoolee09 commented Jun 19, 2025

What this PR contributes

  • Improve CQL Injection Query.

    • A new class CqlQueryRunnerCall now expands and replaces TaintedCqlClause, now covering cds.run, cds.db.run, srv.run, and tx.run.
      • Also, it models CRUD-style "shortcut calls" that translates to running a CQL on a service under the hood.
    • Expand base objects as possible receivers of methods .run and property read entities:
      • EntityEntry is "absorbed" into EntityReference. Plus, EntityReference now covers more examples, namely, cds as a shortcut to cds.db.
    • Make the alert message more useful by making the query itself the primary location which is different from sink (the string concatenation).
  • Add robust test cases (This is the gist of this PR, please take a look for the behavioral summary description of what this PR aims to implement).

    • send11-send17: Service1 runs query on the database service using cds.run and friends.
    • send21-send25: Service1 runs query on itself by await-ing the query.
    • send31-send37: Service1 runs query on itself using this.run and friends.
    • send41-send47: Service1 runs query on Service2 using Service2.run and friends.
    • send5: Service1 runs query on Service2 using CQN parsed with cds.ql.
    • send6: Service1 runs query on the database service using CQN parsed with cds.parse.cql.
    • send7: Service1 runs query on the database service using CQN parsed with global function CQL.
    • send81: Service1 runs query on Service2 using an unparsed CDL string (only valid in old versions of CAP).
    • send91-send97: Service1 runs query on Service2 using Service2.tx( tx => tx.run(...) ) and friends.
    • send101-send107: Service1 runs query on itself using this.tx( tx => tx.run(...) ) and friends.
    • send111-send117: Service1 runs query on the database service using cds.tx( tx => tx.run(...) ) and friends.
    • send121-send127: Service1 runs query on the database service using cds.db.tx( tx => tx.run(...) ) and friends.

Future works

  • The alert message can be more refined: if a sink is in cds.read(...).from(...), only the cds.read(...) part is alerted on, where the entire chained method call is more desirable as a alert location.
  • SensitiveExposure.ql seems to be quite brittle, it needs a rewrite. This PR only updates the query's reference to old definitions that are no longer available.

@jeongsoolee09 jeongsoolee09 marked this pull request as draft June 19, 2025 13:22
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@jeongsoolee09 jeongsoolee09 marked this pull request as ready for review June 24, 2025 23:08
@jeongsoolee09 jeongsoolee09 requested a review from lcartey June 24, 2025 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant