Add CDS Utils path injection query

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

@@ -7,13 +7,21 @@
 import javascript
 import advanced_security.javascript.frameworks.cap.CDSUtils
 
-abstract class UtilsAccessedPathSink extends DataFlow::Node { }
+abstract class UtilsSink extends DataFlow::Node {
+  abstract string sinkType();
+}
 
-abstract class UtilsControlledDataSink extends DataFlow::Node { }
+abstract class UtilsAccessedPathSink extends UtilsSink {
+  override string sinkType() { result = "unrestricted file read" }
+}
 
-abstract class UtilsControlledPathSink extends DataFlow::Node { }
+abstract class UtilsControlledDataSink extends UtilsSink {
+  override string sinkType() { result = "tainted data being written to a file" }
+}
 
-abstract class UtilsExtraFlow extends DataFlow::Node { }
+abstract class UtilsControlledPathSink extends UtilsSink {
+  override string sinkType() { result = "unrestricted file operations" }
+}
 
 /**
  * This represents the data in calls as follows:
@@ -67,21 +75,3 @@ class ControlledInputPath extends UtilsControlledPathSink {
     exists(DirectoryReaders dr | dr.getPath() = this)
   }
 }
-
-/**
- * This represents calls where the taint flows through the call. e.g.
- * ```javascript
- * let dir = isdir ('app')
- * ```
- */
-class AdditionalFlowStep extends UtilsExtraFlow {
-  AdditionalFlowStep() {
-    exists(PathConverters pc | pc.getPath() = this)
-    or
-    exists(PathPredicates pr | pr.getPath() = this)
-  }
-
-  DataFlow::CallNode getOutgoingNode() { result = this }
-
-  DataFlow::Node getIngoingNode() { result = this.(DataFlow::CallNode).getAnArgument() }
-}

javascript/frameworks/cap/src/path-traversal/PathInjection.md

@@ -0,0 +1,63 @@
+# CAP CDS Utils used with user-controlled sources
+
+If a path is constructed from user-provided input without sufficient sanitization, a malicious user may be able to manipulate the contents of the filesystem without proper authorization.
+
+Additionally if user-provided input is used to create file contents this can also result in a malicious user manipulating the filesystem in an unchecked way.
+
+## Recommendation
+
+CAP applications using CDS Utils should not use user-provided input without sanitization.
+
+The sanitization stragety can vary depending on what types of paths are satisfactory as user-provided input. A simple approach to sanitization is to check user-provided input against an allow list. Other potential approaches include checking components of paths or normalizing them to make sure that the path does not escape the expected root folder. 
+
+Normalization techniques should be carefully considered and simple naive replacement strategies will not be sufficient, for example replacing any match of a parent directory reference (`../`) in the sample `.../...//` will still result in the path `../` being used which could escape the intended directory.
+
+## Examples
+
+This CAP service directly uses user-provided input to construct a path.
+
+``` javascript
+const cds = require("@sap/cds");
+const { rm } = cds.utils
+
+module.exports = class Service1 extends cds.ApplicationService {
+
+    init() {
+        this.on("send1", async (req) => {
+            let userinput = req.data
+            await rm(userinput, 'db', 'data') // Path injection alert
+        }
+    }
+}
+```
+
+This CAP service directly uses user-provided input to add content to a file.
+
+``` javascript
+const cds = require("@sap/cds");
+const { rm } = cds.utils
+
+module.exports = class Service1 extends cds.ApplicationService {
+  init() {
+    this.on("send1", async (req) => {
+      let userinput = req.data
+      await write(userinput).to('db/data') // Path injection alert
+
+      // GOOD: the path can not be controlled by an attacker
+      let allowedDirectories = [
+        'this-is-a-safe-directory'
+      ];
+      if (allowedDirectories.includes(userinput)) {
+        await rm(userinput) // sanitized - No Path injection alert
+      }
+    }
+  }
+}
+```
+
+## References
+
+- OWASP 2021: [Injection](https://owasp.org/Top10/A03_2021-Injection/).
+- SAP CAP CDS Utils : [Documentation](https://cap.cloud.sap/docs/node.js/cds-utils).
+- Common Weakness Enumeration: [CWE-020](https://cwe.mitre.org/data/definitions/20.html).
+- Common Weakness Enumeration: [CWE-022](https://cwe.mitre.org/data/definitions/22.html).

javascript/frameworks/cap/src/path-traversal/PathInjection.ql

@@ -0,0 +1,48 @@
+/**
+ * @name Use of user controlled input in CAP CDS file system utilities
+ * @description Using unchecked user controlled values can allow an
+ *              attacker to affect paths constructed and accessed in
+ *              the filesystem.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 7.5
+ * @precision medium
+ * @id js/cap-path-injection
+ * @tags security
+ *       external/cwe/cwe-020
+ *       external/cwe/cwe-022
+ */
+
+import javascript
+import advanced_security.javascript.frameworks.cap.CAPPathInjectionQuery
+import advanced_security.javascript.frameworks.cap.RemoteFlowSources
+private import semmle.javascript.security.dataflow.TaintedPathCustomizations
+private import semmle.javascript.security.dataflow.TaintedPathQuery as tq
+
+module PathInjectionConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof UtilsSink }
+
+  predicate isAdditionalFlowStep(DataFlow::Node nodein, DataFlow::Node nodeout) {
+    exists(PathConverters pc | pc.getPath() = nodein and nodeout = pc)
+    or
+    exists(PathPredicates pr | pr.getPath() = nodein and nodeout = pr)
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof TaintedPath::Sanitizer
+    or
+    tq::TaintedPathConfig::isBarrier(node)
+  }
+}
+
+module PathInjectionConfigFlow = TaintTracking::Global<PathInjectionConfig>;
+
+import PathInjectionConfigFlow::PathGraph
+
+from PathInjectionConfigFlow::PathNode source, PathInjectionConfigFlow::PathNode sink
+where PathInjectionConfigFlow::flowPath(source, sink)
+select sink, source, sink,
+  "This CDS utils usage relies on user-provided value and can result in " +
+    sink.getNode().(UtilsSink).sinkType() + "."

javascript/frameworks/cap/test/models/cds/utils/utils.expected

@@ -1,38 +1,33 @@
-| utils.js:5:21:5:30 | "%E0%A4%A" | "%E0%A4%A": additional flow step |
-| utils.js:7:31:7:40 | "%E0%A4%A" | "%E0%A4%A": additional flow step |
-| utils.js:9:18:9:27 | "%E0%A4%A" | "%E0%A4%A": additional flow step |
-| utils.js:13:17:13:21 | 'app' | 'app': additional flow step |
-| utils.js:15:19:15:32 | 'package.json' | 'package.json': additional flow step |
-| utils.js:17:22:17:35 | 'package.json' | 'package.json': controlled path sink |
-| utils.js:19:26:19:39 | 'package.json' | 'package.json': controlled path sink |
-| utils.js:21:20:21:33 | 'package.json' | 'package.json': controlled path sink |
-| utils.js:23:20:23:33 | 'package.json' | 'package.json': controlled path sink |
-| utils.js:25:14:25:22 | 'db/data' | 'db/data': controlled data sink |
-| utils.js:25:28:25:41 | 'dist/db/data' | 'dist/db/data': controlled path sink |
-| utils.js:26:14:26:22 | 'db/data' | 'db/data': controlled path sink |
-| utils.js:26:25:26:38 | 'dist/db/data' | 'dist/db/data': controlled data sink |
-| utils.js:28:12:28:20 | 'db/data' | 'db/data': accessed path sink |
-| utils.js:28:26:28:39 | 'dist/db/data' | 'dist/db/data': controlled path sink |
-| utils.js:29:12:29:20 | 'db/data' | 'db/data': accessed path sink |
-| utils.js:29:23:29:36 | 'dist/db/data' | 'dist/db/data': controlled path sink |
-| utils.js:31:13:31:26 | { foo: 'bar' } | { foo: 'bar' }: controlled data sink |
-| utils.js:31:32:31:47 | 'some/file.json' | 'some/file.json': controlled path sink |
-| utils.js:32:13:32:28 | 'some/file.json' | 'some/file.json': controlled path sink |
-| utils.js:32:31:32:44 | { foo: 'bar' } | { foo: 'bar' }: controlled data sink |
-| utils.js:34:14:34:19 | 'dist' | 'dist': controlled path sink |
-| utils.js:34:22:34:25 | 'db' | 'db': controlled path sink |
-| utils.js:34:28:34:33 | 'data' | 'data': controlled path sink |
-| utils.js:35:14:35:27 | 'dist/db/data' | 'dist/db/data': controlled path sink |
-| utils.js:37:13:37:18 | 'dist' | 'dist': controlled path sink |
-| utils.js:37:21:37:24 | 'db' | 'db': controlled path sink |
-| utils.js:37:27:37:32 | 'data' | 'data': controlled path sink |
-| utils.js:38:13:38:26 | 'dist/db/data' | 'dist/db/data': controlled path sink |
-| utils.js:40:14:40:19 | 'dist' | 'dist': controlled path sink |
-| utils.js:40:22:40:25 | 'db' | 'db': controlled path sink |
-| utils.js:40:28:40:33 | 'data' | 'data': controlled path sink |
-| utils.js:41:14:41:27 | 'dist/db/data' | 'dist/db/data': controlled path sink |
-| utils.js:43:10:43:15 | 'dist' | 'dist': controlled path sink |
-| utils.js:43:18:43:21 | 'db' | 'db': controlled path sink |
-| utils.js:43:24:43:29 | 'data' | 'data': controlled path sink |
-| utils.js:44:10:44:23 | 'dist/db/data' | 'dist/db/data': controlled path sink |
-| utils.js:52:20:52:28 | 'db/data' | 'db/data': controlled data sink |
+| utils.js:5:22:5:35 | 'package.json' | 'package.json': controlled path sink |
+| utils.js:7:26:7:39 | 'package.json' | 'package.json': controlled path sink |
+| utils.js:9:20:9:33 | 'package.json' | 'package.json': controlled path sink |
+| utils.js:11:20:11:33 | 'package.json' | 'package.json': controlled path sink |
+| utils.js:13:14:13:22 | 'db/data' | 'db/data': controlled data sink |
+| utils.js:13:28:13:41 | 'dist/db/data' | 'dist/db/data': controlled path sink |
+| utils.js:14:14:14:22 | 'db/data' | 'db/data': controlled path sink |
+| utils.js:14:25:14:38 | 'dist/db/data' | 'dist/db/data': controlled data sink |
+| utils.js:16:12:16:20 | 'db/data' | 'db/data': accessed path sink |
+| utils.js:16:26:16:39 | 'dist/db/data' | 'dist/db/data': controlled path sink |
+| utils.js:17:12:17:20 | 'db/data' | 'db/data': accessed path sink |
+| utils.js:17:23:17:36 | 'dist/db/data' | 'dist/db/data': controlled path sink |
+| utils.js:19:13:19:26 | { foo: 'bar' } | { foo: 'bar' }: controlled data sink |
+| utils.js:19:32:19:47 | 'some/file.json' | 'some/file.json': controlled path sink |
+| utils.js:20:13:20:28 | 'some/file.json' | 'some/file.json': controlled path sink |
+| utils.js:20:31:20:44 | { foo: 'bar' } | { foo: 'bar' }: controlled data sink |
+| utils.js:22:14:22:19 | 'dist' | 'dist': controlled path sink |
+| utils.js:22:22:22:25 | 'db' | 'db': controlled path sink |
+| utils.js:22:28:22:33 | 'data' | 'data': controlled path sink |
+| utils.js:23:14:23:27 | 'dist/db/data' | 'dist/db/data': controlled path sink |
+| utils.js:25:13:25:18 | 'dist' | 'dist': controlled path sink |
+| utils.js:25:21:25:24 | 'db' | 'db': controlled path sink |
+| utils.js:25:27:25:32 | 'data' | 'data': controlled path sink |
+| utils.js:26:13:26:26 | 'dist/db/data' | 'dist/db/data': controlled path sink |
+| utils.js:28:14:28:19 | 'dist' | 'dist': controlled path sink |
+| utils.js:28:22:28:25 | 'db' | 'db': controlled path sink |
+| utils.js:28:28:28:33 | 'data' | 'data': controlled path sink |
+| utils.js:29:14:29:27 | 'dist/db/data' | 'dist/db/data': controlled path sink |
+| utils.js:31:10:31:15 | 'dist' | 'dist': controlled path sink |
+| utils.js:31:18:31:21 | 'db' | 'db': controlled path sink |
+| utils.js:31:24:31:29 | 'data' | 'data': controlled path sink |
+| utils.js:32:10:32:23 | 'dist/db/data' | 'dist/db/data': controlled path sink |
+| utils.js:40:20:40:28 | 'db/data' | 'db/data': controlled data sink |

javascript/frameworks/cap/test/models/cds/utils/utils.js

@@ -1,18 +1,6 @@
 const cds = require("@sap/cds");
 
-const { decodeURI, decodeURIComponent, local, exists, isdir, isfile, read, readdir, append, write, copy, stat, find, mkdirp, rmdir, rimraf, rm } = cds.utils
-
-let uri = decodeURI("%E0%A4%A") // taint step
-
-let uri2 = decodeURIComponent("%E0%A4%A") // taint step
-
-let uri3 = local("%E0%A4%A") // taint step
-
-let uri4 = exists("%E0%A4%A") // NOT a taint step - returns a boolean
-
-let dir = isdir('app') // taint step
-
-let file = isfile('package.json') // taint step
+const { read, readdir, append, write, copy, stat, find, mkdirp, rmdir, rimraf, rm } = cds.utils
 
 let pkg = await read('package.json') // sink
 

javascript/frameworks/cap/test/models/cds/utils/utils.ql

@@ -8,6 +8,4 @@ where
   node.(UtilsAccessedPathSink).toString() = str and strfull = str + ": accessed path sink"
   or
   node.(UtilsControlledDataSink).toString() = str and strfull = str + ": controlled data sink"
-  or
-  node.(UtilsExtraFlow).toString() = str and strfull = str + ": additional flow step"
 select node, strfull