Merge branch 'main' into jeongsoolee09/port-UI5PathGraph

Author: jeongsoolee09

.github/workflows/javascript.sarif.expected

@@ -1,6 +1,6 @@
 ---
 library: true
 name: advanced-security/javascript-sap-cap-models
-version: 2.0.0
+version: 2.1.0
 extensionTargets:
   codeql/javascript-all: "^2.4.0"

javascript/frameworks/cap/ext/qlpack.yml

@@ -22,6 +22,10 @@ class CdsLogger extends MethodCallNode {
   string getName() { result = name }
 }
 
+/**
+ * A template literal that is not interpolated. It is basically a string literal
+ * defined in backticks (\`\`).
+ */
 class ConstantOnlyTemplateLiteral extends TemplateLiteral {
   ConstantOnlyTemplateLiteral() {
     forall(Expr e | e = this.getAnElement() | e instanceof TemplateElement)
@@ -51,9 +55,26 @@ module CAPLogInjectionConfiguration implements DataFlow::ConfigSig {
   }
 
   predicate isBarrier(DataFlow::Node node) {
+    /*
+     * This predicate includes cases such as:
+     * 1. A CDS entity element lacking a type annotation.
+     *   - Possibly because it relies on a common aspect.
+     * 2. A CDS entity element annotated with a non-string type listed above.
+     *
+     * Therefore, the data held by the handler parameter data (e.g. `req.data.X`)
+     * has to be EXPLICITLY annotated as `String` or `LargeString` to be excluded
+     * from the next condition.
+     */
+
     exists(HandlerParameterData handlerParameterData |
       node = handlerParameterData and
-      not handlerParameterData.getType() = ["cds.String", "cds.LargeString"]
+      /* Note the use of `.. != ..` instead of `not .. = ..` below. */
+      exists(string handlerParameterDataType |
+        handlerParameterDataType = handlerParameterData.getType()
+      |
+        handlerParameterDataType != "cds.String" and
+        handlerParameterDataType != "cds.LargeString"
+      )
     )
   }
 

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPLogInjectionQuery.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/lib/advanced_security/javascript/frameworks/cap/CAPPathInjectionQuery.qll

@@ -184,12 +184,12 @@ class ServiceInstanceFromConstructor extends ServiceInstance {
  * const cds = require("@sap/cds");
  * module.exports = class SomeService extends cds.ApplicationService {
  *   init() {
- *     this.on("SomeEvent", (req) => { ... } ) 
+ *     this.on("SomeEvent", (req) => { ... } )
  *   }
  * }
  * ```
  * This class captures the access to the `this` variable as in `this.on(...)`.
- * 
+ *
  * e.g.2. Given this code:
  * ``` javascript
  * const cds = require('@sap/cds');
@@ -424,13 +424,14 @@ class HandlerRegistration extends MethodCallNode {
  *   this.after("SomeEvent", "SomeEntity", (req, next) => { ... });
  * }
  * ```
- * All parameters named `req` above are captured. Also see `HandlerParameterOfExposedService`
+ * All parameters named `req` above are captured. Also see
+ * `RemoteflowSources::HandlerParameterOfExposedService`
  * for a subset of this class that is only about handlers exposed to some protocol.
  */
 class HandlerParameter extends ParameterNode {
   Handler handler;
 
-  HandlerParameter() { this = handler.getParameter(0) }
+  HandlerParameter() { this = isHandlerParameter(handler) }
 
   Handler getHandler() { result = handler }
 }

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

@@ -39,3 +39,12 @@ private SourceNode cdsApplicationServiceInstantiation(TypeTracker t) {
 SourceNode cdsApplicationServiceInstantiation() {
   result = cdsApplicationServiceInstantiation(TypeTracker::end())
 }
+
+private SourceNode isHandlerParameter(TypeTracker t, Handler handler) {
+  result = handler.getParameter(0) or
+  exists(TypeTracker t2 | result = isHandlerParameter(t, handler).track(t2, t))
+}
+
+SourceNode isHandlerParameter(Handler handler) {
+  result = isHandlerParameter(TypeTracker::end(), handler)
+}

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

@@ -1,7 +1,7 @@
 ---
 library: true
 name: advanced-security/javascript-sap-cap-all
-version: 2.0.0
+version: 2.1.0
 suites: codeql-suites
 extractor: javascript
 dependencies:

javascript/frameworks/cap/lib/qlpack.yml

@@ -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.md

@@ -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/src/path-traversal/PathInjection.ql

@@ -1,11 +1,11 @@
 ---
 library: false
 name: advanced-security/javascript-sap-cap-queries
-version: 2.0.0
+version: 2.1.0
 suites: codeql-suites
 extractor: javascript
 dependencies:
   codeql/javascript-all: "^2.4.0"
-  advanced-security/javascript-sap-cap-models: "^2.0.0"
-  advanced-security/javascript-sap-cap-all: "^2.0.0"
+  advanced-security/javascript-sap-cap-models: "^2.1.0"
+  advanced-security/javascript-sap-cap-all: "^2.1.0"
 default-suite-file: codeql-suites/javascript-code-scanning.qls