Finalize working draft on current CqlInjection test case

Author: jeongsoolee09

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()
     )
   }
 }

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()
+    )
+  }
 }

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)
+    )
+  }
+}

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"