From ac0265e76187d2a54f1ed5ae357f20e1a6dbaf0c Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 17 Jun 2025 12:57:45 -0400 Subject: [PATCH 01/29] Minor readability --- .../cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index 75af9f1b..1db6bfaa 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -739,5 +739,5 @@ class HandlerParameterData instanceof PropRead { ) } - string toString() { result = this.(PropRead).toString() } + string toString() { result = PropRead.super.toString() } } From 6fd6b2a67c252df8621169122e15280c71e129fc Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 17 Jun 2025 13:20:27 -0400 Subject: [PATCH 02/29] Make definition of `ImplMethodCallApplicationServiceDefinition` more idiomatic --- .../lib/advanced_security/javascript/frameworks/cap/CDS.qll | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index 1db6bfaa..8c65d2f5 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -494,10 +494,7 @@ class ImplMethodCallApplicationServiceDefinition extends MethodCallNode, UserDefinedApplicationService { ImplMethodCallApplicationServiceDefinition() { - exists(CdsFacade cds | - this.getReceiver() = cds.getMember("service").asSource() and - this.getMethodName() = "impl" - ) + exists(CdsFacade cds | this = cds.getMember("service").getMember("impl").getACall()) } override FunctionNode getInitFunction() { result = this.getArgument(0) } From 439ad431318d5d429d8cbf4601a05f1e6cf578ae Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 17 Jun 2025 14:27:32 -0400 Subject: [PATCH 03/29] Add `srv.entities` as part of `EntityEntry` --- .../javascript/frameworks/cap/CDS.qll | 46 +++++++++++++++---- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index 8c65d2f5..83efff5a 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -20,29 +20,57 @@ class CdsFacade extends API::Node { /** * A call to `entities` on a CDS facade. */ -class CdsEntitiesCall extends API::Node { - CdsEntitiesCall() { exists(CdsFacade cds | this = cds.getMember("entities")) } +class CdsEntitiesCall extends DataFlow::CallNode { + CdsEntitiesCall() { exists(CdsFacade cds | this = cds.getMember("entities").getACall()) } + + /** + * Gets the namespace that this entity belongs to. + */ + string getNamespace() { + result = this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() + } } /** - * An entity instance obtained by the entity's namespace, - * via `cds.entities` + * An entity object that belongs to a service. + * * ```javascript * // Obtained through `cds.entities` * const { Service1 } = cds.entities("sample.application.namespace"); + * // Obtained through `Service.entities`, in this case the `Service` + * // being a `this` variable of the service. + * const { Service1 } = this.entities; * ``` */ -class EntityEntry extends DataFlow::CallNode { - EntityEntry() { exists(CdsEntitiesCall c | c.getACall() = this) } +abstract class EntityEntry instanceof PropRead { + Location getLocation() { result = PropRead.super.getLocation() } + + string toString() { result = PropRead.super.toString() } /** - * Gets the namespace that this entity belongs to. + * Gets the definition of the service to which this entity belongs to. */ - string getNamespace() { - result = this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() + abstract UserDefinedApplicationService getServiceDefinition(); +} + +private class EntityEntryFromCdsEntities extends EntityEntry { + CdsEntitiesCall cdsEntities; + + EntityEntryFromCdsEntities() { this = cdsEntities.getAPropertyRead() } + + override UserDefinedApplicationService getServiceDefinition() { + result.getServiceName() = cdsEntities.getNamespace() } } +private class EntityEntryFromServiceInstance extends EntityEntry { + ServiceInstance srv; + + EntityEntryFromServiceInstance() { this = srv.getAPropertyRead("entities").getAPropertyRead() } + + override UserDefinedApplicationService getServiceDefinition() { result = srv.getDefinition() } +} + /** * A call to `serve` on a CDS facade. */ From 3b7d741eccae158ea79fcfedda9eae087b81abfc Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 17 Jun 2025 14:29:13 -0400 Subject: [PATCH 04/29] Put CQL query-relevant definitions in a query library --- .../frameworks/cap/CAPCqlInjectionQuery.qll | 80 +++++++++++++++++++ .../javascript/frameworks/cap/CQL.qll | 49 ------------ .../cap/src/cqlinjection/CqlInjection.ql | 31 +------ 3 files changed, 81 insertions(+), 79 deletions(-) create mode 100644 javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll new file mode 100644 index 00000000..f039f316 --- /dev/null +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll @@ -0,0 +1,80 @@ +import javascript +import semmle.javascript.security.dataflow.SqlInjectionCustomizations +import advanced_security.javascript.frameworks.cap.CQL +import advanced_security.javascript.frameworks.cap.RemoteFlowSources + +/** + * A possibly tainted clause + * any clause with a string concatenation in it + * regardless of where that operand came from + */ +class TaintedClause instanceof CqlClause { + TaintedClause() { exists(StringConcatenation::getAnOperand(this.getArgument().flow())) } + + string toString() { result = super.toString() } + + Expr getArgument() { result = super.getArgument() } + + Expr asExpr() { result = super.asExpr() } +} + +/** + * Call to`cds.db.run` + * or + * an await surrounding a sql statement + */ +class CQLSink extends DataFlow::Node { + CQLSink() { + this = any(CdsFacade cds).getMember("db").getMember("run").getACall().getAnArgument() + or + exists(AwaitExpr a, CqlClause clause | + a.getAChildExpr() = clause.asExpr() and this.asExpr() = clause.asExpr() + ) + } +} + +/** + * a more heurisitic based taint step + * captures one of the alternative ways to construct query strings: + * `cds.parse.cql(`string`+userInput)` + * and considers them tainted if they've been concatenated against + * in any manner + */ +class ParseCQLTaintedClause extends CallNode { + ParseCQLTaintedClause() { + this = any(CdsFacade cds).getMember("parse").getMember("cql").getACall() and + exists(DataFlow::Node n | + n = StringConcatenation::getAnOperand(this.getAnArgument()) and + //omit the fact that the arg of cds.parse.cql (`SELECT * from Foo`) + //is technically a string concat + not n.asExpr() instanceof TemplateElement + ) + } +} + +class CqlIConfiguration extends TaintTracking::Configuration { + CqlIConfiguration() { this = "CqlInjection" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof CQLSink } + + override predicate isSanitizer(DataFlow::Node node) { + super.isSanitizer(node) or + node instanceof SqlInjection::Sanitizer + } + + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + //string concatenation in a clause arg taints the clause + exists(TaintedClause clause | + clause.getArgument() = pred.asExpr() and + clause.asExpr() = succ.asExpr() + ) + or + //less precise, any concat in the alternative sql stmt construction techniques + exists(ParseCQLTaintedClause parse | + parse.getAnArgument() = pred and + parse = succ + ) + } +} diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll index 5b4c39b9..93a9ec77 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll @@ -350,52 +350,3 @@ class CqlDeleteClause extends CqlClause { result.asDotExpr().getPropertyName() = "from" } } - -/** - * A possibly tainted clause - * any clause with a string concatenation in it - * regardless of where that operand came from - */ -class TaintedClause instanceof CqlClause { - TaintedClause() { exists(StringConcatenation::getAnOperand(this.getArgument().flow())) } - - string toString() { result = super.toString() } - - Expr getArgument() { result = super.getArgument() } - - Expr asExpr() { result = super.asExpr() } -} - -/** - * Call to`cds.db.run` - * or - * an await surrounding a sql statement - */ -class CQLSink extends DataFlow::Node { - CQLSink() { - this = any(CdsFacade cds).getMember("db").getMember("run").getACall().getAnArgument() - or - exists(AwaitExpr a, CqlClause clause | - a.getAChildExpr() = clause.asExpr() and this.asExpr() = clause.asExpr() - ) - } -} - -/** - * a more heurisitic based taint step - * captures one of the alternative ways to construct query strings: - * `cds.parse.cql(`string`+userInput)` - * and considers them tainted if they've been concatenated against - * in any manner - */ -class ParseCQLTaintedClause extends CallNode { - ParseCQLTaintedClause() { - this = any(CdsFacade cds).getMember("parse").getMember("cql").getACall() and - exists(DataFlow::Node n | - n = StringConcatenation::getAnOperand(this.getAnArgument()) and - //omit the fact that the arg of cds.parse.cql (`SELECT * from Foo`) - //is technically a string concat - not n.asExpr() instanceof TemplateElement - ) - } -} diff --git a/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql b/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql index 291dfec9..dc97cf91 100644 --- a/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql +++ b/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql @@ -12,36 +12,7 @@ import javascript import DataFlow::PathGraph -import semmle.javascript.security.dataflow.SqlInjectionCustomizations::SqlInjection -import advanced_security.javascript.frameworks.cap.CQL -import advanced_security.javascript.frameworks.cap.RemoteFlowSources - -class CqlIConfiguration extends TaintTracking::Configuration { - CqlIConfiguration() { this = "CqlInjection" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof CQLSink } - - override predicate isSanitizer(DataFlow::Node node) { - super.isSanitizer(node) or - node instanceof Sanitizer - } - - override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { - //string concatenation in a clause arg taints the clause - exists(TaintedClause clause | - clause.getArgument() = pred.asExpr() and - clause.asExpr() = succ.asExpr() - ) - or - //less precise, any concat in the alternative sql stmt construction techniques - exists(ParseCQLTaintedClause parse | - parse.getAnArgument() = pred and - parse = succ - ) - } -} +import advanced_security.javascript.frameworks.cap.CAPCqlInjectionQuery from CqlIConfiguration sql, DataFlow::PathNode source, DataFlow::PathNode sink where sql.hasFlowPath(source, sink) From 440712fd4b341013b3d76894e5ae7d786cbfca2a Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Thu, 19 Jun 2025 14:53:16 -0400 Subject: [PATCH 05/29] Do some refactoring --- .../frameworks/cap/CAPCqlInjectionQuery.qll | 24 ++---- .../javascript/frameworks/cap/CDS.qll | 77 ++++++++++++++++++- .../javascript/frameworks/cap/CQL.qll | 34 ++++---- 3 files changed, 101 insertions(+), 34 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll index f039f316..21847751 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll @@ -18,21 +18,6 @@ class TaintedClause instanceof CqlClause { Expr asExpr() { result = super.asExpr() } } -/** - * Call to`cds.db.run` - * or - * an await surrounding a sql statement - */ -class CQLSink extends DataFlow::Node { - CQLSink() { - this = any(CdsFacade cds).getMember("db").getMember("run").getACall().getAnArgument() - or - exists(AwaitExpr a, CqlClause clause | - a.getAChildExpr() = clause.asExpr() and this.asExpr() = clause.asExpr() - ) - } -} - /** * a more heurisitic based taint step * captures one of the alternative ways to construct query strings: @@ -57,7 +42,14 @@ class CqlIConfiguration extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { sink instanceof CQLSink } + override predicate isSink(DataFlow::Node node) { + node = any(CdsFacade cds).getMember("db").getMember("run").getACall().getAnArgument() + or + exists(AwaitExpr awaitExpr, CqlClause clause | + node.asExpr() = clause.asExpr() and + awaitExpr.getOperand() = clause.asExpr() + ) + } override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) or diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index 83efff5a..f07ce0e2 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -7,6 +7,7 @@ import advanced_security.javascript.frameworks.cap.CQL import advanced_security.javascript.frameworks.cap.RemoteFlowSources /** + * The CDS facade that provides useful interfaces to the current CAP application. * ```js * const cds = require('@sap/cds') * ``` @@ -31,6 +32,23 @@ class CdsEntitiesCall extends DataFlow::CallNode { } } +/** + * The property `db` of on a CDS facade, often accessed as `cds.db`. + */ +class CdsDb extends SourceNode { + CdsDb() { exists(CdsFacade cds | this = cds.getMember("db").asSource()) } + + MethodCallNode getRunCall() { result = this.getAMemberCall("run") } + + MethodCallNode getCreateCall() { result = this.getAMemberCall("create") } + + MethodCallNode getUpdateCall() { result = this.getAMemberCall("update") } + + MethodCallNode getDeleteCall() { result = this.getAMemberCall("delete") } + + MethodCallNode getInsertCall() { result = this.getAMemberCall("insert") } +} + /** * An entity object that belongs to a service. * @@ -123,7 +141,8 @@ class CdsConnectToCall extends DataFlow::CallNode { } /** - * A dataflow node that represents a service. Note that its definition is a `UserDefinedApplicationService`, not a `ServiceInstance`. + * A dataflow node that represents a service. + * Note that its definition is a `UserDefinedApplicationService`, not a `ServiceInstance`. */ abstract class ServiceInstance extends SourceNode { abstract UserDefinedApplicationService getDefinition(); @@ -764,5 +783,59 @@ class HandlerParameterData instanceof PropRead { ) } - string toString() { result = PropRead.super.toString() } + string toString() { result = super.toString() } +} + +/** + * A call to a method capable of running a CQL query. This includes the following: + * - Generic query runners: `cds.run`, `cds.db.run`, `srv.run` + * - Shortcut to CQL's `READ`: `cds.read`, `cds.db.read`, `srv.read` + * - Shortcut to CQL's `CREATE`: `cds.create`, `cds.db.create`, `srv.create` + * - Shortcut to CQL's `INSERT`: `cds.insert`, `cds.db.insert`, `srv.insert` + * - Shortcut to CQL's `UPSERT`: `cds.upsert`, `cds.db.upsert`, `srv.upsert` + * - Shortcut to CQL's `UPDATE`: `cds.update`, `cds.db.update`, `srv.update` + * - Shortcut to CQL's `DELETE`: `cds.delete`, `cds.db.delete`, `srv.delete` + */ +abstract class CqlQueryRunnerCall extends MethodCallNode { + SourceNode base; + string methodName; + + CqlQueryRunnerCall() { + this = base.getAMemberCall(methodName) and + ( + /* + * 1. Method call on the CDS facade or the base database service, + * accessed as `cds.db`. + */ + + exists(CdsFacade cds | base = cds.asSource()) or + exists(CdsDb cdsDb | base = cdsDb) or + /* 2. Method call on a service instance object. */ + exists(ServiceInstance srv | base = srv) + ) + } +} + +class CqlRunMethodCall extends CqlQueryRunnerCall { + CqlRunMethodCall() { this.getMethodName() = "run" } +} + +class CqlReadMethodCall extends CqlQueryRunnerCall { + CqlReadMethodCall() { this.getMethodName() = "read" } +} + +class CqlCreateMethodCall extends CqlQueryRunnerCall { + CqlCreateMethodCall() { this.getMethodName() = "create" } +} + +class CqlUpdateMethodCall extends CqlQueryRunnerCall { + CqlUpdateMethodCall() { this.getMethodName() = "update" } +} + +class CqlDeleteMethodCall extends CqlQueryRunnerCall { + CqlDeleteMethodCall() { this.getMethodName() = "delete" } +} + +class CqlInsertMethodCall extends CqlQueryRunnerCall { + CqlInsertMethodCall() { this.getMethodName() = "insert" } } diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll index 93a9ec77..d140c5c8 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll @@ -11,12 +11,14 @@ class CqlQueryBase extends VarRef { exists(string name | this.getName() = name and name in ["SELECT", "INSERT", "DELETE", "UPDATE", "UPSERT"] and - /* Made available as a global variable */ - exists(GlobalVariable queryBase | this = queryBase.getAReference()) - or - /* Imported from `cds.ql` */ - exists(CdsFacade cds | - cds.getMember("ql").getMember(name).getAValueReachableFromSource().asExpr() = this + ( + /* Made available as a global variable */ + exists(GlobalVariable queryBase | this = queryBase.getAReference()) + or + /* Imported from `cds.ql` */ + exists(CdsFacade cds | + cds.getMember("ql").getMember(name).getAValueReachableFromSource().asExpr() = this + ) ) ) } @@ -88,22 +90,22 @@ Expr getRootReceiver(Expr e) { * provided by the module cds.ql */ private newtype TCqlClause = - TaggedTemplate(TaggedTemplateExpr taggedTemplateExpr) { + TTaggedTemplate(TaggedTemplateExpr taggedTemplateExpr) { exists(CqlQueryBase base | base = getRootReceiver(taggedTemplateExpr)) or exists(CqlQueryBaseCall call | call = getRootReceiver(taggedTemplateExpr)) } or - MethodCall(MethodCallExpr callExpr) { + TMethodCall(MethodCallExpr callExpr) { exists(CqlQueryBase base | base = getRootReceiver(callExpr)) or exists(CqlQueryBaseCall call | call = getRootReceiver(callExpr)) } or - ShortcutCall(CqlQueryBaseCall callExpr) + TShortcutCall(CqlQueryBaseCall callExpr) class CqlClause extends TCqlClause { - TaggedTemplateExpr asTaggedTemplate() { this = TaggedTemplate(result) } + TaggedTemplateExpr asTaggedTemplate() { this = TTaggedTemplate(result) } - MethodCallExpr asMethodCall() { this = MethodCall(result) } + MethodCallExpr asMethodCall() { this = TMethodCall(result) } - CallExpr asShortcutCall() { this = ShortcutCall(result) } + CallExpr asShortcutCall() { this = TShortcutCall(result) } Node flow() { result = this.asExpr().flow() } @@ -169,8 +171,8 @@ class CqlClause extends TCqlClause { CqlQueryBase getCqlBase() { result = getRootReceiver(this.asMethodCall()) } CqlQueryBaseCall getCqlBaseCall() { - result = getRootReceiver(this.asTaggedTemplate()).(CqlQueryBaseCall) or - result = getRootReceiver(this.asMethodCall()).(CqlQueryBaseCall) + result = getRootReceiver(this.asTaggedTemplate()) or + result = getRootReceiver(this.asMethodCall()) } /** Describes a parent expression relation */ @@ -184,9 +186,9 @@ class CqlClause extends TCqlClause { * Possible cases for constructing a chain of clauses: * * (looking at the terminal clause and its possible parent types as tuples: (this, parent)) - * 1. MethodCall.MethodCall + * 1. TMethodCall.TMethodCall * - example `(SELECT.from(Table), SELECT.from(Table).where("col1='*'"))` - * 2. ShortcutCall.MethodCall + * 2. TShortcutCall.TMethodCall * - example `(SELECT("col1, col2"), SELECT("col1, col2").from("Table"))` * * Note that ShortcutCalls cannot be added to any clause chain other than the first position, e.g. From dfa8c08ab8b422e83bea22da0e7bd046c89d0f54 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Thu, 19 Jun 2025 16:38:20 -0400 Subject: [PATCH 06/29] Sort out EntityReference --- .../javascript/frameworks/cap/CDS.qll | 124 +++++++++--------- .../javascript/frameworks/cap/CQL.qll | 8 +- 2 files changed, 70 insertions(+), 62 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index f07ce0e2..4329ef5a 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -49,46 +49,6 @@ class CdsDb extends SourceNode { MethodCallNode getInsertCall() { result = this.getAMemberCall("insert") } } -/** - * An entity object that belongs to a service. - * - * ```javascript - * // Obtained through `cds.entities` - * const { Service1 } = cds.entities("sample.application.namespace"); - * // Obtained through `Service.entities`, in this case the `Service` - * // being a `this` variable of the service. - * const { Service1 } = this.entities; - * ``` - */ -abstract class EntityEntry instanceof PropRead { - Location getLocation() { result = PropRead.super.getLocation() } - - string toString() { result = PropRead.super.toString() } - - /** - * Gets the definition of the service to which this entity belongs to. - */ - abstract UserDefinedApplicationService getServiceDefinition(); -} - -private class EntityEntryFromCdsEntities extends EntityEntry { - CdsEntitiesCall cdsEntities; - - EntityEntryFromCdsEntities() { this = cdsEntities.getAPropertyRead() } - - override UserDefinedApplicationService getServiceDefinition() { - result.getServiceName() = cdsEntities.getNamespace() - } -} - -private class EntityEntryFromServiceInstance extends EntityEntry { - ServiceInstance srv; - - EntityEntryFromServiceInstance() { this = srv.getAPropertyRead("entities").getAPropertyRead() } - - override UserDefinedApplicationService getServiceDefinition() { result = srv.getDefinition() } -} - /** * A call to `serve` on a CDS facade. */ @@ -645,38 +605,87 @@ class CdsTransaction extends MethodCallNode { abstract class CdsReference extends DataFlow::Node { } +/** + * A reference object to an entity that belongs to a service. e.g. + * + * ```javascript + * // 1. Obtained through `cds.entities` + * const { Entity1 } = cds.entities("sample.application.namespace"); + * // 2. Obtained through `Service.entities`, in this case the `Service` + * // being a `this` variable of the service. + * const { Entity2 } = this.entities; + * // 3. A direct mention of a name in a literal pass to the fluent API builder. + * SELECT.from`Books`.where(`ID=${id}`) + * ``` + */ abstract class EntityReference extends CdsReference { abstract CdlEntity getCqlDefinition(); } +/** + * A reference object to an entity that belongs to a service, either + * obtained through a method call to `entities`, or a read from property + * `entities`. e.g. + * + * ```javascript + * // 1. Obtained through `cds.entities` + * const { Entity1 } = cds.entities("sample.application.namespace"); + * // 2. Obtained through `Service.entities`, in this case the `Service` + * // being a `this` variable of the service. + * const { Entity2 } = this.entities; + * // 3. A direct mention of a name in a literal pass to the fluent API builder. + * SELECT.from`Books`.where(`ID=${id}`) + * ``` + */ class EntityReferenceFromEntities extends EntityReference instanceof PropRead { - DataFlow::SourceNode entities; + /** + * Property or call to entities + */ + DataFlow::SourceNode entitiesAccess; + /** + * Receiver of the entities call or the base of propread + */ DataFlow::Node receiver; + /** + * Name of the (unqualified) entity being accessed + */ string entityName; EntityReferenceFromEntities() { + /* + * 1. Reference obtained through a call to `entities` on the + * service instance. + */ + exists(MethodCallNode entitiesCall | - entities = entitiesCall and + entitiesAccess = entitiesCall and receiver = entitiesCall.getReceiver() and entitiesCall.getMethodName() = "entities" and this = entitiesCall.getAPropertyRead(entityName) ) or + /* + * 2. Reference obtained through a read from property `entities` of the + * service instance. + */ + exists(PropRead entitiesRead | - entities = entitiesRead and + entitiesAccess = entitiesRead and receiver = entitiesRead.getBase() and entitiesRead.getPropertyName() = "entities" and this = entitiesRead.getAPropertyRead(entityName) ) } - DataFlow::SourceNode getEntities() { result = entities } + DataFlow::SourceNode getEntities() { result = entitiesAccess } DataFlow::Node getReceiver() { result = receiver } string getEntityName() { result = entityName } abstract override CdlEntity getCqlDefinition(); + + abstract UserDefinedApplicationService getServiceDefinition(); } /** @@ -686,13 +695,7 @@ class EntityReferenceFromUserDefinedServiceEntities extends EntityReferenceFromE { ServiceInstance service; - EntityReferenceFromUserDefinedServiceEntities() { - this.getReceiver() = service.(ServiceInstanceFromThisNode) - or - this.getEntities() = service.(ServiceInstanceFromCdsConnectTo).getAMemberCall("entities") - or - this.getEntities() = service.(ServiceInstanceFromCdsConnectTo).getAPropertyRead("entities") - } + EntityReferenceFromUserDefinedServiceEntities() { this.getReceiver().getALocalSource() = service } override CdlEntity getCqlDefinition() { this.getEntities() instanceof PropRead and @@ -709,6 +712,8 @@ class EntityReferenceFromUserDefinedServiceEntities extends EntityReferenceFromE .getEntity(this.getEntities().(MethodCallNode).getArgument(0).getStringValue() + "." + entityName) } + + override UserDefinedApplicationService getServiceDefinition() { result = service.getDefinition() } } /** @@ -716,14 +721,9 @@ class EntityReferenceFromUserDefinedServiceEntities extends EntityReferenceFromE */ class EntityReferenceFromDbOrCdsEntities extends EntityReferenceFromEntities { EntityReferenceFromDbOrCdsEntities() { - exists(DBServiceInstanceFromCdsConnectTo db | - entities = db.getAMemberCall("entities") or entities = db.getAPropertyRead("entities") - ) - or - exists(CdsFacade cds | - entities = cds.getMember("entities").getACall() or - entities = cds.getMember("entities").asSource() - ) + this.getReceiver().getALocalSource() instanceof DBServiceInstanceFromCdsConnectTo or + exists(CdsFacade cds | this.getReceiver().getALocalSource() = cds.getNode()) or + exists(CdsFacade cds | this.getReceiver().getALocalSource() = cds.getMember("db").asSource()) } override CdlEntity getCqlDefinition() { @@ -731,6 +731,12 @@ class EntityReferenceFromDbOrCdsEntities extends EntityReferenceFromEntities { result.getName() = this.getEntities().(MethodCallNode).getArgument(0).getStringValue() + "." + entityName } + + override UserDefinedApplicationService getServiceDefinition() { + /* TODO: Always get the DB service definition. */ + none() + // result.getServiceName() = this.(CdsEntitiesCall).getNamespace() + } } class EntityReferenceFromCqlClause extends EntityReference, ExprNode { diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll index d140c5c8..8b9134c1 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll @@ -299,9 +299,11 @@ class CqlClause extends TCqlClause { result = this.getRunner().getDefinition().getCdsDeclaration().getAnEntity() or /* 2. Variable whose value is a reference to an entity */ - exists(ExprNode entityReference | entityReference = this.getAccessingEntityReference() | - result = entityReference.getALocalSource().(EntityReferenceFromEntities).getCqlDefinition() - ) + result = + this.getAccessingEntityReference() + .getALocalSource() + .(EntityReferenceFromEntities) + .getCqlDefinition() } } From 571b316dcd4ee020501e72ec813fb57b6b31bba3 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 20 Jun 2025 14:27:58 -0400 Subject: [PATCH 07/29] Create a new CQL injection test project and move the old one to a folder --- .../test/queries/cqlinjection/db/schema.cds | 11 ++ .../{ => old}/cqlinjection.expected | 0 .../cqlinjection/{ => old}/cqlinjection.js | 0 .../cqlinjection/{ => old}/cqlinjection.qlref | 0 .../test/queries/cqlinjection/package.json | 23 +++ .../cap/test/queries/cqlinjection/server.js | 4 + .../queries/cqlinjection/srv/service1.cds | 13 ++ .../test/queries/cqlinjection/srv/service1.js | 138 ++++++++++++++++++ .../queries/cqlinjection/srv/service2.cds | 13 ++ .../test/queries/cqlinjection/srv/service2.js | 11 ++ 10 files changed, 213 insertions(+) create mode 100644 javascript/frameworks/cap/test/queries/cqlinjection/db/schema.cds rename javascript/frameworks/cap/test/queries/cqlinjection/{ => old}/cqlinjection.expected (100%) rename javascript/frameworks/cap/test/queries/cqlinjection/{ => old}/cqlinjection.js (100%) rename javascript/frameworks/cap/test/queries/cqlinjection/{ => old}/cqlinjection.qlref (100%) create mode 100644 javascript/frameworks/cap/test/queries/cqlinjection/package.json create mode 100644 javascript/frameworks/cap/test/queries/cqlinjection/server.js create mode 100644 javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.cds create mode 100644 javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js create mode 100644 javascript/frameworks/cap/test/queries/cqlinjection/srv/service2.cds create mode 100644 javascript/frameworks/cap/test/queries/cqlinjection/srv/service2.js diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/db/schema.cds b/javascript/frameworks/cap/test/queries/cqlinjection/db/schema.cds new file mode 100644 index 00000000..1d4202fb --- /dev/null +++ b/javascript/frameworks/cap/test/queries/cqlinjection/db/schema.cds @@ -0,0 +1,11 @@ +namespace advanced_security.log_injection.sample_entities; + +entity Entity1 { + Attribute1 : String(100); + Attribute2 : String(100) +} + +entity Entity2 { + Attribute3 : String(100); + Attribute4 : String(100) +} \ No newline at end of file diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.expected b/javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.expected similarity index 100% rename from javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.expected rename to javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.expected diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.js b/javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.js similarity index 100% rename from javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.js rename to javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.js diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.qlref b/javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.qlref similarity index 100% rename from javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.qlref rename to javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.qlref diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/package.json b/javascript/frameworks/cap/test/queries/cqlinjection/package.json new file mode 100644 index 00000000..be11dbc2 --- /dev/null +++ b/javascript/frameworks/cap/test/queries/cqlinjection/package.json @@ -0,0 +1,23 @@ +{ + "name": "@advanced-security/log-injection", + "version": "1.0.0", + "dependencies": { + "@sap/cds": "^7", + "express": "^4.17.1", + "@cap-js/sqlite": "*" + }, + "scripts": { + "start": "cds-serve", + "watch": "cds watch" + }, + "cds": { + "requires": { + "service-1": { + "impl": "srv/service1.js" + }, + "service-2": { + "impl": "srv/service2.js" + } + } + } +} diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/server.js b/javascript/frameworks/cap/test/queries/cqlinjection/server.js new file mode 100644 index 00000000..b4f60916 --- /dev/null +++ b/javascript/frameworks/cap/test/queries/cqlinjection/server.js @@ -0,0 +1,4 @@ +const cds = require("@sap/cds"); +const app = require("express")(); + +cds.serve("all").in(app); diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.cds b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.cds new file mode 100644 index 00000000..9f033086 --- /dev/null +++ b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.cds @@ -0,0 +1,13 @@ +using { advanced_security.log_injection.sample_entities as db_schema } from '../db/schema'; + +/* Uncomment the line below to make the service hidden */ +// @protocol: 'none' +service Service1 @(path: '/service-1') { + /* Entity to send READ/GET about. */ + entity Service1Entity as projection on db_schema.Entity1 excluding { Attribute2 } + + /* API to talk to Service1. */ + action send1 ( + messageToPass : String + ) returns String; +} diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js new file mode 100644 index 00000000..7d6b6e64 --- /dev/null +++ b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js @@ -0,0 +1,138 @@ +const cds = require("@sap/cds"); + +module.exports = class Service1 extends cds.ApplicationService { + init() { + /* ========== Service1 running query on the database service using `cds.run` and friends using Fluent API ========== */ + this.on("send11", async (req) => { + const { id } = req.data; + const query = SELECT.from`Entity1`.where("ID=" + id); + cds.run(query); + }); + + this.on("send12", async (req) => { + const { id } = req.data; + cds.read("Entity1").where("ID =" + id); + }); + + this.on("send13", async (req) => { + const { id } = req.data; + cds.create("Entity1").entries({id: "" + id}); + }); + + this.on("send14", async (req) => { + const { id, amount } = req.data; + cds.update("Entity1").set("col1 = col1" + amount).where("col1 = " + id); + }); + + this.on("send15", async (req) => { + const { id } = req.data; + cds.upsert("Entity1").entries({id: "" + id}); + }); + + this.on("send16", async (req) => { + const { id } = req.data; + cds.delete("Entity1").where("ID =" + id); + }); + + /* ========== Service1 running query on itself by `await`-ing the query ========== */ + this.on("send21", async (req) => { + const { id } = req.data; + const { Service1Entity } = this.entities; + await SELECT.from(Service1Entity).where("ID=" + id); + }); + + this.on("send22", async (req) => { + const { id } = req.data; + const { Service1Entity } = this.entities; + await INSERT.into(Service1Entity).entries({ id: "" + id }); + }); + + this.on("send23", async (req) => { + const { id, amount } = req.data; + const { Service1Entity } = this.entities; + await UPDATE.entity(Service1Entity).set(`col1 = col1 -` + amount).where("id=" + id); + }); + + this.on("send24", async (req) => { + const { id } = req.data; + const { Service1Entity } = this.entities; + await UPSERT.into(Service1Entity).entries({ id: "" + id }); + }); + + this.on("send25", async (req) => { + const { id } = req.data; + const { Service1Entity } = this.entities; + await DELETE.from(Service1Entity).where("ID =" + id); + }); + + /* ========== Service1 running query on itself using `this.run` and friends using Fluent API ========== */ + this.on("send31", async (req) => { + const { id } = req.data; + const query = SELECT.from`Service1Entity`.where("ID=" + id); + this.run(query); + }); + + this.on("send32", async (req) => { + const { id } = req.data; + this.read(`Service1Entity`).where("ID =" + id); + }); + + this.on("send33", async (req) => { + const { id } = req.data; + this.create(`Service1Entity`).entries({id: "" + id}); + }); + + this.on("send34", async (req) => { + const { id, amount } = req.data; + this.update(`Service1Entity`).set("col1 = col1" + amount).where("col1 = " + id); + }); + + this.on("send35", async (req) => { + const { id } = req.data; + this.upsert(`Service1Entity`).entries({id: "" + id}); + }); + + this.on("send36", async (req) => { + const { id } = req.data; + this.delete(`Service1Entity`).where("ID =" + id); + }); + + /* ========== Service1 running query on Service2 using `Service2.run` and friends ========== */ + this.on("send41", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + const query = SELECT.from`Service1Entity`.where("ID=" + id); + Service2.run(query); + }); + + this.on("send42", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.read(`Service2Entity`).where("ID =" + id); + }); + + this.on("send43", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.create(`Service2Entity`).entries({id: "" + id}); + }); + + this.on("send44", async (req) => { + const { id, amount } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.update(`Service2Entity`).set("col1 = col1" + amount).where("col1 = " + id); + }); + + this.on("send45", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.upsert(`Service2Entity`).entries({id: "" + id}); + }); + + this.on("send46", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.delete(`Service2Entity`).where("ID =" + id); + }); + } +}; diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service2.cds b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service2.cds new file mode 100644 index 00000000..26a56b56 --- /dev/null +++ b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service2.cds @@ -0,0 +1,13 @@ +using { advanced_security.log_injection.sample_entities as db_schema } from '../db/schema'; + +/* Uncomment the line below to make the service hidden */ +// @protocol: 'none' +service Service2 @(path: '/service-2') { + /* Entity to send READ/GET about. */ + entity Service2Entity as projection on db_schema.Entity2 excluding { Attribute4 } + + /* API to talk to Service2. */ + action send2 ( + messageToPass: String + ) returns String; +} diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service2.js b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service2.js new file mode 100644 index 00000000..703d8f3d --- /dev/null +++ b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service2.js @@ -0,0 +1,11 @@ +const cds = require("@sap/cds"); + +module.exports = cds.service.impl(function () { + /* Log upon receiving an "send2" event. */ + this.on("send2", async (msg) => { + const { messageToPass } = msg.data; + /* Do something with the received data; customize below to individual needs. */ + const doSomething = console.log; + doSomething(messageToPass); + }); +}); From 1262260b2706cf7c71be33c3549d691d03b4a189 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 20 Jun 2025 14:28:50 -0400 Subject: [PATCH 08/29] Minor numbering --- .../cap/test/queries/cqlinjection/srv/service1.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js index 7d6b6e64..9fa72478 100644 --- a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js +++ b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js @@ -2,7 +2,7 @@ const cds = require("@sap/cds"); module.exports = class Service1 extends cds.ApplicationService { init() { - /* ========== Service1 running query on the database service using `cds.run` and friends using Fluent API ========== */ + /* ========== 1. Service1 running query on the database service using `cds.run` and friends using Fluent API ========== */ this.on("send11", async (req) => { const { id } = req.data; const query = SELECT.from`Entity1`.where("ID=" + id); @@ -34,7 +34,7 @@ module.exports = class Service1 extends cds.ApplicationService { cds.delete("Entity1").where("ID =" + id); }); - /* ========== Service1 running query on itself by `await`-ing the query ========== */ + /* ========== 2. Service1 running query on itself by `await`-ing the query ========== */ this.on("send21", async (req) => { const { id } = req.data; const { Service1Entity } = this.entities; @@ -65,7 +65,7 @@ module.exports = class Service1 extends cds.ApplicationService { await DELETE.from(Service1Entity).where("ID =" + id); }); - /* ========== Service1 running query on itself using `this.run` and friends using Fluent API ========== */ + /* ========== 3. Service1 running query on itself using `this.run` and friends using Fluent API ========== */ this.on("send31", async (req) => { const { id } = req.data; const query = SELECT.from`Service1Entity`.where("ID=" + id); @@ -97,7 +97,7 @@ module.exports = class Service1 extends cds.ApplicationService { this.delete(`Service1Entity`).where("ID =" + id); }); - /* ========== Service1 running query on Service2 using `Service2.run` and friends ========== */ + /* ========== 4. Service1 running query on Service2 using `Service2.run` and friends ========== */ this.on("send41", async (req) => { const { id } = req.data; const { Service2 } = await cds.connect.to("Service2"); From 908a572d0546fad769bd15e1d71afd0641410c32 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 20 Jun 2025 15:03:36 -0400 Subject: [PATCH 09/29] install `@sap/cds-dk` and bump version of `@sap/cds` --- .../frameworks/cap/test/queries/cqlinjection/package.json | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/package.json b/javascript/frameworks/cap/test/queries/cqlinjection/package.json index be11dbc2..9845c03b 100644 --- a/javascript/frameworks/cap/test/queries/cqlinjection/package.json +++ b/javascript/frameworks/cap/test/queries/cqlinjection/package.json @@ -2,9 +2,10 @@ "name": "@advanced-security/log-injection", "version": "1.0.0", "dependencies": { - "@sap/cds": "^7", - "express": "^4.17.1", - "@cap-js/sqlite": "*" + "@cap-js/sqlite": "*", + "@sap/cds": "^9", + "@sap/cds-dk": "^9.0.5", + "express": "^4.17.1" }, "scripts": { "start": "cds-serve", From a8a0bb44d0e88537b5a1ae29edf5423f1e69d95b Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 23 Jun 2025 14:43:57 -0400 Subject: [PATCH 10/29] Move existing CQL Injection test case to `old/` --- .../test/queries/old/cqlinjection.expected | 88 +++++++++++++++++++ .../cap/test/queries/old/cqlinjection.js | 50 +++++++++++ .../cap/test/queries/old/cqlinjection.qlref | 1 + 3 files changed, 139 insertions(+) create mode 100644 javascript/frameworks/cap/test/queries/old/cqlinjection.expected create mode 100644 javascript/frameworks/cap/test/queries/old/cqlinjection.js create mode 100644 javascript/frameworks/cap/test/queries/old/cqlinjection.qlref diff --git a/javascript/frameworks/cap/test/queries/old/cqlinjection.expected b/javascript/frameworks/cap/test/queries/old/cqlinjection.expected new file mode 100644 index 00000000..5b6e7e20 --- /dev/null +++ b/javascript/frameworks/cap/test/queries/old/cqlinjection.expected @@ -0,0 +1,88 @@ +WARNING: module 'PathGraph' has been deprecated and may be removed in future (CqlInjection.ql:14,8-27) +WARNING: type 'Configuration' has been deprecated and may be removed in future (CqlInjection.ql:19,33-61) +WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:46,29-47) +WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:46,56-74) +nodes +| cqlinjection.js:7:34:7:36 | req | +| cqlinjection.js:7:34:7:36 | req | +| cqlinjection.js:8:13:8:30 | { book, quantity } | +| cqlinjection.js:8:13:8:41 | book | +| cqlinjection.js:8:15:8:18 | book | +| cqlinjection.js:8:34:8:36 | req | +| cqlinjection.js:8:34:8:41 | req.data | +| cqlinjection.js:12:11:12:56 | query | +| cqlinjection.js:12:19:12:56 | SELECT. ... book}`) | +| cqlinjection.js:12:44:12:55 | `ID=${book}` | +| cqlinjection.js:12:50:12:53 | book | +| cqlinjection.js:13:36:13:40 | query | +| cqlinjection.js:13:36:13:40 | query | +| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | +| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | +| cqlinjection.js:15:52:15:63 | `ID=${book}` | +| cqlinjection.js:15:58:15:61 | book | +| cqlinjection.js:17:11:17:57 | query2 | +| cqlinjection.js:17:20:17:57 | SELECT. ... + book) | +| cqlinjection.js:17:45:17:56 | 'ID=' + book | +| cqlinjection.js:17:53:17:56 | book | +| cqlinjection.js:18:37:18:42 | query2 | +| cqlinjection.js:18:37:18:42 | query2 | +| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | +| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | +| cqlinjection.js:20:52:20:63 | 'ID=' + book | +| cqlinjection.js:20:60:20:63 | book | +| cqlinjection.js:27:11:27:62 | cqn | +| cqlinjection.js:27:17:27:62 | CQL`SEL ... + book | +| cqlinjection.js:27:59:27:62 | book | +| cqlinjection.js:28:39:28:41 | cqn | +| cqlinjection.js:28:39:28:41 | cqn | +| cqlinjection.js:30:11:30:60 | cqn1 | +| cqlinjection.js:30:18:30:60 | cds.par ... + book) | +| cqlinjection.js:30:32:30:59 | `SELECT ... + book | +| cqlinjection.js:30:56:30:59 | book | +| cqlinjection.js:31:39:31:42 | cqn1 | +| cqlinjection.js:31:39:31:42 | cqn1 | +edges +| cqlinjection.js:7:34:7:36 | req | cqlinjection.js:8:34:8:36 | req | +| cqlinjection.js:7:34:7:36 | req | cqlinjection.js:8:34:8:36 | req | +| cqlinjection.js:8:13:8:30 | { book, quantity } | cqlinjection.js:8:15:8:18 | book | +| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:12:50:12:53 | book | +| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:15:58:15:61 | book | +| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:17:53:17:56 | book | +| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:20:60:20:63 | book | +| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:27:59:27:62 | book | +| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:30:56:30:59 | book | +| cqlinjection.js:8:15:8:18 | book | cqlinjection.js:8:13:8:41 | book | +| cqlinjection.js:8:34:8:36 | req | cqlinjection.js:8:34:8:41 | req.data | +| cqlinjection.js:8:34:8:41 | req.data | cqlinjection.js:8:13:8:30 | { book, quantity } | +| cqlinjection.js:12:11:12:56 | query | cqlinjection.js:13:36:13:40 | query | +| cqlinjection.js:12:11:12:56 | query | cqlinjection.js:13:36:13:40 | query | +| cqlinjection.js:12:19:12:56 | SELECT. ... book}`) | cqlinjection.js:12:11:12:56 | query | +| cqlinjection.js:12:44:12:55 | `ID=${book}` | cqlinjection.js:12:19:12:56 | SELECT. ... book}`) | +| cqlinjection.js:12:50:12:53 | book | cqlinjection.js:12:44:12:55 | `ID=${book}` | +| cqlinjection.js:15:52:15:63 | `ID=${book}` | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | +| cqlinjection.js:15:52:15:63 | `ID=${book}` | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | +| cqlinjection.js:15:58:15:61 | book | cqlinjection.js:15:52:15:63 | `ID=${book}` | +| cqlinjection.js:17:11:17:57 | query2 | cqlinjection.js:18:37:18:42 | query2 | +| cqlinjection.js:17:11:17:57 | query2 | cqlinjection.js:18:37:18:42 | query2 | +| cqlinjection.js:17:20:17:57 | SELECT. ... + book) | cqlinjection.js:17:11:17:57 | query2 | +| cqlinjection.js:17:45:17:56 | 'ID=' + book | cqlinjection.js:17:20:17:57 | SELECT. ... + book) | +| cqlinjection.js:17:53:17:56 | book | cqlinjection.js:17:45:17:56 | 'ID=' + book | +| cqlinjection.js:20:52:20:63 | 'ID=' + book | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | +| cqlinjection.js:20:52:20:63 | 'ID=' + book | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | +| cqlinjection.js:20:60:20:63 | book | cqlinjection.js:20:52:20:63 | 'ID=' + book | +| cqlinjection.js:27:11:27:62 | cqn | cqlinjection.js:28:39:28:41 | cqn | +| cqlinjection.js:27:11:27:62 | cqn | cqlinjection.js:28:39:28:41 | cqn | +| cqlinjection.js:27:17:27:62 | CQL`SEL ... + book | cqlinjection.js:27:11:27:62 | cqn | +| cqlinjection.js:27:59:27:62 | book | cqlinjection.js:27:17:27:62 | CQL`SEL ... + book | +| cqlinjection.js:30:11:30:60 | cqn1 | cqlinjection.js:31:39:31:42 | cqn1 | +| cqlinjection.js:30:11:30:60 | cqn1 | cqlinjection.js:31:39:31:42 | cqn1 | +| cqlinjection.js:30:18:30:60 | cds.par ... + book) | cqlinjection.js:30:11:30:60 | cqn1 | +| cqlinjection.js:30:32:30:59 | `SELECT ... + book | cqlinjection.js:30:18:30:60 | cds.par ... + book) | +| cqlinjection.js:30:56:30:59 | book | cqlinjection.js:30:32:30:59 | `SELECT ... + book | +#select +| cqlinjection.js:13:36:13:40 | query | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:13:36:13:40 | query | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:18:37:18:42 | query2 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:18:37:18:42 | query2 | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:28:39:28:41 | cqn | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:28:39:28:41 | cqn | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:31:39:31:42 | cqn1 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:31:39:31:42 | cqn1 | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | diff --git a/javascript/frameworks/cap/test/queries/old/cqlinjection.js b/javascript/frameworks/cap/test/queries/old/cqlinjection.js new file mode 100644 index 00000000..ee16c5c9 --- /dev/null +++ b/javascript/frameworks/cap/test/queries/old/cqlinjection.js @@ -0,0 +1,50 @@ +import cds from '@sap/cds' +const { Books } = cds.entities('sap.capire.bookshop') + +class SampleVulnService extends cds.ApplicationService { + init() { + // contains a sample CQL injection + this.on('submitOrder', async req => { + const { book, quantity } = req.data + + let { stock } = await SELECT`stock`.from(Books, book) + + let query = SELECT.from`Books`.where(`ID=${book}`) + let books = await cds.db.run(query) // CQL injection alert + + let books11 = await SELECT.from`Books`.where(`ID=${book}`) // CQL injection alert + + let query2 = SELECT.from`Books`.where('ID=' + book) + let books2 = await cds.db.run(query2) // CQL injection alert + + let books22 = await SELECT.from`Books`.where('ID=' + book) // CQL injection alert + + let books3 = await SELECT.from`Books`.where`ID=${book}` //safe + + let id = 2 + let books33 = await SELECT.from`Books`.where('ID=' + id) //safe + + let cqn = CQL`SELECT col1, col2, col3 from Books` + book + let books222 = await cds.db.run(cqn) // CQL injection alert + + let cqn1 = cds.parse.cql(`SELECT * from Books` + book) + let books111 = await cds.db.run(cqn1) // CQL injection alert + + const pg = require("pg"), + pool = new pg.Pool(config); + pool.query(req.params.category, [], function (err, results) { // non-CQL injection alert from CAP source + // process results + }); + + const app = require("express")(); + app.get("search", function handler(req2, res) { + pool.query(req2.params.category, [], function (err, results) { // non-CQL injection alert from non-CAP source + // process results + }); + }); + + return super.init() + }) + } +} +export { SampleVulnService } \ No newline at end of file diff --git a/javascript/frameworks/cap/test/queries/old/cqlinjection.qlref b/javascript/frameworks/cap/test/queries/old/cqlinjection.qlref new file mode 100644 index 00000000..d6231d2a --- /dev/null +++ b/javascript/frameworks/cap/test/queries/old/cqlinjection.qlref @@ -0,0 +1 @@ +cqlinjection/CqlInjection.ql \ No newline at end of file From 5e39be5cd2566aa5cc71f67b20720e60438d626f Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 23 Jun 2025 14:44:16 -0400 Subject: [PATCH 11/29] Refine test cases and add more cases --- .../test/queries/cqlinjection/srv/service1.js | 56 +++++++++++++++++-- 1 file changed, 51 insertions(+), 5 deletions(-) diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js index 9fa72478..68d97c1f 100644 --- a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js +++ b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js @@ -23,13 +23,18 @@ module.exports = class Service1 extends cds.ApplicationService { const { id, amount } = req.data; cds.update("Entity1").set("col1 = col1" + amount).where("col1 = " + id); }); - + this.on("send15", async (req) => { const { id } = req.data; - cds.upsert("Entity1").entries({id: "" + id}); + cds.insert("Entity1").entries({id: "" + id}); }); this.on("send16", async (req) => { + const { id } = req.data; + cds.upsert("Entity1").entries({id: "" + id}); + }); + + this.on("send17", async (req) => { const { id } = req.data; cds.delete("Entity1").where("ID =" + id); }); @@ -86,13 +91,18 @@ module.exports = class Service1 extends cds.ApplicationService { const { id, amount } = req.data; this.update(`Service1Entity`).set("col1 = col1" + amount).where("col1 = " + id); }); - + this.on("send35", async (req) => { const { id } = req.data; - this.upsert(`Service1Entity`).entries({id: "" + id}); + this.insert(`Service1Entity`).entries({id: "" + id}); }); this.on("send36", async (req) => { + const { id } = req.data; + this.upsert(`Service1Entity`).entries({id: "" + id}); + }); + + this.on("send37", async (req) => { const { id } = req.data; this.delete(`Service1Entity`).where("ID =" + id); }); @@ -126,13 +136,49 @@ module.exports = class Service1 extends cds.ApplicationService { this.on("send45", async (req) => { const { id } = req.data; const { Service2 } = await cds.connect.to("Service2"); - Service2.upsert(`Service2Entity`).entries({id: "" + id}); + Service2.insert(`Service2Entity`).entries({id: "" + id}); }); this.on("send46", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.upsert(`Service2Entity`).entries({id: "" + id}); + }); + + this.on("send47", async (req) => { const { id } = req.data; const { Service2 } = await cds.connect.to("Service2"); Service2.delete(`Service2Entity`).where("ID =" + id); }); + + /* ========== 5. Service1 running query on Service2 using CQN parsed with `cds.ql` ========== */ + this.on("send5", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + const query = cds.ql("SELECT * from Service1Entity where ID =" + id); + Service2.run(query); + }); + + /* ========== 6. Service1 running query on the database service using CQN parsed with `cds.parse.cql` ========== */ + this.on("send6", async (req) => { + const { id } = req.data; + const query = cds.parse.cql(`SELECT * from Entity1 where ID =` + id); + cds.run(query); + }); + + /* ========== 7. Service1 running query on Service2 using an unparsed CDL string (only valid in old versions of CAP) ========== */ + this.on("send71", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + const query = "SELECT * from Entity1 where ID =" + id; + Service2.run(query); + }); + + this.on("send72", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + const query = `SELECT * from Entity1 where ID =` + id; + Service2.run(query); + }); } }; From 25450a856c1708d53207f8b0e78e699a1cc5ddad Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 23 Jun 2025 16:04:47 -0400 Subject: [PATCH 12/29] Finalize working draft on current CqlInjection test case --- .../frameworks/cap/CAPCqlInjectionQuery.qll | 95 +++++++++-------- .../javascript/frameworks/cap/CDS.qll | 100 +++++++++++++++--- .../javascript/frameworks/cap/CQL.qll | 35 ++++++ .../cap/src/cqlinjection/CqlInjection.ql | 2 +- 4 files changed, 170 insertions(+), 62 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll index 21847751..38d7d9e5 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll @@ -2,71 +2,78 @@ 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 -/** - * A possibly tainted clause - * any clause with a string concatenation in it - * regardless of where that operand came from - */ -class TaintedClause instanceof CqlClause { - TaintedClause() { exists(StringConcatenation::getAnOperand(this.getArgument().flow())) } +class CqlClauseWithStringConcatParameter instanceof CqlClause { + CqlClauseWithStringConcatParameter() { + exists(DataFlow::Node queryParameter | + ( + if this instanceof CqlInsertClause or this instanceof CqlUpsertClause + then + queryParameter = this.getArgument().flow() or + 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() } +} - Expr getArgument() { result = super.getArgument() } +class CqlShortcutMethodCallWithStringConcat instanceof CqlShortcutMethodCall { + CqlShortcutMethodCallWithStringConcat() { + exists(StringConcatenation::getAnOperand(super.getAQueryParameter())) + } + + Location getLocation() { result = super.getLocation() } - Expr asExpr() { result = super.asExpr() } + string toString() { result = super.toString() } } -/** - * a more heurisitic based taint step - * captures one of the alternative ways to construct query strings: - * `cds.parse.cql(`string`+userInput)` - * and considers them tainted if they've been concatenated against - * in any manner - */ -class ParseCQLTaintedClause extends CallNode { - ParseCQLTaintedClause() { - this = any(CdsFacade cds).getMember("parse").getMember("cql").getACall() and - exists(DataFlow::Node n | - n = StringConcatenation::getAnOperand(this.getAnArgument()) and - //omit the fact that the arg of cds.parse.cql (`SELECT * from Foo`) - //is technically a string concat - not n.asExpr() instanceof TemplateElement - ) +class CqlClauseParserCallWithStringConcat instanceof CqlClauseParserCall { + CqlClauseParserCallWithStringConcat() { + exists(StringConcatenation::getAnOperand(super.getCdlString())) } + + Location getLocation() { result = super.getLocation() } + + string toString() { result = super.toString() } } -class CqlIConfiguration extends TaintTracking::Configuration { - CqlIConfiguration() { this = "CqlInjection" } +class CqlInjectionConfiguration extends TaintTracking::Configuration { + CqlInjectionConfiguration() { this = "CqlInjection" } override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node node) { - node = any(CdsFacade cds).getMember("db").getMember("run").getACall().getAnArgument() + exists(CqlRunMethodCall cqlRunMethodCall | + node = cqlRunMethodCall.(CqlRunMethodCall).getAQueryParameter() + ) or - exists(AwaitExpr awaitExpr, CqlClause clause | - node.asExpr() = clause.asExpr() and - awaitExpr.getOperand() = clause.asExpr() + exists(CqlShortcutMethodCallWithStringConcat queryRunnerCall | + node = queryRunnerCall.(CqlQueryRunnerCall).getAQueryParameter() + ) + or + exists(AwaitExpr await, CqlClauseWithStringConcatParameter cqlClauseWithStringConcat | + node = await.flow() and + await.getOperand() = cqlClauseWithStringConcat.(CqlClause).getArgument() ) } - override predicate isSanitizer(DataFlow::Node node) { - super.isSanitizer(node) or - node instanceof SqlInjection::Sanitizer - } + override predicate isSanitizer(DataFlow::Node node) { node instanceof SqlInjection::Sanitizer } - override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { - //string concatenation in a clause arg taints the clause - exists(TaintedClause clause | - clause.getArgument() = pred.asExpr() and - clause.asExpr() = succ.asExpr() + override predicate isAdditionalTaintStep(DataFlow::Node start, DataFlow::Node end) { + exists(CqlClauseParserCallWithStringConcat cqlParseCallWithStringConcat | + start = cqlParseCallWithStringConcat.(CqlClauseParserCall).getAnArgument() and + end = cqlParseCallWithStringConcat ) or - //less precise, any concat in the alternative sql stmt construction techniques - exists(ParseCQLTaintedClause parse | - parse.getAnArgument() = pred and - parse = succ + exists(CqlClause cqlClause | + start = cqlClause.getArgument().flow() and + end = cqlClause.flow() ) } } diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index 4329ef5a..ad3fc6f8 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -146,26 +146,31 @@ class ServiceInstanceFromCdsServe extends ServiceInstance { * const Service1 = cds.connect.to("service-2"); * ``` */ -class ServiceInstanceFromCdsConnectTo extends ServiceInstance, SourceNode { +class ServiceInstanceFromCdsConnectTo extends ServiceInstance, SourceNode instanceof PropRead { + string serviceDesignator; string serviceName; - ServiceInstanceFromCdsConnectTo() { this = serviceInstanceFromCdsConnectTo(serviceName) } + ServiceInstanceFromCdsConnectTo() { + this = serviceInstanceFromCdsConnectTo(serviceDesignator).getAPropertyRead(serviceName) + } override UserDefinedApplicationService getDefinition() { /* 1. The service */ exists(RequiredService serviceDecl | - serviceDecl.getName() = serviceName and + serviceDecl.getName() = [serviceName, serviceDesignator] and result.hasLocationInfo(serviceDecl.getImplementationFile().getAbsolutePath(), _, _, _, _) ) or - result.getUnqualifiedName() = serviceName + result.getUnqualifiedName() = serviceDesignator } + string getServiceDesignator() { result = serviceDesignator } + string getServiceName() { result = serviceName } } class DBServiceInstanceFromCdsConnectTo extends ServiceInstanceFromCdsConnectTo { - DBServiceInstanceFromCdsConnectTo() { serviceName = "db" } + DBServiceInstanceFromCdsConnectTo() { serviceDesignator = "db" } } /** @@ -639,15 +644,15 @@ abstract class EntityReference extends CdsReference { */ class EntityReferenceFromEntities extends EntityReference instanceof PropRead { /** - * Property or call to entities + * A read from property `entities` or a method call to `entities`. */ DataFlow::SourceNode entitiesAccess; /** - * Receiver of the entities call or the base of propread + * The receiver of the call to `entities` or the base of the read from `entities`. */ DataFlow::Node receiver; /** - * Name of the (unqualified) entity being accessed + * The unqualified name of the entity being accessed. */ string entityName; @@ -748,14 +753,16 @@ class EntityReferenceFromCqlClause extends EntityReference, ExprNode { } /** - * The `"data"` property of the handler's parameter that represents the request or message passed to this handler. - * This property carries the user-provided payload provided to the CAP application. e.g. + * The `"data"` property of the handler's parameter that represents the request or message + * passed to this handler. This property carries the user-provided payload provided to the + * CAP application. e.g. * ``` javascript * srv.on("send", async (msg) => { * const { payload } = msg.data; * }) * ``` - * The `payload` carries the data that is sent to this application on the action or event named `send`. + * The `payload` carries the data that is sent to this application on the action or event + * named `send`. */ class HandlerParameterData instanceof PropRead { HandlerParameter handlerParameter; @@ -807,7 +814,7 @@ abstract class CqlQueryRunnerCall extends MethodCallNode { string methodName; CqlQueryRunnerCall() { - this = base.getAMemberCall(methodName) and + this = base.getAMemberInvocation(methodName) and ( /* * 1. Method call on the CDS facade or the base database service, @@ -820,28 +827,87 @@ abstract class CqlQueryRunnerCall extends MethodCallNode { exists(ServiceInstance srv | base = srv) ) } + + /** + * Gets an argument to this runner call, including the subsequent builder functions + * called in a chained manner on this one. + */ + abstract DataFlow::Node getAQueryParameter(); } class CqlRunMethodCall extends CqlQueryRunnerCall { CqlRunMethodCall() { this.getMethodName() = "run" } + + override DataFlow::Node getAQueryParameter() { result = this.getArgument(0) } } -class CqlReadMethodCall extends CqlQueryRunnerCall { +class CqlShortcutMethodCall extends CqlQueryRunnerCall { + CqlShortcutMethodCall() { + this.getMethodName() = ["read", "create", "update", "delete", "insert", "upsert"] + } + + abstract override DataFlow::Node getAQueryParameter(); +} + +class CqlReadMethodCall extends CqlShortcutMethodCall { CqlReadMethodCall() { this.getMethodName() = "read" } + + override DataFlow::Node getAQueryParameter() { + result = this.getAChainedMethodCall(_).getAnArgument() + } } -class CqlCreateMethodCall extends CqlQueryRunnerCall { +class CqlCreateMethodCall extends CqlShortcutMethodCall { CqlCreateMethodCall() { this.getMethodName() = "create" } + + override DataFlow::Node getAQueryParameter() { + exists(DataFlow::CallNode chainedMethodCall | + chainedMethodCall = this.getAChainedMethodCall(_) + | + result = chainedMethodCall.getAnArgument() or + result = chainedMethodCall.getAnArgument().(SourceNode).getAPropertyWrite().getRhs() + ) + } } -class CqlUpdateMethodCall extends CqlQueryRunnerCall { +class CqlUpdateMethodCall extends CqlShortcutMethodCall { CqlUpdateMethodCall() { this.getMethodName() = "update" } + + override DataFlow::Node getAQueryParameter() { + result = this.getAChainedMethodCall(_).getAnArgument() + } } -class CqlDeleteMethodCall extends CqlQueryRunnerCall { +class CqlDeleteMethodCall extends CqlShortcutMethodCall { CqlDeleteMethodCall() { this.getMethodName() = "delete" } + + override DataFlow::Node getAQueryParameter() { + result = this.getAChainedMethodCall(_).getAnArgument() + } } -class CqlInsertMethodCall extends CqlQueryRunnerCall { +class CqlInsertMethodCall extends CqlShortcutMethodCall { CqlInsertMethodCall() { this.getMethodName() = "insert" } + + override DataFlow::Node getAQueryParameter() { + exists(DataFlow::CallNode chainedMethodCall | + chainedMethodCall = this.getAChainedMethodCall(_) + | + result = chainedMethodCall.getAnArgument() or + result = chainedMethodCall.getAnArgument().(SourceNode).getAPropertyWrite().getRhs() + ) + } +} + +class CqlUpsertMethodCall extends CqlShortcutMethodCall { + CqlUpsertMethodCall() { this.getMethodName() = "upsert" } + + override DataFlow::Node getAQueryParameter() { + exists(DataFlow::CallNode chainedMethodCall | + chainedMethodCall = this.getAChainedMethodCall(_) + | + result = chainedMethodCall.getAnArgument() or + result = chainedMethodCall.getAnArgument().(SourceNode).getAPropertyWrite().getRhs() + ) + } } diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll index 8b9134c1..8ae28470 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll @@ -354,3 +354,38 @@ class CqlDeleteClause extends CqlClause { result.asDotExpr().getPropertyName() = "from" } } + +/** + * A call to APIs that takes the given input string written in CDL and parses it according to + * the CQN specification. + * + * Note that the outcome of calling the fluent APIs is also a CQN, which means both can be run + * against a service with `srv.run`. + */ +abstract class CqlClauseParserCall extends DataFlow::CallNode { + DataFlow::ExprNode cdlString; + + DataFlow::ExprNode getCdlString() { result = cdlString } +} + +class GlobalCQLFunction extends CqlClauseParserCall { + GlobalCQLFunction() { this = DataFlow::globalVarRef("CQL").getACall() } +} + +class CdsParseCqlCall extends CqlClauseParserCall { + CdsParseCqlCall() { + exists(CdsFacade cds | + this = cds.getMember("parse").getMember("cql").getACall() and + cdlString = this.getArgument(0) + ) + } +} + +class CdsQlCall extends CqlClauseParserCall { + CdsQlCall() { + exists(CdsFacade cds | + this = cds.getMember("ql").getACall() and + cdlString = this.getArgument(0) + ) + } +} diff --git a/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql b/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql index dc97cf91..07dc4baf 100644 --- a/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql +++ b/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql @@ -14,7 +14,7 @@ import javascript import DataFlow::PathGraph import advanced_security.javascript.frameworks.cap.CAPCqlInjectionQuery -from CqlIConfiguration sql, DataFlow::PathNode source, DataFlow::PathNode sink +from CqlInjectionConfiguration sql, DataFlow::PathNode source, DataFlow::PathNode sink where sql.hasFlowPath(source, sink) select sink.getNode(), source, sink, "This query depends on a $@.", source.getNode(), "user-provided value" From 0f950691713f5eb5667b43a903c18f03c65d1a42 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 23 Jun 2025 19:42:45 -0400 Subject: [PATCH 13/29] Finalize CqlInjection --- .../frameworks/cap/CAPCqlInjectionQuery.qll | 2 +- .../javascript/frameworks/cap/CDS.qll | 129 +++++++--- .../javascript/frameworks/cap/CQL.qll | 35 --- .../frameworks/cap/dataflow/DataFlow.qll | 2 +- .../cap/src/cqlinjection/CqlInjection.ql | 3 +- .../test/queries/cqlinjection/srv/service1.js | 224 +++++++++++++++++- 6 files changed, 318 insertions(+), 77 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll index 38d7d9e5..395951a8 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll @@ -44,7 +44,7 @@ class CqlClauseParserCallWithStringConcat instanceof CqlClauseParserCall { } class CqlInjectionConfiguration extends TaintTracking::Configuration { - CqlInjectionConfiguration() { this = "CqlInjection" } + CqlInjectionConfiguration() { this = "CQL injection from untrusted data" } override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index ad3fc6f8..a1756425 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -101,7 +101,7 @@ class CdsConnectToCall extends DataFlow::CallNode { } /** - * A dataflow node that represents a service. + * A data flow node that represents a service. * Note that its definition is a `UserDefinedApplicationService`, not a `ServiceInstance`. */ abstract class ServiceInstance extends SourceNode { @@ -146,7 +146,7 @@ class ServiceInstanceFromCdsServe extends ServiceInstance { * const Service1 = cds.connect.to("service-2"); * ``` */ -class ServiceInstanceFromCdsConnectTo extends ServiceInstance, SourceNode instanceof PropRead { +class ServiceInstanceFromCdsConnectTo extends ServiceInstance { string serviceDesignator; string serviceName; @@ -155,9 +155,8 @@ class ServiceInstanceFromCdsConnectTo extends ServiceInstance, SourceNode instan } override UserDefinedApplicationService getDefinition() { - /* 1. The service */ exists(RequiredService serviceDecl | - serviceDecl.getName() = [serviceName, serviceDesignator] and + serviceDecl.getName() = serviceDesignator and result.hasLocationInfo(serviceDecl.getImplementationFile().getAbsolutePath(), _, _, _, _) ) or @@ -169,10 +168,6 @@ class ServiceInstanceFromCdsConnectTo extends ServiceInstance, SourceNode instan string getServiceName() { result = serviceName } } -class DBServiceInstanceFromCdsConnectTo extends ServiceInstanceFromCdsConnectTo { - DBServiceInstanceFromCdsConnectTo() { serviceDesignator = "db" } -} - /** * A service instance obtained by directly calling the constructor * of its class with a `new` keyword. e.g. @@ -271,6 +266,28 @@ class ServiceInstanceFromServeWithParameter extends ServiceInstance { } } +abstract class CdsDbService extends ServiceInstance { + /* A DB service is implicitly defined. */ + override UserDefinedApplicationService getDefinition() { none() } +} + +class GloballyAccessedCdsDbService extends CdsDbService { + GloballyAccessedCdsDbService() { + exists(CdsFacade cds | + this = cds.getMember("db").asSource() or + this = cds.asSource() + ) + } +} + +/* Note: This should not extend `ServiceInstanceFromCdsConnectTo`, as it does NOT do a property read! */ +class DbServiceInstanceFromCdsConnectTo extends CdsDbService { + DbServiceInstanceFromCdsConnectTo() { this = serviceInstanceFromCdsConnectTo("db") } + + /* A DB service is implicitly defined. */ + override UserDefinedApplicationService getDefinition() { none() } +} + /** * A call to `before`, `on`, or `after` on an `cds.ApplicationService`. * It registers an handler to be executed when an event is fired, @@ -564,16 +581,30 @@ class CdsUser extends API::Node { } } -class CdsTransaction extends MethodCallNode { +class CdsTransaction extends SourceNode { ServiceInstance srv; + CallNode txCall; - CdsTransaction() { this = srv.getAMemberCall("tx") } + CdsTransaction() { + txCall = srv.getAMemberCall("tx") and + ( + this = txCall or + this = + txCall + .getABoundCallbackParameter([ + 0, // When the context object is absent + 1 // When the context object is present + ], 0) + ) + } ServiceInstance getRunner() { result = srv } SourceNode getContextObject() { - result = this.getAnArgument().getALocalSource() and not result instanceof FunctionNode + /* 1. An object node passed as the first argument to a call to `srv.tx`. */ + result = txCall.getALocalSource() and not result instanceof FunctionNode or + /* 2. A manually overriden `cds.context`. */ exists(Stmt stmt, CdsFacade cds | stmt = this.asExpr().getFirstControlFlowNode().getAPredecessor+() and result = cds.getMember("context").asSink() and @@ -583,25 +614,10 @@ class CdsTransaction extends MethodCallNode { DataFlow::Node getUser() { result = this.getContextObject().getAPropertyWrite("user").getRhs() } - MethodCallNode getATransactionCall() { - exists(ControlFlowNode exprOrStmt | - exprOrStmt = - this.getAnArgument().(FunctionNode).getALocalSource().asExpr().(Function).getABodyStmt() and - exprOrStmt.(Stmt).getAChildExpr().flow().(MethodCallNode).getReceiver().getALocalSource() = - this.getAnArgument().(FunctionNode).getParameter(_) and - result = exprOrStmt.(Stmt).getAChildExpr().flow() - or - exprOrStmt = - this.getAnArgument().(FunctionNode).getALocalSource().asExpr().(Function).getAChildExpr() and - exprOrStmt.(Expr).flow().(MethodCallNode).getReceiver().getALocalSource() = - this.getAnArgument().(FunctionNode).getParameter(_) and - result = exprOrStmt.(MethodCallExpr).flow() - or - exprOrStmt = this.asExpr().getFirstControlFlowNode().getASuccessor+() and - exprOrStmt.(Expr).flow().(MethodCallNode).getReceiver().getALocalSource() = this and - result = exprOrStmt.(MethodCallExpr).flow() - ) - } + /** + * Gets a method call on this transaction object. + */ + MethodCallNode getATransactionCall() { result = this.getAMemberCall(_) } CqlClause getAnExecutedCqlClause() { result.asExpr() = this.getATransactionCall().getAnArgument().asExpr() @@ -722,13 +738,11 @@ class EntityReferenceFromUserDefinedServiceEntities extends EntityReferenceFromE } /** - * db.entities, db.entities(...), cds.entities, cds.entities(...) + * cds.entities, cds.entities(...), cds.db.entities, cds.db.entities(...) */ class EntityReferenceFromDbOrCdsEntities extends EntityReferenceFromEntities { EntityReferenceFromDbOrCdsEntities() { - this.getReceiver().getALocalSource() instanceof DBServiceInstanceFromCdsConnectTo or - exists(CdsFacade cds | this.getReceiver().getALocalSource() = cds.getNode()) or - exists(CdsFacade cds | this.getReceiver().getALocalSource() = cds.getMember("db").asSource()) + this.getReceiver().getALocalSource() instanceof CdsDbService } override CdlEntity getCqlDefinition() { @@ -823,8 +837,16 @@ abstract class CqlQueryRunnerCall extends MethodCallNode { exists(CdsFacade cds | base = cds.asSource()) or exists(CdsDb cdsDb | base = cdsDb) or - /* 2. Method call on a service instance object. */ - exists(ServiceInstance srv | base = srv) + /* + * 2. Method call on a service instance object. + */ + + exists(ServiceInstance srv | base.getALocalSource() = srv) or + /* + * 3. Method call on a transaction object. + */ + + exists(CdsTransaction tx | base = tx) ) } @@ -911,3 +933,38 @@ class CqlUpsertMethodCall extends CqlShortcutMethodCall { ) } } + +/** + * A call to APIs that takes the given input string written in CDL and parses it according to + * the CQN specification. + * + * Note that the outcome of calling the fluent APIs is also a CQN, which means both can be run + * against a service with `srv.run`. + */ +abstract class CqlClauseParserCall extends DataFlow::CallNode { + DataFlow::ExprNode cdlString; + + DataFlow::ExprNode getCdlString() { result = cdlString } +} + +class GlobalCQLFunction extends CqlClauseParserCall { + GlobalCQLFunction() { this = DataFlow::globalVarRef("CQL").getACall() } +} + +class CdsParseCqlCall extends CqlClauseParserCall { + CdsParseCqlCall() { + exists(CdsFacade cds | + this = cds.getMember("parse").getMember("cql").getACall() and + cdlString = this.getArgument(0) + ) + } +} + +class CdsQlCall extends CqlClauseParserCall { + CdsQlCall() { + exists(CdsFacade cds | + this = cds.getMember("ql").getACall() and + cdlString = this.getArgument(0) + ) + } +} diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll index 8ae28470..8b9134c1 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll @@ -354,38 +354,3 @@ class CqlDeleteClause extends CqlClause { result.asDotExpr().getPropertyName() = "from" } } - -/** - * A call to APIs that takes the given input string written in CDL and parses it according to - * the CQN specification. - * - * Note that the outcome of calling the fluent APIs is also a CQN, which means both can be run - * against a service with `srv.run`. - */ -abstract class CqlClauseParserCall extends DataFlow::CallNode { - DataFlow::ExprNode cdlString; - - DataFlow::ExprNode getCdlString() { result = cdlString } -} - -class GlobalCQLFunction extends CqlClauseParserCall { - GlobalCQLFunction() { this = DataFlow::globalVarRef("CQL").getACall() } -} - -class CdsParseCqlCall extends CqlClauseParserCall { - CdsParseCqlCall() { - exists(CdsFacade cds | - this = cds.getMember("parse").getMember("cql").getACall() and - cdlString = this.getArgument(0) - ) - } -} - -class CdsQlCall extends CqlClauseParserCall { - CdsQlCall() { - exists(CdsFacade cds | - this = cds.getMember("ql").getACall() and - cdlString = this.getArgument(0) - ) - } -} diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/dataflow/DataFlow.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/dataflow/DataFlow.qll index 9efa768a..67b3dee6 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/dataflow/DataFlow.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/dataflow/DataFlow.qll @@ -110,7 +110,7 @@ class AsyncStyleCommunication extends InterServiceCommunication { [ srvEmit.getEmitter().getDefinition().getManifestName(), srvEmit.getEmitter().getDefinition().getUnqualifiedName() - ] = orchestratingRegistration.getReceiver().(ServiceInstanceFromCdsConnectTo).getServiceName() and + ] = orchestratingRegistration.getReceiver().(ServiceInstanceFromCdsConnectTo).getServiceDesignator() and recipient = methodCallOnReceiver.getReceiver() and methodCallOnReceiver.getEnclosingFunction() = orchestratingRegistration.getHandler().asExpr() ) diff --git a/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql b/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql index 07dc4baf..ef5c8849 100644 --- a/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql +++ b/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql @@ -16,5 +16,6 @@ import advanced_security.javascript.frameworks.cap.CAPCqlInjectionQuery from CqlInjectionConfiguration sql, DataFlow::PathNode source, DataFlow::PathNode sink where sql.hasFlowPath(source, sink) -select sink.getNode(), source, sink, "This query depends on a $@.", source.getNode(), +/* TODO: Print different message if sink is `CqlShortcutMethodCallWithStringConcat` */ +select sink.getNode(), source, sink, "This CQL query depends on a $@.", source.getNode(), "user-provided value" diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js index 68d97c1f..b36c2f9b 100644 --- a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js +++ b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js @@ -23,7 +23,7 @@ module.exports = class Service1 extends cds.ApplicationService { const { id, amount } = req.data; cds.update("Entity1").set("col1 = col1" + amount).where("col1 = " + id); }); - + this.on("send15", async (req) => { const { id } = req.data; cds.insert("Entity1").entries({id: "" + id}); @@ -91,7 +91,7 @@ module.exports = class Service1 extends cds.ApplicationService { const { id, amount } = req.data; this.update(`Service1Entity`).set("col1 = col1" + amount).where("col1 = " + id); }); - + this.on("send35", async (req) => { const { id } = req.data; this.insert(`Service1Entity`).entries({id: "" + id}); @@ -166,7 +166,14 @@ module.exports = class Service1 extends cds.ApplicationService { cds.run(query); }); - /* ========== 7. Service1 running query on Service2 using an unparsed CDL string (only valid in old versions of CAP) ========== */ + /* ========== 7. Service1 running query on the database service using CQN parsed with global function `CQL` ========== */ + this.on("send6", async (req) => { + const { id } = req.data; + const query = cds.parse.cql(`SELECT * from Entity1 where ID =` + id); + cds.run(query); + }); + + /* ========== 8. Service1 running query on Service2 using an unparsed CDL string (only valid in old versions of CAP) ========== */ this.on("send71", async (req) => { const { id } = req.data; const { Service2 } = await cds.connect.to("Service2"); @@ -180,5 +187,216 @@ module.exports = class Service1 extends cds.ApplicationService { const query = `SELECT * from Entity1 where ID =` + id; Service2.run(query); }); + + /* ========== 9. Service1 running query on Service2 using `Service2.tx( tx => tx.run(...) )` and friends ========== */ + this.on("send91", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + const query = SELECT.from`Service2Entity`.where("ID=" + id); + Service2.tx(async (tx) => { + tx.run(query); + }); + }); + + this.on("send92", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.tx(async (tx) => { + tx.read(`Service2Entity`).where("ID =" + id); + }); + }); + + this.on("send93", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.tx(async (tx) => { + tx.create(`Service2Entity`).entries({id: "" + id}); + }); + }); + + this.on("send94", async (req) => { + const { id, amount } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.tx(async (tx) => { + tx.update(`Service2Entity`).set("col1 = col1" + amount).where("col1 = " + id); + }); + }); + + this.on("send95", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.tx(async (tx) => { + tx.insert(`Service2Entity`).entries({id: "" + id}); + }); + }); + + this.on("send96", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.tx(async (tx) => { + tx.upsert(`Service2Entity`).entries({id: "" + id}); + }); + }); + + this.on("send97", async (req) => { + const { id } = req.data; + const { Service2 } = await cds.connect.to("Service2"); + Service2.tx(async (tx) => { + tx.delete(`Service2Entity`).where("ID =" + id); + }); + }); + + /* ========== 10. Service1 running query on itself using `this.tx( tx => tx.run(...) )` and friends ========== */ + this.on("send101", async (req) => { + const { id } = req.data; + const query = SELECT.from`Service1Entity`.where("ID=" + id); + this.tx(async (tx) => { + tx.run(query); + }); + }); + + this.on("send102", async (req) => { + const { id } = req.data; + this.tx(async (tx) => { + tx.read(`Service1Entity`).where("ID =" + id); + }); + }); + + this.on("send103", async (req) => { + const { id } = req.data; + this.tx(async (tx) => { + tx.create(`Service1Entity`).entries({id: "" + id}); + }); + }); + + this.on("send104", async (req) => { + const { id, amount } = req.data; + this.tx(async (tx) => { + tx.update(`Service1Entity`).set("col1 = col1" + amount).where("col1 = " + id); + }); + }); + + this.on("send105", async (req) => { + const { id } = req.data; + this.tx(async (tx) => { + tx.insert(`Service1Entity`).entries({id: "" + id}); + }); + }); + + this.on("send106", async (req) => { + const { id } = req.data; + this.tx(async (tx) => { + tx.upsert(`Service1Entity`).entries({id: "" + id}); + }); + }); + + this.on("send107", async (req) => { + const { id } = req.data; + this.tx(async (tx) => { + tx.delete(`Service1Entity`).where("ID =" + id); + }); + }); + + /* ========== 11. Service1 running query on the database service using `cds.tx( tx => tx.run(...) )` and friends ========== */ + this.on("send111", async (req) => { + const { id } = req.data; + const query = SELECT.from`Entity1`.where("ID=" + id); + cds.tx(async (tx) => { + tx.run(query); + }); + }); + + this.on("send112", async (req) => { + const { id } = req.data; + cds.tx(async (tx) => { + tx.read(`Entity1`).where("ID =" + id); + }); + }); + + this.on("send113", async (req) => { + const { id } = req.data; + cds.tx(async (tx) => { + tx.create(`Entity1`).entries({id: "" + id}); + }); + }); + + this.on("send114", async (req) => { + const { id, amount } = req.data; + cds.tx(async (tx) => { + tx.update(`Entity1`).set("col1 = col1" + amount).where("col1 = " + id); + }); + }); + + this.on("send115", async (req) => { + const { id } = req.data; + cds.tx(async (tx) => { + tx.insert(`Entity1`).entries({id: "" + id}); + }); + }); + + this.on("send116", async (req) => { + const { id } = req.data; + cds.tx(async (tx) => { + tx.upsert(`Entity1`).entries({id: "" + id}); + }); + }); + + this.on("send117", async (req) => { + const { id } = req.data; + cds.tx(async (tx) => { + tx.delete(`Entity1`).where("ID =" + id); + }); + }); + + /* ========== 12. Service1 running query on the database service using `cds.db.tx( tx => tx.run(...) )` and friends ========== */ + this.on("send121", async (req) => { + const { id } = req.data; + const query = SELECT.from`Entity1`.where("ID=" + id); + cds.db.tx(async (tx) => { + tx.run(query); + }); + }); + + this.on("send122", async (req) => { + const { id } = req.data; + cds.db.tx(async (tx) => { + tx.read(`Entity1`).where("ID =" + id); + }); + }); + + this.on("send123", async (req) => { + const { id } = req.data; + cds.db.tx(async (tx) => { + tx.create(`Entity1`).entries({id: "" + id}); + }); + }); + + this.on("send124", async (req) => { + const { id, amount } = req.data; + cds.db.tx(async (tx) => { + tx.update(`Entity1`).set("col1 = col1" + amount).where("col1 = " + id); + }); + }); + + this.on("send125", async (req) => { + const { id } = req.data; + cds.db.tx(async (tx) => { + tx.insert(`Entity1`).entries({id: "" + id}); + }); + }); + + this.on("send126", async (req) => { + const { id } = req.data; + cds.db.tx(async (tx) => { + tx.upsert(`Entity1`).entries({id: "" + id}); + }); + }); + + this.on("send127", async (req) => { + const { id } = req.data; + cds.db.tx(async (tx) => { + tx.delete(`Entity1`).where("ID =" + id); + }); + }); } }; From 1e83f5a75e66d86f6fa39865f0d81f156cb3a1d8 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 23 Jun 2025 20:07:01 -0400 Subject: [PATCH 14/29] Fix test case and code --- .../javascript/frameworks/cap/CDS.qll | 12 ++----- .../test/queries/cqlinjection/srv/service1.js | 34 +++++++++---------- 2 files changed, 20 insertions(+), 26 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index a1756425..7eff46e5 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -148,11 +148,8 @@ class ServiceInstanceFromCdsServe extends ServiceInstance { */ class ServiceInstanceFromCdsConnectTo extends ServiceInstance { string serviceDesignator; - string serviceName; - ServiceInstanceFromCdsConnectTo() { - this = serviceInstanceFromCdsConnectTo(serviceDesignator).getAPropertyRead(serviceName) - } + ServiceInstanceFromCdsConnectTo() { this = serviceInstanceFromCdsConnectTo(serviceDesignator) } override UserDefinedApplicationService getDefinition() { exists(RequiredService serviceDecl | @@ -164,8 +161,6 @@ class ServiceInstanceFromCdsConnectTo extends ServiceInstance { } string getServiceDesignator() { result = serviceDesignator } - - string getServiceName() { result = serviceName } } /** @@ -280,8 +275,7 @@ class GloballyAccessedCdsDbService extends CdsDbService { } } -/* Note: This should not extend `ServiceInstanceFromCdsConnectTo`, as it does NOT do a property read! */ -class DbServiceInstanceFromCdsConnectTo extends CdsDbService { +class DbServiceInstanceFromCdsConnectTo extends ServiceInstanceFromCdsConnectTo, CdsDbService { DbServiceInstanceFromCdsConnectTo() { this = serviceInstanceFromCdsConnectTo("db") } /* A DB service is implicitly defined. */ @@ -602,7 +596,7 @@ class CdsTransaction extends SourceNode { SourceNode getContextObject() { /* 1. An object node passed as the first argument to a call to `srv.tx`. */ - result = txCall.getALocalSource() and not result instanceof FunctionNode + result = txCall.getAnArgument().getALocalSource() and not result instanceof FunctionNode or /* 2. A manually overriden `cds.context`. */ exists(Stmt stmt, CdsFacade cds | diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js index b36c2f9b..aa8cd1ee 100644 --- a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js +++ b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js @@ -110,51 +110,51 @@ module.exports = class Service1 extends cds.ApplicationService { /* ========== 4. Service1 running query on Service2 using `Service2.run` and friends ========== */ this.on("send41", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); const query = SELECT.from`Service1Entity`.where("ID=" + id); Service2.run(query); }); this.on("send42", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.read(`Service2Entity`).where("ID =" + id); }); this.on("send43", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.create(`Service2Entity`).entries({id: "" + id}); }); this.on("send44", async (req) => { const { id, amount } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.update(`Service2Entity`).set("col1 = col1" + amount).where("col1 = " + id); }); this.on("send45", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.insert(`Service2Entity`).entries({id: "" + id}); }); this.on("send46", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.upsert(`Service2Entity`).entries({id: "" + id}); }); this.on("send47", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.delete(`Service2Entity`).where("ID =" + id); }); /* ========== 5. Service1 running query on Service2 using CQN parsed with `cds.ql` ========== */ this.on("send5", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); const query = cds.ql("SELECT * from Service1Entity where ID =" + id); Service2.run(query); }); @@ -176,14 +176,14 @@ module.exports = class Service1 extends cds.ApplicationService { /* ========== 8. Service1 running query on Service2 using an unparsed CDL string (only valid in old versions of CAP) ========== */ this.on("send71", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); const query = "SELECT * from Entity1 where ID =" + id; Service2.run(query); }); this.on("send72", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); const query = `SELECT * from Entity1 where ID =` + id; Service2.run(query); }); @@ -191,7 +191,7 @@ module.exports = class Service1 extends cds.ApplicationService { /* ========== 9. Service1 running query on Service2 using `Service2.tx( tx => tx.run(...) )` and friends ========== */ this.on("send91", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); const query = SELECT.from`Service2Entity`.where("ID=" + id); Service2.tx(async (tx) => { tx.run(query); @@ -200,7 +200,7 @@ module.exports = class Service1 extends cds.ApplicationService { this.on("send92", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.tx(async (tx) => { tx.read(`Service2Entity`).where("ID =" + id); }); @@ -208,7 +208,7 @@ module.exports = class Service1 extends cds.ApplicationService { this.on("send93", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.tx(async (tx) => { tx.create(`Service2Entity`).entries({id: "" + id}); }); @@ -216,7 +216,7 @@ module.exports = class Service1 extends cds.ApplicationService { this.on("send94", async (req) => { const { id, amount } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.tx(async (tx) => { tx.update(`Service2Entity`).set("col1 = col1" + amount).where("col1 = " + id); }); @@ -224,7 +224,7 @@ module.exports = class Service1 extends cds.ApplicationService { this.on("send95", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.tx(async (tx) => { tx.insert(`Service2Entity`).entries({id: "" + id}); }); @@ -232,7 +232,7 @@ module.exports = class Service1 extends cds.ApplicationService { this.on("send96", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.tx(async (tx) => { tx.upsert(`Service2Entity`).entries({id: "" + id}); }); @@ -240,7 +240,7 @@ module.exports = class Service1 extends cds.ApplicationService { this.on("send97", async (req) => { const { id } = req.data; - const { Service2 } = await cds.connect.to("Service2"); + const Service2 = await cds.connect.to("Service2"); Service2.tx(async (tx) => { tx.delete(`Service2Entity`).where("ID =" + id); }); From 9a2e99b7c52db56bd41b23615d1d0125a8abb033 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 23 Jun 2025 20:07:16 -0400 Subject: [PATCH 15/29] Appease compiler to make SensitiveExposure pass Will fix later --- .../sensitive-exposure/SensitiveExposure.ql | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql b/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql index 87990e26..806c9199 100644 --- a/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql +++ b/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql @@ -16,6 +16,25 @@ import advanced_security.javascript.frameworks.cap.CDS import advanced_security.javascript.frameworks.cap.CAPLogInjectionQuery import DataFlow::PathGraph +/** + * An entity instance obtained by the entity's namespace, + * via `cds.entities` + * ```javascript + * // Obtained through `cds.entities` + * const { Service1 } = cds.entities("sample.application.namespace"); + * ``` + */ +class EntityEntry extends DataFlow::CallNode { + EntityEntry() { exists(CdsEntitiesCall c | c.getACall() = this) } + + /** + * Gets the namespace that this entity belongs to. + */ + string getNamespace() { + result = this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() + } +} + SourceNode entityAccesses(TypeTracker t, string entityNamespace) { t.start() and exists(EntityEntry e | result = e and entityNamespace = e.getNamespace()) From c5c97408526adc0afd1d5b3f253ed83c29cd3a65 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 09:37:28 -0400 Subject: [PATCH 16/29] Debug `isSink` --- .../javascript/frameworks/cap/CAPCqlInjectionQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll index 395951a8..a317917e 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll @@ -59,7 +59,7 @@ class CqlInjectionConfiguration extends TaintTracking::Configuration { or exists(AwaitExpr await, CqlClauseWithStringConcatParameter cqlClauseWithStringConcat | node = await.flow() and - await.getOperand() = cqlClauseWithStringConcat.(CqlClause).getArgument() + await.getOperand() = cqlClauseWithStringConcat.(CqlClause).asExpr() ) } From 89522d0f2bdc1ed7a3343ba610eec5d9494c97d7 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 13:25:43 -0400 Subject: [PATCH 17/29] Cover all cases in the current CQL injection test --- .../frameworks/cap/CAPCqlInjectionQuery.qll | 34 +++++++++++++++++++ .../javascript/frameworks/cap/CQL.qll | 24 +++++++------ .../cqlinjection/old/cqlinjection.expected | 29 +++++++++------- .../test/queries/old/cqlinjection.expected | 29 +++++++++------- 4 files changed, 80 insertions(+), 36 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll index a317917e..ed978f68 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll @@ -66,14 +66,48 @@ class CqlInjectionConfiguration extends TaintTracking::Configuration { override predicate isSanitizer(DataFlow::Node node) { node instanceof SqlInjection::Sanitizer } override predicate isAdditionalTaintStep(DataFlow::Node start, DataFlow::Node end) { + /* + * 1. + */ + exists(CqlClauseParserCallWithStringConcat cqlParseCallWithStringConcat | start = cqlParseCallWithStringConcat.(CqlClauseParserCall).getAnArgument() and end = cqlParseCallWithStringConcat ) 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() 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. + */ + + exists(CqlClause cqlClause, PropWrite propWrite | + (cqlClause instanceof CqlInsertClause or cqlClause instanceof CqlUpsertClause) and + cqlClause.getArgument().flow() = propWrite.getBase() and + start = propWrite.getRhs() and + end = propWrite.getBase() + ) } } diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll index 8b9134c1..fd4fb9de 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll @@ -8,16 +8,18 @@ import advanced_security.javascript.frameworks.cap.CDS */ class CqlQueryBase extends VarRef { CqlQueryBase() { - exists(string name | - this.getName() = name and - name in ["SELECT", "INSERT", "DELETE", "UPDATE", "UPSERT"] and - ( - /* Made available as a global variable */ - exists(GlobalVariable queryBase | this = queryBase.getAReference()) - or - /* Imported from `cds.ql` */ - exists(CdsFacade cds | - cds.getMember("ql").getMember(name).getAValueReachableFromSource().asExpr() = this + exists(VarRef varRef | this = varRef | + exists(string name | + varRef.getName() = name and + name in ["SELECT", "INSERT", "DELETE", "UPDATE", "UPSERT"] and + ( + /* Made available as a global variable */ + exists(GlobalVariable queryBase | this = queryBase.getAReference()) + or + /* Imported from `cds.ql` */ + exists(CdsFacade cds | + cds.getMember("ql").getMember(name).getAValueReachableFromSource().asExpr() = this + ) ) ) ) @@ -107,6 +109,8 @@ class CqlClause extends TCqlClause { CallExpr asShortcutCall() { this = TShortcutCall(result) } + predicate isMethodCall() { this = TMethodCall(_) } + Node flow() { result = this.asExpr().flow() } Expr asExpr() { diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.expected b/javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.expected index 5b6e7e20..edb9b40f 100644 --- a/javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.expected +++ b/javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.expected @@ -1,7 +1,6 @@ WARNING: module 'PathGraph' has been deprecated and may be removed in future (CqlInjection.ql:14,8-27) -WARNING: type 'Configuration' has been deprecated and may be removed in future (CqlInjection.ql:19,33-61) -WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:46,29-47) -WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:46,56-74) +WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:17,37-55) +WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:17,64-82) nodes | cqlinjection.js:7:34:7:36 | req | | cqlinjection.js:7:34:7:36 | req | @@ -16,7 +15,8 @@ nodes | cqlinjection.js:12:50:12:53 | book | | cqlinjection.js:13:36:13:40 | query | | cqlinjection.js:13:36:13:40 | query | -| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | +| cqlinjection.js:15:21:15:64 | await S ... book}`) | +| cqlinjection.js:15:21:15:64 | await S ... book}`) | | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | | cqlinjection.js:15:52:15:63 | `ID=${book}` | | cqlinjection.js:15:58:15:61 | book | @@ -26,7 +26,8 @@ nodes | cqlinjection.js:17:53:17:56 | book | | cqlinjection.js:18:37:18:42 | query2 | | cqlinjection.js:18:37:18:42 | query2 | -| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | +| cqlinjection.js:20:21:20:64 | await S ... + book) | +| cqlinjection.js:20:21:20:64 | await S ... + book) | | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | | cqlinjection.js:20:52:20:63 | 'ID=' + book | | cqlinjection.js:20:60:20:63 | book | @@ -59,7 +60,8 @@ edges | cqlinjection.js:12:19:12:56 | SELECT. ... book}`) | cqlinjection.js:12:11:12:56 | query | | cqlinjection.js:12:44:12:55 | `ID=${book}` | cqlinjection.js:12:19:12:56 | SELECT. ... book}`) | | cqlinjection.js:12:50:12:53 | book | cqlinjection.js:12:44:12:55 | `ID=${book}` | -| cqlinjection.js:15:52:15:63 | `ID=${book}` | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | +| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:15:21:15:64 | await S ... book}`) | +| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:15:21:15:64 | await S ... book}`) | | cqlinjection.js:15:52:15:63 | `ID=${book}` | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | | cqlinjection.js:15:58:15:61 | book | cqlinjection.js:15:52:15:63 | `ID=${book}` | | cqlinjection.js:17:11:17:57 | query2 | cqlinjection.js:18:37:18:42 | query2 | @@ -67,7 +69,8 @@ edges | cqlinjection.js:17:20:17:57 | SELECT. ... + book) | cqlinjection.js:17:11:17:57 | query2 | | cqlinjection.js:17:45:17:56 | 'ID=' + book | cqlinjection.js:17:20:17:57 | SELECT. ... + book) | | cqlinjection.js:17:53:17:56 | book | cqlinjection.js:17:45:17:56 | 'ID=' + book | -| cqlinjection.js:20:52:20:63 | 'ID=' + book | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | +| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:20:21:20:64 | await S ... + book) | +| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:20:21:20:64 | await S ... + book) | | cqlinjection.js:20:52:20:63 | 'ID=' + book | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | | cqlinjection.js:20:60:20:63 | book | cqlinjection.js:20:52:20:63 | 'ID=' + book | | cqlinjection.js:27:11:27:62 | cqn | cqlinjection.js:28:39:28:41 | cqn | @@ -80,9 +83,9 @@ edges | cqlinjection.js:30:32:30:59 | `SELECT ... + book | cqlinjection.js:30:18:30:60 | cds.par ... + book) | | cqlinjection.js:30:56:30:59 | book | cqlinjection.js:30:32:30:59 | `SELECT ... + book | #select -| cqlinjection.js:13:36:13:40 | query | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:13:36:13:40 | query | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:18:37:18:42 | query2 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:18:37:18:42 | query2 | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:28:39:28:41 | cqn | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:28:39:28:41 | cqn | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:31:39:31:42 | cqn1 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:31:39:31:42 | cqn1 | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:13:36:13:40 | query | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:13:36:13:40 | query | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:15:21:15:64 | await S ... book}`) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:15:21:15:64 | await S ... book}`) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:18:37:18:42 | query2 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:18:37:18:42 | query2 | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:20:21:20:64 | await S ... + book) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:20:21:20:64 | await S ... + book) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:28:39:28:41 | cqn | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:28:39:28:41 | cqn | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:31:39:31:42 | cqn1 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:31:39:31:42 | cqn1 | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | diff --git a/javascript/frameworks/cap/test/queries/old/cqlinjection.expected b/javascript/frameworks/cap/test/queries/old/cqlinjection.expected index 5b6e7e20..edb9b40f 100644 --- a/javascript/frameworks/cap/test/queries/old/cqlinjection.expected +++ b/javascript/frameworks/cap/test/queries/old/cqlinjection.expected @@ -1,7 +1,6 @@ WARNING: module 'PathGraph' has been deprecated and may be removed in future (CqlInjection.ql:14,8-27) -WARNING: type 'Configuration' has been deprecated and may be removed in future (CqlInjection.ql:19,33-61) -WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:46,29-47) -WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:46,56-74) +WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:17,37-55) +WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:17,64-82) nodes | cqlinjection.js:7:34:7:36 | req | | cqlinjection.js:7:34:7:36 | req | @@ -16,7 +15,8 @@ nodes | cqlinjection.js:12:50:12:53 | book | | cqlinjection.js:13:36:13:40 | query | | cqlinjection.js:13:36:13:40 | query | -| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | +| cqlinjection.js:15:21:15:64 | await S ... book}`) | +| cqlinjection.js:15:21:15:64 | await S ... book}`) | | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | | cqlinjection.js:15:52:15:63 | `ID=${book}` | | cqlinjection.js:15:58:15:61 | book | @@ -26,7 +26,8 @@ nodes | cqlinjection.js:17:53:17:56 | book | | cqlinjection.js:18:37:18:42 | query2 | | cqlinjection.js:18:37:18:42 | query2 | -| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | +| cqlinjection.js:20:21:20:64 | await S ... + book) | +| cqlinjection.js:20:21:20:64 | await S ... + book) | | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | | cqlinjection.js:20:52:20:63 | 'ID=' + book | | cqlinjection.js:20:60:20:63 | book | @@ -59,7 +60,8 @@ edges | cqlinjection.js:12:19:12:56 | SELECT. ... book}`) | cqlinjection.js:12:11:12:56 | query | | cqlinjection.js:12:44:12:55 | `ID=${book}` | cqlinjection.js:12:19:12:56 | SELECT. ... book}`) | | cqlinjection.js:12:50:12:53 | book | cqlinjection.js:12:44:12:55 | `ID=${book}` | -| cqlinjection.js:15:52:15:63 | `ID=${book}` | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | +| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:15:21:15:64 | await S ... book}`) | +| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:15:21:15:64 | await S ... book}`) | | cqlinjection.js:15:52:15:63 | `ID=${book}` | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | | cqlinjection.js:15:58:15:61 | book | cqlinjection.js:15:52:15:63 | `ID=${book}` | | cqlinjection.js:17:11:17:57 | query2 | cqlinjection.js:18:37:18:42 | query2 | @@ -67,7 +69,8 @@ edges | cqlinjection.js:17:20:17:57 | SELECT. ... + book) | cqlinjection.js:17:11:17:57 | query2 | | cqlinjection.js:17:45:17:56 | 'ID=' + book | cqlinjection.js:17:20:17:57 | SELECT. ... + book) | | cqlinjection.js:17:53:17:56 | book | cqlinjection.js:17:45:17:56 | 'ID=' + book | -| cqlinjection.js:20:52:20:63 | 'ID=' + book | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | +| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:20:21:20:64 | await S ... + book) | +| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:20:21:20:64 | await S ... + book) | | cqlinjection.js:20:52:20:63 | 'ID=' + book | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | | cqlinjection.js:20:60:20:63 | book | cqlinjection.js:20:52:20:63 | 'ID=' + book | | cqlinjection.js:27:11:27:62 | cqn | cqlinjection.js:28:39:28:41 | cqn | @@ -80,9 +83,9 @@ edges | cqlinjection.js:30:32:30:59 | `SELECT ... + book | cqlinjection.js:30:18:30:60 | cds.par ... + book) | | cqlinjection.js:30:56:30:59 | book | cqlinjection.js:30:32:30:59 | `SELECT ... + book | #select -| cqlinjection.js:13:36:13:40 | query | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:13:36:13:40 | query | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:18:37:18:42 | query2 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:18:37:18:42 | query2 | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:28:39:28:41 | cqn | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:28:39:28:41 | cqn | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:31:39:31:42 | cqn1 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:31:39:31:42 | cqn1 | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:13:36:13:40 | query | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:13:36:13:40 | query | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:15:21:15:64 | await S ... book}`) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:15:21:15:64 | await S ... book}`) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:18:37:18:42 | query2 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:18:37:18:42 | query2 | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:20:21:20:64 | await S ... + book) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:20:21:20:64 | await S ... + book) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:28:39:28:41 | cqn | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:28:39:28:41 | cqn | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:31:39:31:42 | cqn1 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:31:39:31:42 | cqn1 | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | From 43738d50b684bf6c347fa1eb713a4a93ad2b53e3 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 14:44:08 -0400 Subject: [PATCH 18/29] Get the query depending on the type of sink --- .../cap/src/cqlinjection/CqlInjection.ql | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql b/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql index ef5c8849..0e106546 100644 --- a/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql +++ b/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql @@ -14,8 +14,26 @@ import javascript import DataFlow::PathGraph import advanced_security.javascript.frameworks.cap.CAPCqlInjectionQuery +DataFlow::Node getQueryOfSink(DataFlow::Node sink) { + exists(CqlRunMethodCall cqlRunMethodCall | + sink = cqlRunMethodCall.(CqlRunMethodCall).getAQueryParameter() and + result = sink + ) + or + exists(CqlShortcutMethodCallWithStringConcat shortcutCall | + sink = shortcutCall.(CqlQueryRunnerCall).getAQueryParameter() and + result = shortcutCall + ) + or + exists(AwaitExpr await, CqlClauseWithStringConcatParameter cqlClauseWithStringConcat | + sink = await.flow() and + await.getOperand() = cqlClauseWithStringConcat.(CqlClause).asExpr() and + result = cqlClauseWithStringConcat.(CqlClause).flow() + ) +} + from CqlInjectionConfiguration sql, DataFlow::PathNode source, DataFlow::PathNode sink where sql.hasFlowPath(source, sink) /* TODO: Print different message if sink is `CqlShortcutMethodCallWithStringConcat` */ -select sink.getNode(), source, sink, "This CQL query depends on a $@.", source.getNode(), - "user-provided value" +select getQueryOfSink(sink.getNode()), source, sink, "This CQL query depends on a $@.", + source.getNode(), "user-provided value" From 13691d408166ebbc7cf23e595f33b6066874facc Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 14:47:17 -0400 Subject: [PATCH 19/29] Minor formatting and tidying up --- .../javascript/frameworks/cap/CAPCqlInjectionQuery.qll | 1 + javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll index ed978f68..9e56e402 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll @@ -111,3 +111,4 @@ class CqlInjectionConfiguration extends TaintTracking::Configuration { ) } } + diff --git a/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql b/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql index 0e106546..c05c2784 100644 --- a/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql +++ b/javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql @@ -34,6 +34,5 @@ DataFlow::Node getQueryOfSink(DataFlow::Node sink) { from CqlInjectionConfiguration sql, DataFlow::PathNode source, DataFlow::PathNode sink where sql.hasFlowPath(source, sink) -/* TODO: Print different message if sink is `CqlShortcutMethodCallWithStringConcat` */ select getQueryOfSink(sink.getNode()), source, sink, "This CQL query depends on a $@.", source.getNode(), "user-provided value" From 468a780fbdb3148363760747581906bf08c18651 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 14:54:35 -0400 Subject: [PATCH 20/29] Docstrings and comments --- .../frameworks/cap/CAPCqlInjectionQuery.qll | 21 ++++++++++++++----- .../javascript/frameworks/cap/CDS.qll | 4 ++++ .../javascript/frameworks/cap/CQL.qll | 4 ++-- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll index 9e56e402..3a8aab7f 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll @@ -4,6 +4,9 @@ import advanced_security.javascript.frameworks.cap.CQL import advanced_security.javascript.frameworks.cap.RemoteFlowSources import advanced_security.javascript.frameworks.cap.dataflow.FlowSteps +/** + * A CQL clause parameterized with a string concatentation expression. + */ class CqlClauseWithStringConcatParameter instanceof CqlClause { CqlClauseWithStringConcatParameter() { exists(DataFlow::Node queryParameter | @@ -23,6 +26,10 @@ class CqlClauseWithStringConcatParameter instanceof CqlClause { string toString() { result = super.toString() } } +/** + * A CQL shortcut method call (`read`, `create`, ...) parameterized with a string + * concatenation expression. + */ class CqlShortcutMethodCallWithStringConcat instanceof CqlShortcutMethodCall { CqlShortcutMethodCallWithStringConcat() { exists(StringConcatenation::getAnOperand(super.getAQueryParameter())) @@ -33,6 +40,10 @@ class CqlShortcutMethodCallWithStringConcat instanceof CqlShortcutMethodCall { string toString() { result = super.toString() } } +/** + * A CQL parser call (cds.ql, cds.parse.cql, ...) parameterized with a string + * conatenation expression. + */ class CqlClauseParserCallWithStringConcat instanceof CqlClauseParserCall { CqlClauseParserCallWithStringConcat() { exists(StringConcatenation::getAnOperand(super.getCdlString())) @@ -57,6 +68,7 @@ class CqlInjectionConfiguration extends TaintTracking::Configuration { node = queryRunnerCall.(CqlQueryRunnerCall).getAQueryParameter() ) or + /* 3. An await expression that */ exists(AwaitExpr await, CqlClauseWithStringConcatParameter cqlClauseWithStringConcat | node = await.flow() and await.getOperand() = cqlClauseWithStringConcat.(CqlClause).asExpr() @@ -67,12 +79,12 @@ class CqlInjectionConfiguration extends TaintTracking::Configuration { override predicate isAdditionalTaintStep(DataFlow::Node start, DataFlow::Node end) { /* - * 1. + * 1. Given a call to a CQL parser, jump from the argument to the parser call itself. */ - exists(CqlClauseParserCallWithStringConcat cqlParseCallWithStringConcat | - start = cqlParseCallWithStringConcat.(CqlClauseParserCall).getAnArgument() and - end = cqlParseCallWithStringConcat + exists(CqlClauseParserCall cqlParserCall | + start = cqlParserCall.(CqlClauseParserCall).getAnArgument() and + end = cqlParserCall ) or /* @@ -111,4 +123,3 @@ class CqlInjectionConfiguration extends TaintTracking::Configuration { ) } } - diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index 7eff46e5..7b2f9cb8 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -857,6 +857,10 @@ class CqlRunMethodCall extends CqlQueryRunnerCall { override DataFlow::Node getAQueryParameter() { result = this.getArgument(0) } } +/** + * A [CRUD-style call](https://cap.cloud.sap/docs/node.js/core-services#crud-style-api) + * that translates to running a CQL query internally. + */ class CqlShortcutMethodCall extends CqlQueryRunnerCall { CqlShortcutMethodCall() { this.getMethodName() = ["read", "create", "update", "delete", "insert", "upsert"] diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll index fd4fb9de..5acba2af 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll @@ -3,8 +3,8 @@ import DataFlow import advanced_security.javascript.frameworks.cap.CDS /** - * Objects from the SQL-like fluent API - * this is the set of clauses that acts as the base of a statement + * Objects from the SQL-like fluent API that forms the basis of constructing + * a CQL clause. */ class CqlQueryBase extends VarRef { CqlQueryBase() { From a391d34e3094c3d46659781766f0e2ab30ad2015 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 16:08:38 -0400 Subject: [PATCH 21/29] Fix a regression in `taintedclause` --- .../javascript/frameworks/cap/CAPCqlInjectionQuery.qll | 5 +++-- .../advanced_security/javascript/frameworks/cap/CDS.qll | 8 +++++++- .../cap/test/models/cql/taintedclause/taintedclause.ql | 4 ++-- .../cap/test/queries/cqlinjection/srv/service1.js | 4 ++-- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll index 3a8aab7f..3f5c431a 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll @@ -41,12 +41,13 @@ class CqlShortcutMethodCallWithStringConcat instanceof CqlShortcutMethodCall { } /** - * A CQL parser call (cds.ql, cds.parse.cql, ...) parameterized with a string + * A CQL parser call (`cds.ql`, `cds.parse.cql`, ...) parameterized with a string * conatenation expression. */ class CqlClauseParserCallWithStringConcat instanceof CqlClauseParserCall { CqlClauseParserCallWithStringConcat() { - exists(StringConcatenation::getAnOperand(super.getCdlString())) + not this.getCdlString().(StringOps::Concatenation).asExpr() instanceof TemplateLiteral and + exists(StringConcatenation::getAnOperand(this.getCdlString())) } Location getLocation() { result = super.getLocation() } diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index 7b2f9cb8..a4d3f12c 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -942,11 +942,17 @@ class CqlUpsertMethodCall extends CqlShortcutMethodCall { abstract class CqlClauseParserCall extends DataFlow::CallNode { DataFlow::ExprNode cdlString; + /** + * Gets the data flow node that represents the CDL string to be parsed. + */ DataFlow::ExprNode getCdlString() { result = cdlString } } class GlobalCQLFunction extends CqlClauseParserCall { - GlobalCQLFunction() { this = DataFlow::globalVarRef("CQL").getACall() } + GlobalCQLFunction() { + this = DataFlow::globalVarRef("CQL").getACall() and + cdlString = this.getArgument(0) + } } class CdsParseCqlCall extends CqlClauseParserCall { diff --git a/javascript/frameworks/cap/test/models/cql/taintedclause/taintedclause.ql b/javascript/frameworks/cap/test/models/cql/taintedclause/taintedclause.ql index 3034a38e..31e2973c 100644 --- a/javascript/frameworks/cap/test/models/cql/taintedclause/taintedclause.ql +++ b/javascript/frameworks/cap/test/models/cql/taintedclause/taintedclause.ql @@ -1,5 +1,5 @@ import javascript -import advanced_security.javascript.frameworks.cap.CQL +import advanced_security.javascript.frameworks.cap.CAPCqlInjectionQuery -from ParseCQLTaintedClause clause +from CqlClauseParserCallWithStringConcat clause select clause \ No newline at end of file diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js index aa8cd1ee..3a87a4f8 100644 --- a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js +++ b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js @@ -167,9 +167,9 @@ module.exports = class Service1 extends cds.ApplicationService { }); /* ========== 7. Service1 running query on the database service using CQN parsed with global function `CQL` ========== */ - this.on("send6", async (req) => { + this.on("send7", async (req) => { const { id } = req.data; - const query = cds.parse.cql(`SELECT * from Entity1 where ID =` + id); + const query = CQL(`SELECT * from Entity1 where ID =` + id); cds.run(query); }); From 39b23972fbce46532ed2ec470fac79b1819cf8b8 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 16:09:54 -0400 Subject: [PATCH 22/29] Update expected test outputs of old cqlinjection.js --- .../test/queries/cqlinjection/old/cqlinjection.expected | 8 ++++---- .../frameworks/cap/test/queries/old/cqlinjection.expected | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.expected b/javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.expected index edb9b40f..74a8a5b6 100644 --- a/javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.expected +++ b/javascript/frameworks/cap/test/queries/cqlinjection/old/cqlinjection.expected @@ -1,6 +1,6 @@ WARNING: module 'PathGraph' has been deprecated and may be removed in future (CqlInjection.ql:14,8-27) -WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:17,37-55) -WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:17,64-82) +WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:35,37-55) +WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:35,64-82) nodes | cqlinjection.js:7:34:7:36 | req | | cqlinjection.js:7:34:7:36 | req | @@ -84,8 +84,8 @@ edges | cqlinjection.js:30:56:30:59 | book | cqlinjection.js:30:32:30:59 | `SELECT ... + book | #select | cqlinjection.js:13:36:13:40 | query | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:13:36:13:40 | query | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:15:21:15:64 | await S ... book}`) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:15:21:15:64 | await S ... book}`) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:15:21:15:64 | await S ... book}`) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | | cqlinjection.js:18:37:18:42 | query2 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:18:37:18:42 | query2 | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:20:21:20:64 | await S ... + book) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:20:21:20:64 | await S ... + book) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:20:21:20:64 | await S ... + book) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | | cqlinjection.js:28:39:28:41 | cqn | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:28:39:28:41 | cqn | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | | cqlinjection.js:31:39:31:42 | cqn1 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:31:39:31:42 | cqn1 | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | diff --git a/javascript/frameworks/cap/test/queries/old/cqlinjection.expected b/javascript/frameworks/cap/test/queries/old/cqlinjection.expected index edb9b40f..74a8a5b6 100644 --- a/javascript/frameworks/cap/test/queries/old/cqlinjection.expected +++ b/javascript/frameworks/cap/test/queries/old/cqlinjection.expected @@ -1,6 +1,6 @@ WARNING: module 'PathGraph' has been deprecated and may be removed in future (CqlInjection.ql:14,8-27) -WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:17,37-55) -WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:17,64-82) +WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:35,37-55) +WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:35,64-82) nodes | cqlinjection.js:7:34:7:36 | req | | cqlinjection.js:7:34:7:36 | req | @@ -84,8 +84,8 @@ edges | cqlinjection.js:30:56:30:59 | book | cqlinjection.js:30:32:30:59 | `SELECT ... + book | #select | cqlinjection.js:13:36:13:40 | query | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:13:36:13:40 | query | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:15:21:15:64 | await S ... book}`) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:15:21:15:64 | await S ... book}`) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:15:21:15:64 | await S ... book}`) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | | cqlinjection.js:18:37:18:42 | query2 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:18:37:18:42 | query2 | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:20:21:20:64 | await S ... + book) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:20:21:20:64 | await S ... + book) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | +| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:20:21:20:64 | await S ... + book) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | | cqlinjection.js:28:39:28:41 | cqn | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:28:39:28:41 | cqn | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | | cqlinjection.js:31:39:31:42 | cqn1 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:31:39:31:42 | cqn1 | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | From fd0d13aba60f3ed41d8cd17ca99631d62741c76c Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 16:35:13 -0400 Subject: [PATCH 23/29] Fix a regression in applicationserviceinstance --- .../javascript/frameworks/cap/CDS.qll | 21 +++++++++++++++---- .../sensitive-exposure/SensitiveExposure.ql | 2 +- .../applicationserviceinstance.expected | 1 + 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index a4d3f12c..4fceace8 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -7,13 +7,26 @@ import advanced_security.javascript.frameworks.cap.CQL import advanced_security.javascript.frameworks.cap.RemoteFlowSources /** - * The CDS facade that provides useful interfaces to the current CAP application. + * The CDS facade object that provides useful interfaces to the current CAP application. + * It also acts as a shortcut to `cds.db` when imported via `"@sap/cds"`. + * * ```js * const cds = require('@sap/cds') * ``` */ +/* TODO: Does the `cds` object imported with `"@sap/cds/lib"` also have shortcut to `cds.db`? */ class CdsFacade extends API::Node { - CdsFacade() { this = API::moduleImport(["@sap/cds", "@sap/cds/lib"]) } + string importPath; + + CdsFacade() { + importPath = ["@sap/cds", "@sap/cds/lib"] and + this = API::moduleImport(importPath) + } + + /** + * Holds if this CDS facade object is imported via path `"@sap/cds/lib"`. + */ + predicate isFromCdsLib() { importPath = "@sap/cds/lib" } Node getNode() { result = this.asSource() } } @@ -36,7 +49,7 @@ class CdsEntitiesCall extends DataFlow::CallNode { * The property `db` of on a CDS facade, often accessed as `cds.db`. */ class CdsDb extends SourceNode { - CdsDb() { exists(CdsFacade cds | this = cds.getMember("db").asSource()) } + CdsDb() { exists(CdsFacade cds | not cds.isFromCdsLib() | this = cds.getMember("db").asSource()) } MethodCallNode getRunCall() { result = this.getAMemberCall("run") } @@ -268,7 +281,7 @@ abstract class CdsDbService extends ServiceInstance { class GloballyAccessedCdsDbService extends CdsDbService { GloballyAccessedCdsDbService() { - exists(CdsFacade cds | + exists(CdsFacade cds | not cds.isFromCdsLib() | this = cds.getMember("db").asSource() or this = cds.asSource() ) diff --git a/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql b/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql index 806c9199..ee7f7289 100644 --- a/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql +++ b/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql @@ -21,7 +21,7 @@ import DataFlow::PathGraph * via `cds.entities` * ```javascript * // Obtained through `cds.entities` - * const { Service1 } = cds.entities("sample.application.namespace"); + * const Service1 = cds.entities("sample.application.namespace"); * ``` */ class EntityEntry extends DataFlow::CallNode { diff --git a/javascript/frameworks/cap/test/models/cds/applicationserviceinstance/applicationserviceinstance.expected b/javascript/frameworks/cap/test/models/cds/applicationserviceinstance/applicationserviceinstance.expected index f35e3f24..b829dac5 100644 --- a/javascript/frameworks/cap/test/models/cds/applicationserviceinstance/applicationserviceinstance.expected +++ b/javascript/frameworks/cap/test/models/cds/applicationserviceinstance/applicationserviceinstance.expected @@ -1,3 +1,4 @@ +| applicationserviceinstance.js:1:13:1:31 | require("@sap/cds") | | applicationserviceinstance.js:2:11:2:60 | new cds ... ptions) | | applicationserviceinstance.js:3:12:3:67 | await n ... ptions) | | applicationserviceinstance.js:3:18:3:67 | new cds ... ptions) | From 621a319b522f5e3e66fd167deef6bef2f5aa5527 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 18:22:17 -0400 Subject: [PATCH 24/29] Fix regression in SensitiveExposure --- .../javascript/frameworks/cap/CDS.qll | 8 ++- .../sensitive-exposure/SensitiveExposure.ql | 68 ++++++++----------- .../sensitive-exposure.expected | 8 +-- 3 files changed, 40 insertions(+), 44 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll index 4fceace8..5f70692f 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll @@ -665,7 +665,7 @@ abstract class EntityReference extends CdsReference { * SELECT.from`Books`.where(`ID=${id}`) * ``` */ -class EntityReferenceFromEntities extends EntityReference instanceof PropRead { +class EntityReferenceFromEntities extends EntityReference, SourceNode instanceof PropRead { /** * A read from property `entities` or a method call to `entities`. */ @@ -711,6 +711,12 @@ class EntityReferenceFromEntities extends EntityReference instanceof PropRead { string getEntityName() { result = entityName } + predicate isFromEntitiesCall() { entitiesAccess instanceof MethodCallNode } + + string getEntitiesCallNamespace() { + result = entitiesAccess.(MethodCallNode).getArgument(0).getStringValue() + } + abstract override CdlEntity getCqlDefinition(); abstract UserDefinedApplicationService getServiceDefinition(); diff --git a/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql b/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql index ee7f7289..990be629 100644 --- a/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql +++ b/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql @@ -16,54 +16,44 @@ import advanced_security.javascript.frameworks.cap.CDS import advanced_security.javascript.frameworks.cap.CAPLogInjectionQuery import DataFlow::PathGraph -/** - * An entity instance obtained by the entity's namespace, - * via `cds.entities` - * ```javascript - * // Obtained through `cds.entities` - * const Service1 = cds.entities("sample.application.namespace"); - * ``` - */ -class EntityEntry extends DataFlow::CallNode { - EntityEntry() { exists(CdsEntitiesCall c | c.getACall() = this) } - - /** - * Gets the namespace that this entity belongs to. - */ - string getNamespace() { - result = this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() - } +// /** +// * An entity instance obtained by the entity's namespace via `cds.entities`. e.g. +// * +// * ```javascript +// * const Service1 = cds.entities("sample.application.namespace"); +// * ``` +// */ +// class EntityEntry extends DataFlow::CallNode { +// EntityEntry() { exists(CdsEntitiesCall c | c.getACall() = this) } +// /** +// * Gets the namespace that this entity belongs to. +// */ +// string getNamespace() { +// result = this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() +// } +// } +EntityReferenceFromEntities entityAccesses(string entityNamespace) { + entityNamespace = result.getEntitiesCallNamespace() } -SourceNode entityAccesses(TypeTracker t, string entityNamespace) { - t.start() and - exists(EntityEntry e | result = e and entityNamespace = e.getNamespace()) - or - exists(TypeTracker t2 | result = entityAccesses(t2, entityNamespace).track(t2, t)) -} - -SourceNode entityAccesses(string entityNamespace) { - result = entityAccesses(TypeTracker::end(), entityNamespace) -} - -class SensitiveExposureFieldSource extends DataFlow::Node { - SensitiveAnnotatedAttribute cdsField; - SensitiveAnnotatedEntity entity; +class SensitiveExposureFieldSource instanceof PropRead { + SensitiveAnnotatedAttribute cdlAttribute; + SensitiveAnnotatedEntity cdlEntity; string namespace; SensitiveExposureFieldSource() { - this = entityAccesses(namespace).getAPropertyRead().getAPropertyRead() and + this = entityAccesses(namespace).getAPropertyRead() and //field name is same as some cds declared field - this.(PropRead).getPropertyName() = cdsField.getName() and - //entity name is the same as some cds declared entity - entityAccesses(namespace).getAPropertyRead().toString() = entity.getShortName() and - //and that field belongs to that entity in the cds - entity.(CdlEntity).getAttribute(cdsField.getName()) = cdsField and + this.getPropertyName() = cdlAttribute.getName() and + //and that field belongs to that cdlEntity in the cds + cdlEntity.(CdlEntity).getAttribute(cdlAttribute.getName()) = cdlAttribute and //and the namespace is the same (fully qualified id match) - entity.(NamespacedEntity).getNamespace() = namespace + cdlEntity.(NamespacedEntity).getNamespace() = namespace } - SensitiveAnnotatedAttribute getCdsField() { result = cdsField } + SensitiveAnnotatedAttribute getCdsField() { result = cdlAttribute } + + string toString() { result = super.toString() } } class SensitiveLogExposureConfig extends TaintTracking::Configuration { diff --git a/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected b/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected index 0aeb08ed..1cbec71c 100644 --- a/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected +++ b/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected @@ -1,7 +1,7 @@ WARNING: module 'PathGraph' has been deprecated and may be removed in future (SensitiveExposure.ql:17,8-27) -WARNING: type 'Configuration' has been deprecated and may be removed in future (SensitiveExposure.ql:50,42-70) -WARNING: type 'PathNode' has been deprecated and may be removed in future (SensitiveExposure.ql:60,41-59) -WARNING: type 'PathNode' has been deprecated and may be removed in future (SensitiveExposure.ql:60,68-86) +WARNING: type 'Configuration' has been deprecated and may be removed in future (SensitiveExposure.ql:59,42-70) +WARNING: type 'PathNode' has been deprecated and may be removed in future (SensitiveExposure.ql:69,41-59) +WARNING: type 'PathNode' has been deprecated and may be removed in future (SensitiveExposure.ql:69,68-86) nodes | sensitive-exposure.js:9:32:9:42 | Sample.name | | sensitive-exposure.js:9:32:9:42 | Sample.name | @@ -9,4 +9,4 @@ nodes edges | sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | #select -| sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | Log entry depends on the $@ field which is annotated as potentially sensitive. | sensitive-exposure.cds:4:5:4:8 | {\\n ... } | name | +| sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | Log entry depends on the $@ field which is annotated as potentially sensitive. | sensitive-exposure.cds.json:9:17:13:9 | {\\n ... } | name | From a0f6d9a5b901c0ff600f93814aff504e1e9ca186 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 19:06:04 -0400 Subject: [PATCH 25/29] Remove unneeded code --- .../src/sensitive-exposure/SensitiveExposure.ql | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql b/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql index 990be629..7b9c04c6 100644 --- a/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql +++ b/javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposure.ql @@ -16,22 +16,6 @@ import advanced_security.javascript.frameworks.cap.CDS import advanced_security.javascript.frameworks.cap.CAPLogInjectionQuery import DataFlow::PathGraph -// /** -// * An entity instance obtained by the entity's namespace via `cds.entities`. e.g. -// * -// * ```javascript -// * const Service1 = cds.entities("sample.application.namespace"); -// * ``` -// */ -// class EntityEntry extends DataFlow::CallNode { -// EntityEntry() { exists(CdsEntitiesCall c | c.getACall() = this) } -// /** -// * Gets the namespace that this entity belongs to. -// */ -// string getNamespace() { -// result = this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() -// } -// } EntityReferenceFromEntities entityAccesses(string entityNamespace) { entityNamespace = result.getEntitiesCallNamespace() } From 76521491f354da695deef31b1c14cbf95c42bf33 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 19:08:09 -0400 Subject: [PATCH 26/29] Update expected results of sensitive-exposure --- .../sensitive-exposure/sensitive-exposure.expected | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected b/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected index 1cbec71c..0aeb08ed 100644 --- a/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected +++ b/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected @@ -1,7 +1,7 @@ WARNING: module 'PathGraph' has been deprecated and may be removed in future (SensitiveExposure.ql:17,8-27) -WARNING: type 'Configuration' has been deprecated and may be removed in future (SensitiveExposure.ql:59,42-70) -WARNING: type 'PathNode' has been deprecated and may be removed in future (SensitiveExposure.ql:69,41-59) -WARNING: type 'PathNode' has been deprecated and may be removed in future (SensitiveExposure.ql:69,68-86) +WARNING: type 'Configuration' has been deprecated and may be removed in future (SensitiveExposure.ql:50,42-70) +WARNING: type 'PathNode' has been deprecated and may be removed in future (SensitiveExposure.ql:60,41-59) +WARNING: type 'PathNode' has been deprecated and may be removed in future (SensitiveExposure.ql:60,68-86) nodes | sensitive-exposure.js:9:32:9:42 | Sample.name | | sensitive-exposure.js:9:32:9:42 | Sample.name | @@ -9,4 +9,4 @@ nodes edges | sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | #select -| sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | Log entry depends on the $@ field which is annotated as potentially sensitive. | sensitive-exposure.cds.json:9:17:13:9 | {\\n ... } | name | +| sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | Log entry depends on the $@ field which is annotated as potentially sensitive. | sensitive-exposure.cds:4:5:4:8 | {\\n ... } | name | From 2e56ae0cd934fba5b027c12443f84e491a8da784 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 19:21:09 -0400 Subject: [PATCH 27/29] Fix numbering and remove duplicate case --- .../cap/test/queries/cqlinjection/srv/service1.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js index 3a87a4f8..8dce681c 100644 --- a/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js +++ b/javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js @@ -174,20 +174,13 @@ module.exports = class Service1 extends cds.ApplicationService { }); /* ========== 8. Service1 running query on Service2 using an unparsed CDL string (only valid in old versions of CAP) ========== */ - this.on("send71", async (req) => { + this.on("send81", async (req) => { const { id } = req.data; const Service2 = await cds.connect.to("Service2"); const query = "SELECT * from Entity1 where ID =" + id; Service2.run(query); }); - this.on("send72", async (req) => { - const { id } = req.data; - const Service2 = await cds.connect.to("Service2"); - const query = `SELECT * from Entity1 where ID =` + id; - Service2.run(query); - }); - /* ========== 9. Service1 running query on Service2 using `Service2.tx( tx => tx.run(...) )` and friends ========== */ this.on("send91", async (req) => { const { id } = req.data; From a13a825d9ca38b450b0ea629b86120955d7ee6ba Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 19:25:59 -0400 Subject: [PATCH 28/29] Make the new CQL injection test project a proper test case and remove duplicate old test case --- .../cqlinjection/cqlinjection.expected | 0 .../{old => cqlinjection}/cqlinjection.qlref | 0 .../test/queries/old/cqlinjection.expected | 91 ------------------- .../cap/test/queries/old/cqlinjection.js | 50 ---------- 4 files changed, 141 deletions(-) create mode 100644 javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.expected rename javascript/frameworks/cap/test/queries/{old => cqlinjection}/cqlinjection.qlref (100%) delete mode 100644 javascript/frameworks/cap/test/queries/old/cqlinjection.expected delete mode 100644 javascript/frameworks/cap/test/queries/old/cqlinjection.js diff --git a/javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.expected b/javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.expected new file mode 100644 index 00000000..e69de29b diff --git a/javascript/frameworks/cap/test/queries/old/cqlinjection.qlref b/javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.qlref similarity index 100% rename from javascript/frameworks/cap/test/queries/old/cqlinjection.qlref rename to javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.qlref diff --git a/javascript/frameworks/cap/test/queries/old/cqlinjection.expected b/javascript/frameworks/cap/test/queries/old/cqlinjection.expected deleted file mode 100644 index 74a8a5b6..00000000 --- a/javascript/frameworks/cap/test/queries/old/cqlinjection.expected +++ /dev/null @@ -1,91 +0,0 @@ -WARNING: module 'PathGraph' has been deprecated and may be removed in future (CqlInjection.ql:14,8-27) -WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:35,37-55) -WARNING: type 'PathNode' has been deprecated and may be removed in future (CqlInjection.ql:35,64-82) -nodes -| cqlinjection.js:7:34:7:36 | req | -| cqlinjection.js:7:34:7:36 | req | -| cqlinjection.js:8:13:8:30 | { book, quantity } | -| cqlinjection.js:8:13:8:41 | book | -| cqlinjection.js:8:15:8:18 | book | -| cqlinjection.js:8:34:8:36 | req | -| cqlinjection.js:8:34:8:41 | req.data | -| cqlinjection.js:12:11:12:56 | query | -| cqlinjection.js:12:19:12:56 | SELECT. ... book}`) | -| cqlinjection.js:12:44:12:55 | `ID=${book}` | -| cqlinjection.js:12:50:12:53 | book | -| cqlinjection.js:13:36:13:40 | query | -| cqlinjection.js:13:36:13:40 | query | -| cqlinjection.js:15:21:15:64 | await S ... book}`) | -| cqlinjection.js:15:21:15:64 | await S ... book}`) | -| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | -| cqlinjection.js:15:52:15:63 | `ID=${book}` | -| cqlinjection.js:15:58:15:61 | book | -| cqlinjection.js:17:11:17:57 | query2 | -| cqlinjection.js:17:20:17:57 | SELECT. ... + book) | -| cqlinjection.js:17:45:17:56 | 'ID=' + book | -| cqlinjection.js:17:53:17:56 | book | -| cqlinjection.js:18:37:18:42 | query2 | -| cqlinjection.js:18:37:18:42 | query2 | -| cqlinjection.js:20:21:20:64 | await S ... + book) | -| cqlinjection.js:20:21:20:64 | await S ... + book) | -| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | -| cqlinjection.js:20:52:20:63 | 'ID=' + book | -| cqlinjection.js:20:60:20:63 | book | -| cqlinjection.js:27:11:27:62 | cqn | -| cqlinjection.js:27:17:27:62 | CQL`SEL ... + book | -| cqlinjection.js:27:59:27:62 | book | -| cqlinjection.js:28:39:28:41 | cqn | -| cqlinjection.js:28:39:28:41 | cqn | -| cqlinjection.js:30:11:30:60 | cqn1 | -| cqlinjection.js:30:18:30:60 | cds.par ... + book) | -| cqlinjection.js:30:32:30:59 | `SELECT ... + book | -| cqlinjection.js:30:56:30:59 | book | -| cqlinjection.js:31:39:31:42 | cqn1 | -| cqlinjection.js:31:39:31:42 | cqn1 | -edges -| cqlinjection.js:7:34:7:36 | req | cqlinjection.js:8:34:8:36 | req | -| cqlinjection.js:7:34:7:36 | req | cqlinjection.js:8:34:8:36 | req | -| cqlinjection.js:8:13:8:30 | { book, quantity } | cqlinjection.js:8:15:8:18 | book | -| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:12:50:12:53 | book | -| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:15:58:15:61 | book | -| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:17:53:17:56 | book | -| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:20:60:20:63 | book | -| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:27:59:27:62 | book | -| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:30:56:30:59 | book | -| cqlinjection.js:8:15:8:18 | book | cqlinjection.js:8:13:8:41 | book | -| cqlinjection.js:8:34:8:36 | req | cqlinjection.js:8:34:8:41 | req.data | -| cqlinjection.js:8:34:8:41 | req.data | cqlinjection.js:8:13:8:30 | { book, quantity } | -| cqlinjection.js:12:11:12:56 | query | cqlinjection.js:13:36:13:40 | query | -| cqlinjection.js:12:11:12:56 | query | cqlinjection.js:13:36:13:40 | query | -| cqlinjection.js:12:19:12:56 | SELECT. ... book}`) | cqlinjection.js:12:11:12:56 | query | -| cqlinjection.js:12:44:12:55 | `ID=${book}` | cqlinjection.js:12:19:12:56 | SELECT. ... book}`) | -| cqlinjection.js:12:50:12:53 | book | cqlinjection.js:12:44:12:55 | `ID=${book}` | -| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:15:21:15:64 | await S ... book}`) | -| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:15:21:15:64 | await S ... book}`) | -| cqlinjection.js:15:52:15:63 | `ID=${book}` | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | -| cqlinjection.js:15:58:15:61 | book | cqlinjection.js:15:52:15:63 | `ID=${book}` | -| cqlinjection.js:17:11:17:57 | query2 | cqlinjection.js:18:37:18:42 | query2 | -| cqlinjection.js:17:11:17:57 | query2 | cqlinjection.js:18:37:18:42 | query2 | -| cqlinjection.js:17:20:17:57 | SELECT. ... + book) | cqlinjection.js:17:11:17:57 | query2 | -| cqlinjection.js:17:45:17:56 | 'ID=' + book | cqlinjection.js:17:20:17:57 | SELECT. ... + book) | -| cqlinjection.js:17:53:17:56 | book | cqlinjection.js:17:45:17:56 | 'ID=' + book | -| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:20:21:20:64 | await S ... + book) | -| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:20:21:20:64 | await S ... + book) | -| cqlinjection.js:20:52:20:63 | 'ID=' + book | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | -| cqlinjection.js:20:60:20:63 | book | cqlinjection.js:20:52:20:63 | 'ID=' + book | -| cqlinjection.js:27:11:27:62 | cqn | cqlinjection.js:28:39:28:41 | cqn | -| cqlinjection.js:27:11:27:62 | cqn | cqlinjection.js:28:39:28:41 | cqn | -| cqlinjection.js:27:17:27:62 | CQL`SEL ... + book | cqlinjection.js:27:11:27:62 | cqn | -| cqlinjection.js:27:59:27:62 | book | cqlinjection.js:27:17:27:62 | CQL`SEL ... + book | -| cqlinjection.js:30:11:30:60 | cqn1 | cqlinjection.js:31:39:31:42 | cqn1 | -| cqlinjection.js:30:11:30:60 | cqn1 | cqlinjection.js:31:39:31:42 | cqn1 | -| cqlinjection.js:30:18:30:60 | cds.par ... + book) | cqlinjection.js:30:11:30:60 | cqn1 | -| cqlinjection.js:30:32:30:59 | `SELECT ... + book | cqlinjection.js:30:18:30:60 | cds.par ... + book) | -| cqlinjection.js:30:56:30:59 | book | cqlinjection.js:30:32:30:59 | `SELECT ... + book | -#select -| cqlinjection.js:13:36:13:40 | query | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:13:36:13:40 | query | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:15:21:15:64 | await S ... book}`) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:18:37:18:42 | query2 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:18:37:18:42 | query2 | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:20:21:20:64 | await S ... + book) | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:28:39:28:41 | cqn | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:28:39:28:41 | cqn | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | -| cqlinjection.js:31:39:31:42 | cqn1 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:31:39:31:42 | cqn1 | This CQL query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value | diff --git a/javascript/frameworks/cap/test/queries/old/cqlinjection.js b/javascript/frameworks/cap/test/queries/old/cqlinjection.js deleted file mode 100644 index ee16c5c9..00000000 --- a/javascript/frameworks/cap/test/queries/old/cqlinjection.js +++ /dev/null @@ -1,50 +0,0 @@ -import cds from '@sap/cds' -const { Books } = cds.entities('sap.capire.bookshop') - -class SampleVulnService extends cds.ApplicationService { - init() { - // contains a sample CQL injection - this.on('submitOrder', async req => { - const { book, quantity } = req.data - - let { stock } = await SELECT`stock`.from(Books, book) - - let query = SELECT.from`Books`.where(`ID=${book}`) - let books = await cds.db.run(query) // CQL injection alert - - let books11 = await SELECT.from`Books`.where(`ID=${book}`) // CQL injection alert - - let query2 = SELECT.from`Books`.where('ID=' + book) - let books2 = await cds.db.run(query2) // CQL injection alert - - let books22 = await SELECT.from`Books`.where('ID=' + book) // CQL injection alert - - let books3 = await SELECT.from`Books`.where`ID=${book}` //safe - - let id = 2 - let books33 = await SELECT.from`Books`.where('ID=' + id) //safe - - let cqn = CQL`SELECT col1, col2, col3 from Books` + book - let books222 = await cds.db.run(cqn) // CQL injection alert - - let cqn1 = cds.parse.cql(`SELECT * from Books` + book) - let books111 = await cds.db.run(cqn1) // CQL injection alert - - const pg = require("pg"), - pool = new pg.Pool(config); - pool.query(req.params.category, [], function (err, results) { // non-CQL injection alert from CAP source - // process results - }); - - const app = require("express")(); - app.get("search", function handler(req2, res) { - pool.query(req2.params.category, [], function (err, results) { // non-CQL injection alert from non-CAP source - // process results - }); - }); - - return super.init() - }) - } -} -export { SampleVulnService } \ No newline at end of file From 8a9966172089eb9089062562625284a354c42cc4 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 24 Jun 2025 19:27:24 -0400 Subject: [PATCH 29/29] Remove unneeded comment --- .../javascript/frameworks/cap/CAPCqlInjectionQuery.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll index 3f5c431a..aad95daa 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll @@ -69,7 +69,6 @@ class CqlInjectionConfiguration extends TaintTracking::Configuration { node = queryRunnerCall.(CqlQueryRunnerCall).getAQueryParameter() ) or - /* 3. An await expression that */ exists(AwaitExpr await, CqlClauseWithStringConcatParameter cqlClauseWithStringConcat | node = await.flow() and await.getOperand() = cqlClauseWithStringConcat.(CqlClause).asExpr()