Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .codeql-version
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
v2.27.3
2 changes: 1 addition & 1 deletion .github/workflows/javascript.sarif.expected

Large diffs are not rendered by default.

27 changes: 27 additions & 0 deletions .github/workflows/ql-unit-tests-linux.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,33 @@ jobs:
run: |
qlt test run get-matrix --os-version ubuntu-latest

custom-bundle-regression:
name: Validate JavaScript custom bundle
runs-on: ubuntu-latest
timeout-minutes: 10
steps:
- name: Checkout repository
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1

- name: Install QLT
uses: advanced-security/codeql-development-toolkit/.github/actions/install-qlt@177984f7fc369b131c844b3a50d8d1e9a8e5223b # main
with:
qlt-version: "latest"
add-to-path: true

- name: Install CodeQL
shell: bash
run: qlt codeql run install

- name: Install JavaScript pack dependencies
shell: bash
run: |
"$QLT_CODEQL_PATH" pack install javascript/frameworks/cap/lib

- name: Validate JavaScript custom bundle
shell: bash
run: CODEQL_BIN="$QLT_CODEQL_PATH" ./scripts/test-custom-bundle.sh

run-test-suites:
name: Run Unit Tests
needs: create-unit-test-matrix
Expand Down
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,14 @@ NOTES:
- Running the above command multiple times with the same database directory will result in an error. The `--overwrite` command-line option can be used to avoid this error.
- See `codeql database create -h -v` for verbose command help.

## Model regression tests

Focused positive/negative fixtures cover [CAP service variants and events](javascript/frameworks/cap/test/models/cds/servicevariants),
[UI5 XML model constructors](javascript/frameworks/ui5/test/models/xml_model), and
[XSJS response headers and XSS flow](javascript/frameworks/xsjs/test/models/headers).
Prepare CDS fixtures with `extractors/cds/tools/workflow/cds-compilation-for-actions.sh`,
then use `codeql test run` with the XML/JSON extraction settings above.

## License

This project is licensed under the terms of the MIT open source license. Please refer to [MIT](LICENSE.txt) for the full terms.
Expand Down
43 changes: 43 additions & 0 deletions javascript/frameworks/cap/ext/cql.model.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
extensions:
- addsTo:
pack: codeql/javascript-all
extensible: typeModel
data:
- ["CapCqlSelect", "global", "Member[SELECT]"]
- ["CapCqlSelect", "@sap/cds", "Member[ql].Member[SELECT]"]
- ["CapCqlSelect", "CapCqlSelect", "Member[one,distinct]"]
- ["CapCqlSelect", "CapCqlSelect", "ReturnValue"]
- ["CapCqlSelectQuery", "CapCqlSelect", "Member[from].ReturnValue"]
- ["CapCqlSelectQuery", "CapCqlSelectQuery", "Member[alias,columns,forShareLock,forUpdate,groupBy,having,hints,limit,orderBy,where].ReturnValue"]
- ["CapCqlInto", "global", "Member[INSERT,UPSERT]"]
- ["CapCqlInto", "@sap/cds", "Member[ql].Member[INSERT,UPSERT]"]
- ["CapCqlInto", "CapCqlInto", "ReturnValue"]
- ["CapCqlIntoQuery", "CapCqlInto", "Member[into].ReturnValue"]
- ["CapCqlIntoQuery", "CapCqlIntoQuery", "Member[as,columns,entries,from,rows,values].ReturnValue"]
- ["CapCqlUpdate", "global", "Member[UPDATE]"]
- ["CapCqlUpdate", "@sap/cds", "Member[ql].Member[UPDATE]"]
- ["CapCqlUpdateQuery", "CapCqlUpdate", "ReturnValue"]
- ["CapCqlUpdateQuery", "CapCqlUpdate", "Member[entity].ReturnValue"]
- ["CapCqlUpdateQuery", "CapCqlUpdateQuery", "Member[set,where,with].ReturnValue"]
- ["CapCqlDelete", "global", "Member[DELETE]"]
- ["CapCqlDelete", "@sap/cds", "Member[ql].Member[DELETE]"]
- ["CapCqlDeleteQuery", "CapCqlDelete", "Member[from].ReturnValue"]
- ["CapCqlDeleteQuery", "CapCqlDeleteQuery", "Member[where].ReturnValue"]

- addsTo:
pack: codeql/javascript-all
extensible: sinkModel
data:
- ["CapCqlSelect", "Argument[0..]", "cap-cql-structure"]
- ["CapCqlSelect", "Member[from].Argument[0,2]", "cap-cql-structure"]
- ["CapCqlSelectQuery", "Member[columns,groupBy,orderBy].Argument[0..]", "cap-cql-structure"]
- ["CapCqlSelectQuery", "Member[having,where].Argument[0]", "cap-cql-structure"]
- ["CapCqlSelectQuery", "Member[alias].Argument[0]", "cap-cql-structure"]
- ["CapCqlSelectQuery", "Member[hints].Argument[0..]", "cap-cql-structure"]
- ["CapCqlInto", "Member[into].Argument[0]", "cap-cql-structure"]
- ["CapCqlIntoQuery", "Member[columns].Argument[0..]", "cap-cql-structure"]
- ["CapCqlUpdate", "Argument[0]", "cap-cql-structure"]
- ["CapCqlUpdate", "Member[entity].Argument[0]", "cap-cql-structure"]
- ["CapCqlUpdateQuery", "Member[set,where,with].Argument[0]", "cap-cql-structure"]
- ["CapCqlDelete", "Member[from].Argument[0]", "cap-cql-structure"]
- ["CapCqlDeleteQuery", "Member[where].Argument[0]", "cap-cql-structure"]
2 changes: 2 additions & 0 deletions javascript/frameworks/cap/ext/qlpack.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@ name: advanced-security/javascript-sap-cap-models
version: 2.27.3
extensionTargets:
codeql/javascript-all: "^2.6.24"
dataExtensions:
- "*.model.yml"
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ abstract class CqlInjectionSink extends DataFlow::Node {
* accurate reporting.
*/
abstract DataFlow::Node getQuery();

string getMessage() { result = "This CQL query contains a string concatenation with a $@." }
}

/**
* A CQL clause parameterized with a string concatentation expression.
* A CQL clause parameterized with a string concatenation expression.
*/
class CqlClauseWithStringConcatParameter instanceof CqlClause {
CqlClauseWithStringConcatParameter() {
Expand All @@ -28,6 +30,67 @@ class CqlClauseWithStringConcatParameter instanceof CqlClause {
string toString() { result = super.toString() }
}

/** Holds if existing sinks cover the concatenated argument in `await SELECT.from("A" + input)`. */
private predicate hasExistingStringConcatSink(DataFlow::Node queryParameter) {
exists(CqlClause cqlClause |
queryParameter = cqlClause.getArgument().flow() and
exists(StringConcatenation::getAnOperand(queryParameter)) and
(
exists(AwaitExpr await | await.getOperand() = cqlClause.getFinalClause().asExpr())
or
exists(CqlRunMethodCall run |
run.getArgument(0).getALocalSource() = cqlClause.getFinalClause().flow()
)
)
)
}

private predicate isSimpleObjectClause(DataFlow::Node queryParameter) {
exists(MethodCallExpr call, DataFlow::ObjectLiteralNode object |
queryParameter = call.getAnArgument().flow() and
call.getMethodName() = ["having", "set", "where", "with"] and
queryParameter.getALocalSource() = object and
not exists(object.getAComputedPropertyName()) and
not exists(object.getASpreadProperty()) and
not exists(DataFlow::PropWrite write |
write = object.getAPropertyWrite() and
write.getRhs().getALocalSource() instanceof DataFlow::ObjectLiteralNode
)
)
}

private DataFlow::Node getModeledCqlQuery(DataFlow::Node queryParameter) {
exists(CqlClause cqlClause |
queryParameter = cqlClause.getArgument().flow() and
result = cqlClause.getFinalClause().flow()
)
or
exists(CallExpr call |
not exists(CqlClause cqlClause | queryParameter = cqlClause.getArgument().flow()) and
queryParameter = call.getAnArgument().flow() and
result = call.flow()
)
}

/** A modeled structural argument, such as `SELECT.from(entity).orderBy(ordering)`. */
class ModeledCqlStructureParameter extends CqlInjectionSink {
DataFlow::Node query;

ModeledCqlStructureParameter() {
this = ModelOutput::getASinkNode("cap-cql-structure").asSink() and
query = getModeledCqlQuery(this) and
not isSimpleObjectClause(this) and
not hasExistingStringConcatSink(this)
}

override DataFlow::Node getQuery() { result = query }

override string getMessage() {
result =
"This CQL query uses a $@ as query structure, which may allow injection of identifiers or expressions."
}
}

/**
* An await expression that has as its operand a CQL clause that includes a
* string concatenation operation.
Expand Down Expand Up @@ -150,9 +213,12 @@ class CqlClauseParserCallWithStringConcat instanceof CqlClauseParserCall {
* 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
* CQL injection can happen when a caller-controlled value is passed to a modeled
* structural clause such as `columns`, `groupBy`, or `orderBy`.
*
* It can also happen if a fluent API builder (`SELECT`, `INSERT`, ...) or a
* shortcut method call (`srv.read`, `srv.create`, ...) is called with a string
* concatenation as one of its arguments, which in practice can take one of the
* following forms:
*
* 1. Concatentation with a string value with the `+` operator:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ class CqlQueryBase extends VarRef {
}
}

private predicate cqlQueryBaseName(Expr expression, string name) {
expression = any(CqlQueryBase base) and
name = expression.(CqlQueryBase).getName()
or
name in ["SELECT", "INSERT", "DELETE", "UPDATE", "UPSERT"] and
exists(CdsFacade cds |
cds.getMember("ql").getMember(name).getAValueReachableFromSource().asExpr() = expression
)
}

class CqlSelectBase extends CqlQueryBase {
CqlSelectBase() { this.getName() = "SELECT" }
}
Expand Down Expand Up @@ -71,6 +81,8 @@ class CqlUpsertBaseCall extends CqlQueryBaseCall {
Expr getRootReceiver(Expr e) {
result = e and
(
exists(string name | cqlQueryBaseName(e, name))
or
e instanceof VarRef
or
e instanceof CallExpr and not exists(e.(CallExpr).getReceiver())
Expand All @@ -91,11 +103,11 @@ Expr getRootReceiver(Expr e) {
*/
private newtype TCqlClause =
TTaggedTemplate(TaggedTemplateExpr taggedTemplateExpr) {
exists(CqlQueryBase base | base = getRootReceiver(taggedTemplateExpr)) or
exists(string name | cqlQueryBaseName(getRootReceiver(taggedTemplateExpr), name)) or
exists(CqlQueryBaseCall call | call = getRootReceiver(taggedTemplateExpr))
} or
TMethodCall(MethodCallExpr callExpr) {
exists(CqlQueryBase base | base = getRootReceiver(callExpr)) or
exists(string name | cqlQueryBaseName(getRootReceiver(callExpr), name)) or
exists(CqlQueryBaseCall call | call = getRootReceiver(callExpr))
} or
TShortcutCall(CqlQueryBaseCall callExpr)
Expand Down Expand Up @@ -158,9 +170,9 @@ class CqlClause extends TCqlClause {
}

string getTypeString() {
result = getRootReceiver(this.asTaggedTemplate()).(VarRef).getName() or
cqlQueryBaseName(getRootReceiver(this.asTaggedTemplate()), result) or
result = getRootReceiver(this.asTaggedTemplate()).(CallExpr).getCalleeName() or
result = getRootReceiver(this.asMethodCall()).(VarRef).getName() or
cqlQueryBaseName(getRootReceiver(this.asMethodCall()), result) or
result = getRootReceiver(this.asMethodCall()).(CallExpr).getCalleeName()
}

Expand Down Expand Up @@ -324,7 +336,7 @@ class CqlSelectClause extends CqlClause {
CqlSelectClause() { this.isSelect() }

override CqlSelectClause getEntityAccessingClause() {
result = this.getADescendantCqlClause() and
(result = this or result = this.getADescendantCqlClause()) and
result.asDotExpr().getPropertyName() = "from"
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,9 @@
* be included in the out-of-the-box security queries.
*/

/* We import under alias to avoid any potential naming conflicts */
import advanced_security.javascript.frameworks.cap.RemoteFlowSources as CAPRemoteFlowSources
/*
* Keep this import private because this module is injected into `javascript.qll`.
* Re-exporting it from there breaks recursive standard-library module resolution.
*/

private import advanced_security.javascript.frameworks.cap.RemoteFlowSources
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: majorAnalysis
---

* Improved `js/cap-sql-injection` to detect user-controlled structural arguments in CAP CQL queries.
* Improved `js/cap-sensitive-log` to track sensitive CAP query results while respecting explicit column projections.
18 changes: 15 additions & 3 deletions javascript/frameworks/cap/src/cqlinjection/CqlInjection.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,28 @@ If a database query is built from user-provided data without sufficient sanitiza
CAP's intrinsic data querying engine is immune with regards to SQL injections that are introduced by query parameter values that are derived from malicious user input. CQL statements are transformed into prepared statements that are executed in SQL databases such as SAP HANA.
Injections are still possible even via CQL when the query structure (e.g. target entity, columns etc.) is based on user input.

Pass user-controlled values separately from query text, using tagged templates or fixed-key object predicates where supported.

## Examples

This CAP application uses user submitted input as entity and column in a CQL query without any validation.
This CAP application uses user submitted input as structural parts of a CQL
query without any validation.

``` javascript
const entity = <from user input>
const column = <from user input>
SELECT.from(entity).columns(column)
const columns = <from user input>
const condition = <from user input>
const assignments = <from user input>
const groupBy = <from user input>
const orderBy = <from user input>
SELECT.from(entity).columns(columns).where(condition).groupBy(groupBy).orderBy(orderBy)
UPDATE(entity).set(assignments).where(condition)
```

Validate structural query inputs against fixed allow-lists.

For query values, use parameter binding rather than the allow-list validation needed for structural inputs. For example, `` .where`ID = ${id}` `` binds `id`, whereas `` .where(`ID = ${id}`) `` inserts it into the CQL text. Parenthesized calls such as `.where({ ID: id })` can still bind values safely.

## References

- OWASP: [SQL Injection](https://owasp.org/www-community/attacks/SQL_Injectionn).
Expand Down
3 changes: 1 addition & 2 deletions javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,4 @@ import CqlInjectionConfigurationFlow::PathGraph
from CqlInjectionConfigurationFlow::PathNode source, CqlInjectionConfigurationFlow::PathNode sink
where CqlInjectionConfigurationFlow::flowPath(source, sink)
select sink.getNode().(CqlInjectionSink).getQuery(), source, sink,
"This CQL query contains a string concatenation with a $@.", source.getNode(),
"user-provided value"
sink.getNode().(CqlInjectionSink).getMessage(), source.getNode(), "user-provided value"
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ CAP applications should not log sensitive information. Check CDS declarations fo

## Examples

This CAP service directly logs the sensitive information.
This CAP service directly logs a record containing sensitive information and
one of its annotated fields.

```cds
namespace advanced_security.log_exposure.sample_entities;
Expand All @@ -35,7 +36,11 @@ const { Sample } = cds.entities('advanced_security.log_exposure.sample_entities'

class SampleVulnService extends cds.ApplicationService {
init() {
LOG.info("Received: ", Sample.name); // CAP log exposure alert
this.on("lookup", async () => {
const sample = await SELECT.one.from(Sample)
LOG.info("Received: ", sample)
console.log(sample.name)
})
}
}
```
Expand Down
Loading
Loading