Merge pull request #115 from advanced-security/mbaluda/type_sanitizer

Exclude injection alerts where the input data type is not String

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPLogInjectionQuery.qll

@@ -29,7 +29,7 @@ class ConstantOnlyTemplateLiteral extends TemplateLiteral {
 }
 
 /**
- * Arguments of calls to `cds.log.{trace, debug, info, log, warn, error}`
+ * Arguments of calls to `cds.log.{trace, debug, info, log, warn, error}`.
  */
 class CdsLogSink extends DataFlow::Node {
   CdsLogSink() {
@@ -49,5 +49,12 @@ class CAPLogInjectionConfiguration extends LogInjectionConfiguration {
     start instanceof RemoteFlowSource
   }
 
+  override predicate isBarrier(DataFlow::Node node) {
+    exists(HandlerParameterData handlerParameterData |
+      node = handlerParameterData and
+      not handlerParameterData.getType() = ["cds.String", "cds.LargeString"]
+    )
+  }
+
   override predicate isSink(DataFlow::Node end) { end instanceof CdsLogSink }
 }

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll

@@ -181,6 +181,10 @@ class CdlService extends CdlElement {
 
   CdlEntity getAnEntity() { result = this.getEntity(_) }
 
+  CdlActionOrFunction getAnActionOrFunction() {
+    result = this.getAnEvent() or result = this.getAnAction()
+  }
+
   CdlEvent getEvent(string eventName) {
     result.getName() = eventName and this.getName() = result.getName().splitAt(".", 0)
   }
@@ -232,13 +236,27 @@ class CdlEntity extends CdlElement {
   }
 }
 
-class CdlEvent extends CdlElement {
+abstract class CdlActionOrFunction extends CdlElement {
+  /**
+   * Gets a parameter definition of this action with a given name.
+   */
+  CdlAttribute getParameter(string paramName) {
+    result = this.getPropValue("params").getPropValue(paramName)
+  }
+
+  /**
+   * Gets a parameter definition of this action.
+   */
+  CdlAttribute getAParameter() { result = this.getParameter(_) }
+}
+
+class CdlEvent extends CdlActionOrFunction {
   CdlEvent() { kind = CdlEventKind(this.getPropStringValue("kind")) }
 
   string getBasename() { result = name.splitAt(".", count(name.indexOf("."))) }
 }
 
-class CdlAction extends CdlElement {
+class CdlAction extends CdlActionOrFunction {
   CdlAction() { kind = CdlActionKind(this.getPropStringValue("kind")) }
 
   predicate belongsToServiceWithNoAuthn() {
@@ -260,7 +278,10 @@ class CdlAttribute extends CdlObject {
   string name;
 
   CdlAttribute() {
-    exists(CdlElement entity | this = entity.getPropValue("elements").getPropValue(name))
+    exists(CdlElement entity | this = entity.getPropValue("elements").getPropValue(name)) or
+    exists(CdlActionOrFunction actionOrFunction |
+      this = actionOrFunction.getPropValue("params").getPropValue(name)
+    )
   }
 
   override string getObjectLocationName() { result = getName() }

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll

@@ -4,6 +4,7 @@ import advanced_security.javascript.frameworks.cap.TypeTrackers
 import advanced_security.javascript.frameworks.cap.PackageJson
 import advanced_security.javascript.frameworks.cap.CDL
 import advanced_security.javascript.frameworks.cap.CQL
+import advanced_security.javascript.frameworks.cap.RemoteFlowSources
 
 /**
  * ```js
@@ -701,3 +702,48 @@ class EntityReferenceFromCqlClause extends EntityReference, ExprNode {
 
   override CdlEntity getCqlDefinition() { result = cql.getAccessingEntityDefinition() }
 }
+
+/**
+ * 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`.
+ */
+class HandlerParameterData instanceof PropRead {
+  HandlerParameter handlerParameter;
+  string dataName;
+
+  HandlerParameterData() {
+    this = handlerParameter.getAPropertyRead("data").getAPropertyRead(dataName)
+  }
+
+  /**
+   * Gets the type of this handler parameter data.
+   */
+  string getType() {
+    /*
+     * The result string is the type of this parameter if:
+     * - There is an actionName which is this HandlerRegistration's action/function name, and
+     * - The actionName in the CDS declaration has params with the same name of this parameter, and
+     * - The result is the name of the type of the parameter.
+     */
+
+    exists(
+      UserDefinedApplicationService userDefinedApplicationService,
+      CdlActionOrFunction cdlActionOrFunction, HandlerRegistration handlerRegistration
+    |
+      handlerRegistration = userDefinedApplicationService.getAHandlerRegistration() and
+      handlerRegistration = handlerParameter.getHandlerRegistration() and
+      handlerRegistration.getAnEventName() = cdlActionOrFunction.getUnqualifiedName() and
+      cdlActionOrFunction =
+        userDefinedApplicationService.getCdsDeclaration().getAnActionOrFunction() and
+      result = cdlActionOrFunction.getParameter(dataName).getType()
+    )
+  }
+
+  string toString() { result = this.(PropRead).toString() }
+}

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/RemoteFlowSources.qll

@@ -12,11 +12,11 @@ import advanced_security.javascript.frameworks.cap.CDS
  * All the parameters named `req` and `msg` are captured in the above example.
  */
 class HandlerParameter extends ParameterNode, RemoteFlowSource {
+  Handler handler;
+  HandlerRegistration handlerRegistration;
+
   HandlerParameter() {
-    exists(
-      Handler handler, HandlerRegistration handlerRegistration,
-      UserDefinedApplicationService service
-    |
+    exists(UserDefinedApplicationService service |
       handler = handlerRegistration.getHandler() and
       this = handler.getParameter(0) and
       service.getAHandlerRegistration() = handlerRegistration and
@@ -27,13 +27,26 @@ class HandlerParameter extends ParameterNode, RemoteFlowSource {
   override string getSourceType() {
     result = "Parameter of an event handler belonging to an exposed service"
   }
+
+  /**
+   * Gets the handler this is a parameter of.
+   */
+  Handler getHandler() { result = handler }
+
+  /**
+   * Gets the handler registration registering the handler it is a parameter of.
+   */
+  HandlerRegistration getHandlerRegistration() { result = handlerRegistration }
 }
 
 /**
  * A service may be described only in a CDS file, but event handlers may still be registered in a format such as:
  * ```javascript
  * module.exports = srv => {
- * srv.before('CREATE', 'Media', req => { //an entity name is used to describe which to register this handler to
+ *   srv.before('CREATE', 'Media', req => { // an entity name is used to describe which to register this handler to.
+ *     ...
+ *   });
+ * }
  * ```
  * parameters named `req` are captured in the above example.
  */

javascript/frameworks/cap/test/queries/loginjection/log-injection-type-sanitized/README.md

@@ -0,0 +1,7 @@
+# Sanitized Log Injection
+
+This application demonstrates how a potential injection vulnerability is not reported if the data type definied in the service description is not strings.
+
+## It _is_ a false positive case
+
+Service responds to a Received event and logs the data. However, the type of the message (Integer) does not allow for the injection to succeed.

javascript/frameworks/cap/test/queries/loginjection/log-injection-type-sanitized/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)
+}

javascript/frameworks/cap/test/queries/loginjection/log-injection-type-sanitized/log-injection-type-sanitized.expected

@@ -0,0 +1,3 @@
+nodes
+edges
+#select

javascript/frameworks/cap/test/queries/loginjection/log-injection-type-sanitized/log-injection-type-sanitized.qlref

@@ -0,0 +1 @@
+loginjection/LogInjection.ql

javascript/frameworks/cap/test/queries/loginjection/log-injection-type-sanitized/package.json

@@ -0,0 +1,21 @@
+{
+    "name": "@advanced-security/log-injection",
+    "version": "1.0.0",
+    "dependencies": {
+        "@cap-js/sqlite": "*",
+        "@sap/cds": "^7.9.5",
+        "@sap/cds-dk": "^8.6.1",
+        "express": "^4.17.1"
+    },
+    "scripts": {
+        "start": "cds-serve",
+        "watch": "cds watch"
+    },
+    "cds": {
+        "requires": {
+            "service": {
+                "impl": "srv/service.js"
+            }
+        }
+    }
+}

javascript/frameworks/cap/test/queries/loginjection/log-injection-type-sanitized/server.js

@@ -0,0 +1,4 @@
+const cds = require('@sap/cds');
+const app = require('express')();
+
+cds.serve('all').in(app);

javascript/frameworks/cap/test/queries/loginjection/log-injection-type-sanitized/srv/service.cds

@@ -0,0 +1,11 @@
+using { advanced_security.log_injection.sample_entities as db_schema } from '../db/schema';
+
+service Service @(path: '/service') {
+  /* Entity to send READ/GET about. */
+  entity ServiceEntity as projection on db_schema.Entity2 excluding { Attribute4 }
+
+  /* API to talk to Service. */
+  action send (
+    messageToPass: Integer
+  ) returns String;
+}

javascript/frameworks/cap/test/queries/loginjection/log-injection-type-sanitized/srv/service.js

@@ -0,0 +1,11 @@
+const cds = require("@sap/cds");
+const LOG = cds.log("logger");
+
+module.exports = cds.service.impl(function() {
+    /* Log upon receiving an "send" event. */
+    this.on("send", async (msg) => {
+        const { messageToPass } = msg.data;
+        /* A log injection sink. */
+        LOG.info("Received: ", messageToPass); // messageToPass is Integer, not a log injection!
+  });
+})