-
Notifications
You must be signed in to change notification settings - Fork 3
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
Merged
Merged
Changes from 53 commits
Commits
Show all changes
70 commits
Select commit
Hold shift + click to select a range
ac0265e
Minor readability
jeongsoolee09 6fd6b2a
Make definition of `ImplMethodCallApplicationServiceDefinition` more …
jeongsoolee09 439ad43
Add `srv.entities` as part of `EntityEntry`
jeongsoolee09 3b7d741
Put CQL query-relevant definitions in a query library
jeongsoolee09 440712f
Do some refactoring
jeongsoolee09 dfa8c08
Sort out EntityReference
jeongsoolee09 571b316
Create a new CQL injection test project and move the old one to a folder
jeongsoolee09 1262260
Minor numbering
jeongsoolee09 908a572
install `@sap/cds-dk` and bump version of `@sap/cds`
jeongsoolee09 a8a0bb4
Move existing CQL Injection test case to `old/`
jeongsoolee09 5e39be5
Refine test cases and add more cases
jeongsoolee09 25450a8
Finalize working draft on current CqlInjection test case
jeongsoolee09 0f95069
Finalize CqlInjection
jeongsoolee09 1e83f5a
Fix test case and code
jeongsoolee09 9a2e99b
Appease compiler to make SensitiveExposure pass
jeongsoolee09 c5c9740
Debug `isSink`
jeongsoolee09 89522d0
Cover all cases in the current CQL injection test
jeongsoolee09 43738d5
Get the query depending on the type of sink
jeongsoolee09 13691d4
Minor formatting and tidying up
jeongsoolee09 468a780
Docstrings and comments
jeongsoolee09 a391d34
Fix a regression in `taintedclause`
jeongsoolee09 39b2397
Update expected test outputs of old cqlinjection.js
jeongsoolee09 fd0d13a
Fix a regression in applicationserviceinstance
jeongsoolee09 621a319
Fix regression in SensitiveExposure
jeongsoolee09 a0f6d9a
Remove unneeded code
jeongsoolee09 7652149
Update expected results of sensitive-exposure
jeongsoolee09 2e56ae0
Fix numbering and remove duplicate case
jeongsoolee09 a13a825
Make the new CQL injection test project a proper test case and remove…
jeongsoolee09 8a99661
Remove unneeded comment
jeongsoolee09 5c1a5ba
Checkpoint
jeongsoolee09 a93a7b1
Add some more unit test cases
jeongsoolee09 d968632
Add some more cases and add FP tags
jeongsoolee09 fb979d2
Remove the old CQL injection test files
jeongsoolee09 bdfc5bd
Add FP cases that are left out in the previous commit.
jeongsoolee09 0004b88
Debug query according to the new test cases
jeongsoolee09 f0bb40c
Add expected test result of new CQL injection test
jeongsoolee09 8a6e11d
Update test results of sensitive-exposure
jeongsoolee09 18ef3b5
Update expected analysis results
jeongsoolee09 cd26594
Remove `CdsFacade.getNode/0`
jeongsoolee09 d7f5ed4
Use `getStringValue/0` in `getNamespace/0`
jeongsoolee09 cee86f1
Add documentation, make `CdsDb` extend `CdsDbService`, and remove `is…
jeongsoolee09 5d52200
Factor out sink definition in a class and associate location reportin…
jeongsoolee09 caac1ae
Add some comments and docstrings
jeongsoolee09 97d6a01
Minor documentation
jeongsoolee09 9c3138b
Update test result
jeongsoolee09 378436a
Tidy up code
jeongsoolee09 689e00a
Refine the start and end of the second and third steps
jeongsoolee09 806e615
Explicitly use argument index in `CdsTransaction.getContextObject/0`
jeongsoolee09 dc42b54
Improve writing of a QLDoc
jeongsoolee09 28ae679
Explicitly state the safe/unsafe reason and add some documentation
jeongsoolee09 a3efdd7
Add documentation to `CdsTransaction`
jeongsoolee09 c05e8a6
Remove duplicate logic introduced by `exists(VarRef ... )`
jeongsoolee09 c3c882f
Update expected results of CQL injection query
jeongsoolee09 9101ff3
Remove all test cases that include `entries`
jeongsoolee09 a763874
Remove propagation steps for INSERT and UPSERT
jeongsoolee09 3ae7055
Update expected Code Scanning Results
jeongsoolee09 3b073eb
Remove related logic that detects query parameters
jeongsoolee09 4e77ab2
Bring back test cases with `entries`
jeongsoolee09 7a2db7b
Update expected result of CQL injection query
jeongsoolee09 d286f51
Flip `UNSAFE` label to `SAFE` and fix/remove impossible cases
jeongsoolee09 0d4942f
Remove additional logic to detect `entries` cases and remove the case…
jeongsoolee09 66ef528
Update expected result of Code Scanning
jeongsoolee09 565dde2
Fix a regression where the alert was not made if a child CQL clause i…
jeongsoolee09 71ff926
Fix wrong SAFE labels and shift comment location
jeongsoolee09 16c6157
Remove safe cases from expected results
jeongsoolee09 d26d930
Minor relocation of a label
jeongsoolee09 375a26a
Mark the entire shortcut method call as primary location
jeongsoolee09 f95efde
Change the non-clickable part of alert message
jeongsoolee09 5adcff5
Update expected results of code scanning
jeongsoolee09 6bfec53
Remove temporary internal test cases from expected results
jeongsoolee09 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
236 changes: 236 additions & 0 deletions
236
...t/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,236 @@ | ||
| import javascript | ||
| import semmle.javascript.security.dataflow.SqlInjectionCustomizations | ||
| import advanced_security.javascript.frameworks.cap.CQL | ||
| import advanced_security.javascript.frameworks.cap.RemoteFlowSources | ||
| import advanced_security.javascript.frameworks.cap.dataflow.FlowSteps | ||
|
|
||
| abstract class CqlInjectionSink extends DataFlow::Node { | ||
| /** | ||
| * Gets the data flow node that represents the query being run, for | ||
| * accurate reporting. | ||
| */ | ||
| abstract DataFlow::Node getQuery(); | ||
| } | ||
|
|
||
| /** | ||
| * A CQL clause parameterized with a string concatentation expression. | ||
| */ | ||
| class CqlClauseWithStringConcatParameter instanceof CqlClause { | ||
| CqlClauseWithStringConcatParameter() { | ||
| exists(DataFlow::Node queryParameter | | ||
| ( | ||
| if this instanceof CqlInsertClause or this instanceof CqlUpsertClause | ||
| then | ||
| queryParameter = this.getArgument().flow() | ||
| or | ||
| /* | ||
| * Account for cases where an object with a string concatenation is passed. e.g. | ||
| * ``` javascript | ||
| * let insertQuery = INSERT.into`SomeEntity`.entries({col1: "column_" + col}); | ||
| * ``` | ||
| */ | ||
|
|
||
| queryParameter = this.getArgument().flow().(SourceNode).getAPropertyWrite().getRhs() | ||
| else queryParameter = this.getArgument().flow() | ||
| ) and | ||
| exists(StringConcatenation::getAnOperand(queryParameter)) | ||
| ) | ||
| } | ||
|
|
||
| Location getLocation() { result = super.getLocation() } | ||
|
|
||
| string toString() { result = super.toString() } | ||
| } | ||
|
|
||
| /** | ||
| * An await expression that has as its operand a CQL clause that includes a | ||
| * string concatenation operation. | ||
| */ | ||
| class AwaitCqlClauseWithStringConcatParameter extends CqlInjectionSink { | ||
| DataFlow::Node queryParameter; | ||
| DataFlow::Node query; | ||
| CqlClauseWithStringConcatParameter cqlClauseWithStringConcat; | ||
|
|
||
| AwaitCqlClauseWithStringConcatParameter() { | ||
| exists(AwaitExpr await | | ||
| this = await.flow() and | ||
| await.getOperand() = cqlClauseWithStringConcat.(CqlClause).asExpr() | ||
| ) | ||
| } | ||
|
|
||
| override DataFlow::Node getQuery() { result = cqlClauseWithStringConcat.(CqlClause).flow() } | ||
| } | ||
|
|
||
| /** | ||
| * The first argument passed to the call to `cds.run`, `cds.db.run`, or `srv.run` | ||
| * whose value is a CQL query object that includes a string concatenation. e.g. | ||
| * ``` javascript | ||
| * // 1. CQN object constructed from Fluent API | ||
| * const query = SELECT.from`Entity1`.where("ID=" + id); | ||
| * cds.run(query); | ||
| * | ||
| * // 2. CQN object parsed from a string | ||
| * const query = cds.parse.cql("SELECT * from Entity1 where ID =" + id); | ||
| * cds.run(query); | ||
| * | ||
| * // 3. An unparsed CQL string (only valid in old versions of CAP) | ||
| * const query = "SELECT * from Entity1 where ID =" + id; | ||
| * Service2.run(query); | ||
| * ``` | ||
| * The `getQuery/0` member predicate gets the `query` argument of the above calls | ||
| * to `run`. | ||
| */ | ||
| class StringConcatParameterOfCqlRunMethodQueryArgument extends CqlInjectionSink { | ||
| CqlRunMethodCall cqlRunMethodCall; | ||
|
|
||
| StringConcatParameterOfCqlRunMethodQueryArgument() { | ||
| this = cqlRunMethodCall.getAQueryParameter() | ||
| } | ||
|
|
||
| override DataFlow::Node getQuery() { result = this } | ||
| } | ||
|
|
||
| /** | ||
| * A CQL shortcut method call (`read`, `create`, ...) parameterized with a string | ||
| * concatenation expression. e.g. | ||
| * ``` javascript | ||
| * cds.read("Entity1").where(`ID=${id}`); // Notice the surrounding parentheses! | ||
| * cds.create("Entity1").entries({id: "" + id}); | ||
| * cds.update("Entity1").set("col1 = col1" + amount).where("col1 = " + id); | ||
| * cds.insert("Entity1").entries({id: "" + id}); | ||
| * cds.upsert("Entity1").entries({id: "" + id}); | ||
| * cds.delete("Entity1").where("ID =" + id); | ||
| * ``` | ||
| */ | ||
| class CqlShortcutMethodCallWithStringConcat instanceof CqlShortcutMethodCall { | ||
| DataFlow::Node stringConcatParameter; | ||
|
|
||
| CqlShortcutMethodCallWithStringConcat() { | ||
| stringConcatParameter = super.getAQueryParameter() and | ||
| exists(StringConcatenation::getAnOperand(stringConcatParameter)) | ||
| } | ||
|
|
||
| Location getLocation() { result = super.getLocation() } | ||
|
|
||
| string toString() { result = super.toString() } | ||
|
|
||
| DataFlow::Node getStringConcatParameter() { result = stringConcatParameter } | ||
| } | ||
|
|
||
| /** | ||
| * A string concatenation expression included in a CQL shortcut method call. e.g. | ||
| * ``` javascript | ||
| * cds.read("Entity1").where(`ID=${id}`); // Notice the surrounding parentheses! | ||
| * cds.create("Entity1").entries({id: "" + id}); | ||
lcartey marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * cds.update("Entity1").set("col1 = col1" + amount).where("col1 = " + id); | ||
| * cds.insert("Entity1").entries({id: "" + id}); | ||
| * cds.upsert("Entity1").entries({id: "" + id}); | ||
| * cds.delete("Entity1").where("ID =" + id); | ||
| * ``` | ||
| * This class captures the string concatenation expressions appearing above: | ||
| * 1. `ID=${id}` | ||
| * 2. `"" + id` | ||
| * 3. `"col1 = col1" + amount` | ||
| * 4. `"col1 = " + id` | ||
| * 5. `"ID =" + id` | ||
| */ | ||
| class StringConcatParameterOfCqlShortcutMethodCall extends CqlInjectionSink { | ||
| CqlShortcutMethodCallWithStringConcat cqlShortcutMethodCallWithStringConcat; | ||
|
|
||
| StringConcatParameterOfCqlShortcutMethodCall() { | ||
| this = cqlShortcutMethodCallWithStringConcat.getStringConcatParameter() | ||
| } | ||
|
|
||
| override DataFlow::Node getQuery() { result = cqlShortcutMethodCallWithStringConcat } | ||
| } | ||
|
|
||
| /** | ||
| * A CQL parser call (`cds.ql`, `cds.parse.cql`, ...) parameterized with a string | ||
| * conatenation expression. | ||
| */ | ||
| class CqlClauseParserCallWithStringConcat instanceof CqlClauseParserCall { | ||
| CqlClauseParserCallWithStringConcat() { | ||
| not this.getCdlString().(StringOps::Concatenation).asExpr() instanceof TemplateLiteral and | ||
| exists(StringConcatenation::getAnOperand(this.getCdlString())) | ||
| } | ||
|
|
||
| Location getLocation() { result = super.getLocation() } | ||
|
|
||
| string toString() { result = super.toString() } | ||
| } | ||
|
|
||
| /** | ||
| * A data flow configuration from a remote flow source to a handful of sinks that run a CQL | ||
| * query, either directly or indirectly by assembling one under the hood. | ||
| * | ||
| * The CQL injection happens if a fluent API builder (`SELECT`, `INSERT`, ...) or a | ||
| * shortcut method call (`srv.read`, `srv.create`, ...) are called with a string | ||
| * concatentation as one of its argument, which in practice can take one of its | ||
| * following forms: | ||
| * | ||
| * 1. Concatentation with a string value with the `+` operator: | ||
| * - Concatenation with a string: `"ID=" + expr` | ||
| * - Concatenation with a template literal: `` `ID=` + expr `` | ||
| * 2. Template literal that interpolates an expression in it but is not a tagged | ||
| * template literal: `` SELECT.from`Entity`.where(`ID=${expr}`) `` | ||
| * | ||
| * The second case should be distinguished from the ones that have tagged template literals | ||
| * for all of its builder calls: if the example were `` SELECT.from`Entity`.where`ID=${expr}` `` | ||
| * instead (notice the lack of parentheses around the template literal), then the `where` call | ||
| * becomes a parser call of the template literal following it and thus acts as a sanitizer. | ||
| */ | ||
| class CqlInjectionConfiguration extends TaintTracking::Configuration { | ||
| CqlInjectionConfiguration() { this = "CQL injection from untrusted data" } | ||
|
|
||
| override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } | ||
|
|
||
| override predicate isSink(DataFlow::Node node) { node instanceof CqlInjectionSink } | ||
|
|
||
| override predicate isSanitizer(DataFlow::Node node) { node instanceof SqlInjection::Sanitizer } | ||
|
|
||
| override predicate isAdditionalTaintStep(DataFlow::Node start, DataFlow::Node end) { | ||
| /* | ||
| * 1. Given a call to a CQL parser, jump from the argument to the parser call itself. | ||
| */ | ||
|
|
||
| exists(CqlClauseParserCall cqlParserCall | | ||
| start = cqlParserCall.getAnArgument() and | ||
| end = cqlParserCall | ||
| ) | ||
| or | ||
| /* | ||
| * 2. Jump from a query parameter to the CQL query clause itself. e.g. Given below code: | ||
| * | ||
| * ``` javascript | ||
| * await SELECT.from(Service1Entity).where("ID=" + id); | ||
| * ``` | ||
| * | ||
| * This step jumps from `id` in the call to `where` to the entire SELECT clause. | ||
| */ | ||
|
|
||
| exists(CqlClause cqlClause | | ||
| start = cqlClause.getArgument().flow().getAPredecessor*().(StringOps::Concatenation) and | ||
| end = cqlClause.flow() | ||
| ) | ||
| or | ||
| /* | ||
| * 3. In case of INSERT and UPSERT, jump from an object write to a query parameter to the argument itself. | ||
| * e.g. Given below code: | ||
| * | ||
| * ``` javascript | ||
| * await INSERT.into(Service1Entity).entries({ id: "" + id }); | ||
| * ``` | ||
| * | ||
| * This step jumps from `id` in the property value expression to the enclosing object `{ id: "" + id }`. | ||
| * This in conjunction with the above step 2 will make the taint tracker jump from `id` to the entire | ||
| * INSERT clause. | ||
| */ | ||
lcartey marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| exists(CqlClause cqlClause, PropWrite propWrite | | ||
| (cqlClause instanceof CqlInsertClause or cqlClause instanceof CqlUpsertClause) and | ||
| cqlClause.getArgument().flow() = propWrite.getBase() and | ||
| start = propWrite.getRhs().getAPredecessor*().(StringOps::Concatenation) and | ||
| end = cqlClause.flow() | ||
| ) | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.