Restrict RemoteFlowSource of CAP to only some properties and method calls on it

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

@@ -346,16 +346,26 @@ class HandlerRegistration extends MethodCallNode {
 }
 
 /**
- * A parameter of a handler
+ * The first parameter of a handler, representing the request object received either directly
+ * from a user, or from another service that may be internal (defined in the same application) 
+ * or external (defined in another application, or even served from a different server).
+ * e.g.
+ * ``` javascript
+ * module.exports = class Service1 extends cds.ApplicationService {
+ *   this.on("SomeEvent", "SomeEntity", (req) => { ... });
+ *   this.before("SomeEvent", "SomeEntity", (req, next) => { ... });
+ *   this.after("SomeEvent", "SomeEntity", (req, next) => { ... });
+ * }
+ * ``` 
+ * All parameters named `req` above are captured. Also see `HandlerParameterOfExposedService`
+ * for a subset of this class that is only about handlers exposed to some protocol.
  */
-class HandlerParameter instanceof ParameterNode {
+class HandlerParameter extends ParameterNode {
   Handler handler;
 
   HandlerParameter() { this = handler.getParameter(0) }
 
   Handler getHandler() { result = handler }
-
-  string toString() { result = super.toString() }
 }
 
 /**
@@ -832,7 +842,7 @@ class HandlerParameterData instanceof PropRead {
   string dataName;
 
   HandlerParameterData() {
-    this = handlerParameter.(SourceNode).getAPropertyRead("data").getAPropertyRead(dataName)
+    this = handlerParameter.getAPropertyRead("data").getAPropertyRead(dataName)
   }
 
   /**

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

@@ -2,35 +2,81 @@ import javascript
 import advanced_security.javascript.frameworks.cap.CDS
 
 /**
- * Either of:
- * a parameter of a handler registered for an (exposed) service on an event. e.g.
- * ```javascript
- * this.on("SomeEvent", "SomeEntity", (req) => { ... });
- * this.before("SomeEvent", "SomeEntity", (req, next) => { ... });
- * SomeService.on("SomeEvent", "SomeEntity", (msg) => { ... });
- * SomeService.after("SomeEvent", "SomeEntity", (msg) => { ... });
+ * The request parameter of a handler belonging to a service that is exposed to
+ * a protocol. e.g. All parameters named `req` is captured in the below example.
+ * ``` javascript
+ * // srv/service1.js
+ * module.exports = class Service1 extends cds.ApplicationService {
+ *   this.on("SomeEvent", "SomeEntity", (req) => { ... });
+ *   this.before("SomeEvent", "SomeEntity", (req, next) => { ... });
+ * }
  * ```
- * OR
- * a handler parameter that is not connected to a service
- * possibly due to cds compilation failure
- * or non explicit service references in source. e.g.
- * ```javascript
- * cds.serve('./test-service').with((srv) => {
- *    srv.after('READ', req => req.target.data) //req
- * })
+ * ``` cds
+ * // srv/service1.cds
+ * service Service1 @(path: '/service-1') { ... }
  * ```
+ *
+ * NOTE: CDS extraction can fail for various reasons, and if so the detection
+ * logic falls back on overapproximating on the parameters and assume they are
+ * exposed.
  */
-class HandlerParameterOfExposedService extends RemoteFlowSource, HandlerParameter {
+class HandlerParameterOfExposedService extends HandlerParameter {
   HandlerParameterOfExposedService() {
+    /* 1. The CDS definition is there and we can determine it is exposed. */
     this.getHandler().getHandlerRegistration().getService().getDefinition().isExposed()
     or
-    /* no precise service definition is known */
+    /*
+     * 2. (Fallback) The CDS definition is not there, so no precise service definition
+     * is known.
+     */
+
     not exists(this.getHandler().getHandlerRegistration().getService().getDefinition())
   }
+}
+
+/**
+ * Reads of property belonging to a request parameter that is exposed to a protocol.
+ * It currently models the following access paths:
+ * - `req.data` (from `cds.Event.data`)
+ * - `req.params` (from `cds.Request.params`)
+ * - `req.headers` (from `cds.Event.headers`)
+ * - `req.http.req.*` (from `cds.EventContext.http.req`)
+ * - `req.id` (from `cds.EventContext.id`)
+ *
+ * Note that `req.http.req` has type `require("@express").Request`, so their uses are
+ * completely identical. Subsequently, the models for this access path follow Express'
+ * API descriptions (as far back as 3.x). Also see `Express::RequestInputAccess`,
+ * `Express::RequestHeaderAccess`, and `Express::RequestBodyAccess` of the standard
+ * library.
+ */
+class UserProvidedPropertyReadOfHandlerParameterOfExposedService extends RemoteFlowSource {
+  HandlerParameterOfExposedService handlerParameterOfExposedService;
+
+  UserProvidedPropertyReadOfHandlerParameterOfExposedService() {
+    /* 1. `req.(data|params|headers|id)` */
+    this = handlerParameterOfExposedService.getAPropertyRead(["data", "params", "headers", "id"])
+    or
+    /* 2. APIs stemming from `req.http.req`: Defined by Express.js */
+    exists(PropRead reqHttpReq |
+      reqHttpReq = handlerParameterOfExposedService.getAPropertyRead("http").getAPropertyRead("req")
+    |
+      this = reqHttpReq.getAPropertyRead(["originalUrl", "hostname"]) or
+      this =
+        reqHttpReq
+            .getAPropertyRead(["query", "body", "params", "headers", "cookies"])
+            .getAPropertyRead() or
+      this = reqHttpReq.getAMemberCall(["get", "is", "header", "param"])
+    )
+  }
+
+  HandlerParameterOfExposedService getHandlerParameter() {
+    result = handlerParameterOfExposedService
+  }
 
-  override string toString() { result = HandlerParameter.super.toString() }
+  Handler getHandler() { result = handlerParameterOfExposedService.getHandler() }
 
   override string getSourceType() {
-    result = "Parameter of an event handler belonging to an exposed service"
+    result =
+      "Tainted property read of the request parameter of an event handler belonging to an exposed service"
   }
 }

javascript/frameworks/cap/test/models/cds/remoteflowsources/ExposedServices.expected

@@ -0,0 +1,21 @@
+| server.js:7:32:7:34 | req |
+| server.js:11:32:11:34 | req |
+| srv/service1.js:6:29:6:31 | req |
+| srv/service1.js:12:29:12:31 | req |
+| srv/service1.js:18:29:18:31 | req |
+| srv/service1.js:24:29:24:31 | req |
+| srv/service1.js:40:29:40:31 | req |
+| srv/service1.js:46:29:46:31 | req |
+| srv/service1.js:52:29:52:31 | req |
+| srv/service1.js:58:29:58:31 | req |
+| srv/service1.js:64:29:64:31 | req |
+| srv/service2.js:4:27:4:29 | msg |
+| srv/service3.js:5:29:5:31 | req |
+| srv/service3.js:11:29:11:31 | req |
+| srv/service3.js:17:29:17:31 | req |
+| srv/service3.js:23:29:23:31 | req |
+| srv/service3.js:39:29:39:31 | req |
+| srv/service3.js:45:29:45:31 | req |
+| srv/service3.js:51:29:51:31 | req |
+| srv/service3.js:57:29:57:31 | req |
+| srv/service3.js:63:29:63:31 | req |

javascript/frameworks/cap/test/models/cds/remoteflowsources/ExposedServices.ql

@@ -0,0 +1,5 @@
+import javascript
+import advanced_security.javascript.frameworks.cap.RemoteFlowSources
+
+from HandlerParameterOfExposedService handlerParameterOfExposedService
+select handlerParameterOfExposedService

javascript/frameworks/cap/test/models/cds/remoteflowsources/remoteflowsource.expected

@@ -1,12 +1,31 @@
-| srv/service1.js:6:29:6:31 | req |
-| srv/service2.js:5:27:5:29 | msg |
-| srv/service3nocds.js:6:43:6:45 | req |
-| srv/service3nocds.js:7:34:7:36 | req |
-| srv/service3nocds.js:11:28:11:30 | req |
-| srv/service3nocds.js:12:26:12:28 | req |
-| srv/service3nocds.js:19:34:19:36 | req |
-| srv/service4withcds.js:5:38:5:40 | req |
-| srv/service4withcds.js:6:43:6:45 | req |
-| srv/service4withcds.js:14:33:14:35 | req |
-| srv/service4withcds.js:15:38:15:40 | req |
-| srv/service4withcds.js:16:23:16:25 | req |
+| srv/service1.js:7:33:7:40 | req.data |
+| srv/service1.js:13:33:13:42 | req.params |
+| srv/service1.js:19:29:19:39 | req.headers |
+| srv/service1.js:25:30:25:56 | req.htt ... omeProp |
+| srv/service1.js:26:30:26:55 | req.htt ... omeProp |
+| srv/service1.js:27:30:27:57 | req.htt ... omeProp |
+| srv/service1.js:28:30:28:58 | req.htt ... omeProp |
+| srv/service1.js:29:30:29:58 | req.htt ... omeProp |
+| srv/service1.js:30:30:30:53 | req.htt ... inalUrl |
+| srv/service1.js:31:30:31:50 | req.htt ... ostname |
+| srv/service1.js:32:30:32:57 | req.htt ... eProp") |
+| srv/service1.js:33:30:33:56 | req.htt ... eProp") |
+| srv/service1.js:34:31:34:61 | req.htt ... eProp") |
+| srv/service1.js:35:31:35:60 | req.htt ... eProp") |
+| srv/service1.js:41:29:41:34 | req.id |
+| srv/service2.js:5:31:5:38 | msg.data |
+| srv/service3.js:6:33:6:40 | req.data |
+| srv/service3.js:12:33:12:42 | req.params |
+| srv/service3.js:18:29:18:39 | req.headers |
+| srv/service3.js:24:30:24:56 | req.htt ... omeProp |
+| srv/service3.js:25:30:25:55 | req.htt ... omeProp |
+| srv/service3.js:26:30:26:57 | req.htt ... omeProp |
+| srv/service3.js:27:30:27:58 | req.htt ... omeProp |
+| srv/service3.js:28:30:28:58 | req.htt ... omeProp |
+| srv/service3.js:29:30:29:53 | req.htt ... inalUrl |
+| srv/service3.js:30:30:30:50 | req.htt ... ostname |
+| srv/service3.js:31:30:31:57 | req.htt ... eProp") |
+| srv/service3.js:32:30:32:56 | req.htt ... eProp") |
+| srv/service3.js:33:31:33:61 | req.htt ... eProp") |
+| srv/service3.js:34:31:34:60 | req.htt ... eProp") |
+| srv/service3.js:40:29:40:34 | req.id |

javascript/frameworks/cap/test/models/cds/remoteflowsources/server.js

@@ -2,3 +2,11 @@ const cds = require("@sap/cds");
 const app = require("express")();
 
 cds.serve("all").in(app);
+
+cds.serve('Service1').with((srv) => {
+  srv.before('READ', 'Books', (req) => req.reply([])) // SAFE: Exposed service (fallback), but not a taint source
+})
+
+cds.serve('Service3').with((srv) => {
+  srv.before('READ', 'Books', (req) => req.reply([])) // SAFE: Exposed service (fallback), but not a taint source
+})

javascript/frameworks/cap/test/models/cds/remoteflowsources/srv/service1.js

@@ -4,7 +4,65 @@ const cds = require("@sap/cds");
 module.exports = class Service1 extends cds.ApplicationService {
   init() {
     this.on("send1", async (req) => {
-      const { messageToPass } = req.data;
+      const { messageToPass } = req.data;  // UNSAFE: Taint source, Exposed service
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send2", async (req) => {
+      const [ messageToPass ] = req.params;  // UNSAFE: Taint source, Exposed service
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send3", async (req) => {
+      const messageToPass = req.headers["user-agent"];  // UNSAFE: Taint source, Exposed service
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send4", async (req) => {
+      const messageToPass1 = req.http.req.query.someProp;  // UNSAFE: Taint source, Exposed service
+      const messageToPass2 = req.http.req.body.someProp;  // UNSAFE: Taint source, Exposed service
+      const messageToPass3 = req.http.req.params.someProp;  // UNSAFE: Taint source, Exposed service
+      const messageToPass4 = req.http.req.headers.someProp;  // UNSAFE: Taint source, Exposed service
+      const messageToPass5 = req.http.req.cookies.someProp;  // UNSAFE: Taint source, Exposed service
+      const messageToPass6 = req.http.req.originalUrl;  // UNSAFE: Taint source, Exposed service
+      const messageToPass7 = req.http.req.hostname;  // UNSAFE: Taint source, Exposed service
+      const messageToPass8 = req.http.req.get("someProp");  // UNSAFE: Taint source, Exposed service
+      const messageToPass9 = req.http.req.is("someProp");  // UNSAFE: Taint source, Exposed service
+      const messageToPass10 = req.http.req.header("someProp");  // UNSAFE: Taint source, Exposed service
+      const messageToPass11 = req.http.req.param("someProp");  // UNSAFE: Taint source, Exposed service
+      const Service2 = await cds.connect.to("service-2");  // UNSAFE: Taint source, Exposed service
+      Service2.send("send2", { messageToPass1 });
+    });
+
+    this.on("send5", async (req) => {
+      const messageToPass = req.id;  // UNSAFE: Taint source, Exposed service
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send6", async (req) => {
+      const messageToPass = req.locale;  // SAFE: Not a taint source, Exposed service
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send7", async (req) => {
+      const messageToPass = req.tenant;  // SAFE: Not a taint source, Exposed service
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send8", async (req) => {
+      const messageToPass = req.timestamp;  // SAFE: Not a taint source, Exposed service
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send9", async (req) => {
+      const messageToPass = req.user;  // SAFE: Not a taint source, Exposed service
       const Service2 = await cds.connect.to("service-2");
       Service2.send("send2", { messageToPass });
     });

javascript/frameworks/cap/test/models/cds/remoteflowsources/srv/service2.cds

@@ -1,12 +1,8 @@
 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;

javascript/frameworks/cap/test/models/cds/remoteflowsources/srv/service2.js

@@ -1,10 +1,8 @@
 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 { messageToPass } = msg.data;  // UNSAFE: Taint source, Exposed service
     const doSomething = console.log;
     doSomething(messageToPass);
   });

javascript/frameworks/cap/test/models/cds/remoteflowsources/srv/service3.js

@@ -0,0 +1,71 @@
+// This service unit test is a replica of requesthandler.js 
+const cds = require("@sap/cds");
+class Service3 extends cds.ApplicationService {
+  init() {
+    this.on("send1", async (req) => {
+      const { messageToPass } = req.data;  // UNSAFE: Taint source, Exposed service (fallback)
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send2", async (req) => {
+      const [ messageToPass ] = req.params;  // UNSAFE: Taint source, Exposed service (fallback)
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send3", async (req) => {
+      const messageToPass = req.headers["user-agent"];  // UNSAFE: Taint source, Exposed service (fallback)
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send4", async (req) => {
+      const messageToPass1 = req.http.req.query.someProp;  // UNSAFE: Taint source, Exposed service (fallback)
+      const messageToPass2 = req.http.req.body.someProp;  // UNSAFE: Taint source, Exposed service (fallback)
+      const messageToPass3 = req.http.req.params.someProp;  // UNSAFE: Taint source, Exposed service (fallback)
+      const messageToPass4 = req.http.req.headers.someProp;  // UNSAFE: Taint source, Exposed service (fallback)
+      const messageToPass5 = req.http.req.cookies.someProp;  // UNSAFE: Taint source, Exposed service (fallback)
+      const messageToPass6 = req.http.req.originalUrl;  // UNSAFE: Taint source, Exposed service (fallback)
+      const messageToPass7 = req.http.req.hostname;  // UNSAFE: Taint source, Exposed service (fallback)
+      const messageToPass8 = req.http.req.get("someProp");  // UNSAFE: Taint source, Exposed service (fallback)
+      const messageToPass9 = req.http.req.is("someProp");  // UNSAFE: Taint source, Exposed service (fallback)
+      const messageToPass10 = req.http.req.header("someProp");  // UNSAFE: Taint source, Exposed service (fallback)
+      const messageToPass11 = req.http.req.param("someProp");  // UNSAFE: Taint source, Exposed service (fallback)
+      const Service2 = await cds.connect.to("service-2");  // UNSAFE: Taint source, Exposed service (fallback)
+      Service2.send("send2", { messageToPass1 });
+    });
+
+    this.on("send5", async (req) => {
+      const messageToPass = req.id;  // UNSAFE: Taint source, Exposed service (fallback)
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send6", async (req) => {
+      const messageToPass = req.locale;  // SAFE: Not a taint source, Exposed service (fallback)
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send7", async (req) => {
+      const messageToPass = req.tenant;  // SAFE: Not a taint source, Exposed service (fallback)
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send8", async (req) => {
+      const messageToPass = req.timestamp;  // SAFE: Not a taint source, Exposed service (fallback)
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    this.on("send9", async (req) => {
+      const messageToPass = req.user;  // SAFE: Not a taint source, Exposed service (fallback)
+      const Service2 = await cds.connect.to("service-2");
+      Service2.send("send2", { messageToPass });
+    });
+
+    return super.init()
+  }
+}