UI5 client side log-injection improvements

javascript/frameworks/ui5/ext/ui5.model.yml

@@ -25,6 +25,8 @@ extensions:
       - ["SapLogger", "global", "Member[jQuery].Member[sap].Member[log]"]
       # Logging functions as well as `getLogger` also serves as a constructor
       - ["SapLogger", "SapLogger", "Member[debug,error,fatal,info,setLevel,trace,warning,getLogger].ReturnValue"]
+      - ["SapLogEntries", "SapLogger", "Member[addLogListener].Parameter[0].Member[onLogEntry].Argument[0]"]
+      - ["SapLogEntries", "SapLogger", "Member[getLogEntries].ReturnValue"]
       - ["ResourceBundle", "ResourceBundle", "Instance"]
       - ["ResourceBundle", "sap/base/i18n/ResourceBundle", ""]
       - ["Properties", "Properties", "Instance"]
@@ -64,7 +66,6 @@ extensions:
       - ["UI5ClientStorage", "sap/ui/core/util/File", ""]
       - ["UI5ClientStorage", "global", "Member[sap].Member[ui].Member[core].Member[util].Member[File]"]
 
-
   - addsTo:
       pack: codeql/javascript-all
       extensible: "sourceModel"

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/FlowSteps.qll

@@ -1,5 +1,6 @@
 import javascript
 import advanced_security.javascript.frameworks.ui5.UI5
+private import semmle.javascript.frameworks.data.internal.ApiGraphModelsExtensions as ApiGraphModelsExtensions
 
 /**
  * Step from a part of internal model to a relevant control property.
@@ -25,7 +26,8 @@ class InternalModelContentToCustomMetadataPropertyStep extends DataFlow::SharedF
 }
 
 /**
- * This is a step in the opposite direction of the `InternalModelContentToCustomMetadataPropertyStep` above. In order to ensure that this indeed holds, we check if the internal model is set to a two-way binding mode.
+ * This is a step in the opposite direction of the `InternalModelContentToCustomMetadataPropertyStep` above.
+ * In order to ensure that this indeed holds, we check if the internal model is set to a two-way binding mode.
  */
 class CustomMetadataPropertyStepToInternalModelContent extends DataFlow::SharedFlowStep {
   override predicate step(DataFlow::Node start, DataFlow::Node end) {
@@ -81,7 +83,8 @@ class CustomMetadataPropertyReadStep extends DataFlow::SharedFlowStep {
 }
 
 /**
- * Step from the second argument of `setProperty` method call on a local model or a reference to that model, that is, the receiver. If the contents of the model is statically visible, then get the relevant portion of the content instead.
+ * Step from the second argument of `setProperty` method call on a local model or a reference to that model, that is, the receiver.
+ * If the contents of the model is statically visible, then get the relevant portion of the content instead.
  *
  * e.g. Given these methods in a same controller,
  *
@@ -96,7 +99,8 @@ class CustomMetadataPropertyReadStep extends DataFlow::SharedFlowStep {
  * }
  * ```
  *
- * Create an edge from `someValue` to `this.getView().getModel()`. As the content of `oModel` can be statically determined, also create an edge from `someValue` to `x: null`.
+ * Create an edge from `someValue` to `this.getView().getModel()`.
+ * As the content of `oModel` can be statically determined, also create an edge from `someValue` to `x: null`.
  */
 class LocalModelSetPropertyStep extends DataFlow::SharedFlowStep {
   override predicate step(DataFlow::Node start, DataFlow::Node end) {
@@ -170,7 +174,9 @@ class LocalModelSetPropertyStep extends DataFlow::SharedFlowStep {
  * }
  * ```
  *
- * Establish an edge from `this.getView().getModel("someModel")` in `someHandler2` to the entire `getProperty` call in `someHandler1`. Note that `modelRefFrom` and `modelRefTo` may refer to the same `ModelReference`.
+ * Establish an edge from `this.getView().getModel("someModel")` in `someHandler2` to
+ * the entire `getProperty` call in `someHandler1`.
+ * Note that `modelRefFrom` and `modelRefTo` may refer to the same `ModelReference`.
  *
  * This is a dual of `LocalModelSetPropertyStep` above.
  */
@@ -232,7 +238,8 @@ class LocalModelGetPropertyStep extends DataFlow::SharedFlowStep {
 }
 
 /**
- * Step from a local model or a reference to it, to a `getProperty` method call on it. This assumes a corresponding `setProperty` call exists on the same model and its same property.
+ * Step from a local model or a reference to it, to a `getProperty` method call on it.
+ * This assumes a corresponding `setProperty` call exists on the same model and its same property.
  *
  * e.g. Given these methods in a same controller,
  * ```javascript
@@ -250,7 +257,9 @@ class LocalModelGetPropertyStep extends DataFlow::SharedFlowStep {
  * }
  * ```
  *
- * Establish an edge from `this.getView().getModel("someModel")` in `someHandler2` to the entire `getProperty` call in `someHandler1`. Note that `modelRefFrom` and `modelRefTo` may refer to the same `ModelReference`.
+ * Establish an edge from `this.getView().getModel("someModel")` in `someHandler2` to
+ * the entire `getProperty` call in `someHandler1`.
+ * Note that `modelRefFrom` and `modelRefTo` may refer to the same `ModelReference`.
  */
 /*
  * TODO:
@@ -334,3 +343,21 @@ class ResourceBundleGetTextCallArgToReturnValueStep extends DataFlow::SharedFlow
     )
   }
 }
+
+/**
+ * A step from any argument of a SAP logging function to the `onLogEntry`
+ * method of a custom log listener in the same application.
+ */
+class LogArgumentToListener extends DataFlow::SharedFlowStep {
+  override predicate step(DataFlow::Node start, DataFlow::Node end) {
+    exists(WebApp webApp |
+      webApp.getAResource() = start.getFile() and webApp.getAResource() = end.getFile()
+    ) and
+    start =
+      ModelOutput::getATypeNode("SapLogger")
+          .getMember(["debug", "error", "fatal", "info", "trace", "warning"])
+          .getACall()
+          .getAnArgument() and
+    end = ModelOutput::getATypeNode("SapLogEntries").asSource()
+  }
+}

javascript/frameworks/ui5/src/UI5LogInjection/UI5LogInjection.ql

@@ -1,9 +1,8 @@
 /**
- * @name UI5 Log injection
- * @description Building log entries from user-controlled sources is vulnerable to
- *              insertion of forged log entries by a malicious user.
+ * @name UI5 client-side Log injection
+ * @description Log entries should not include user-controlled data.
  * @kind path-problem
- * @problem.severity error
+ * @problem.severity recommendation
  * @security-severity 7.8
  * @precision medium
  * @id js/ui5-log-injection

javascript/frameworks/ui5/src/UI5LogInjection/UI5LogsToHttp.ql

@@ -0,0 +1,36 @@
+/**
+ * @name UI5 Log injection in outbound network request
+ * @description Building log entries from user-controlled sources is vulnerable to
+ *              insertion of forged log entries by a malicious user.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 6.5
+ * @precision medium
+ * @id js/ui5-log-injection-to-http
+ * @tags security
+ *       external/cwe/cwe-117
+ */
+
+import javascript
+import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
+import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph
+import semmle.javascript.security.dataflow.LogInjectionQuery as LogInjection
+
+class UI5LogInjectionConfiguration extends LogInjection::LogInjectionConfiguration {
+  override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node node) {
+    exists(ClientRequest req |
+      node = req.getUrl() or
+      node = req.getADataNode()
+    )
+  }
+}
+
+from
+  UI5LogInjectionConfiguration cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource
+where
+  cfg.hasFlowPath(source.getPathNode(), sink.getPathNode()) and
+  primarySource = source.getAPrimarySource()
+select sink, primarySource, sink, "Outbound network request depends on $@ log data.",
+  primarySource, "user-provided"

javascript/frameworks/ui5/src/UI5LogInjection/UI5LogsUsed.ql

@@ -0,0 +1,35 @@
+/**
+ * @name Access to user-controlled UI5 Logs
+ * @description Building log entries from user-controlled sources is vulnerable to
+ *              insertion of forged log entries by a malicious user.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 7.8
+ * @precision medium
+ * @id js/ui5-unsafe-log-access
+ * @tags security
+ *       external/cwe/cwe-117
+ */
+
+import javascript
+import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
+import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph
+import semmle.javascript.security.dataflow.LogInjectionQuery as LogInjection
+
+class UI5LogInjectionConfiguration extends LogInjection::LogInjectionConfiguration {
+  override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node node) {
+    node = ModelOutput::getASinkNode("ui5-log-injection").asSink()
+  }
+}
+
+from
+  UI5LogInjectionConfiguration cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource,
+  API::Node logListener
+where
+  cfg.hasFlowPath(source.getPathNode(), sink.getPathNode()) and
+  primarySource = source.getAPrimarySource() and
+  logListener = ModelOutput::getATypeNode("SapLogEntries")
+select sink, primarySource, sink, "Log entry depends on a $@ and is $@.",
+  primarySource, "user-provided value", logListener, "further accessed"

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-notifications/UI5LogInjection.expected

@@ -0,0 +1,15 @@
+nodes
+| webapp/controller/app.controller.js:9:17:9:27 | input: null |
+| webapp/controller/app.controller.js:15:17:15:52 | input |
+| webapp/controller/app.controller.js:15:25:15:52 | oModel. ... input') |
+| webapp/controller/app.controller.js:17:34:17:38 | input |
+| webapp/view/app.view.xml:6:5:8:28 | value={/input} |
+edges
+| webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/controller/app.controller.js:15:25:15:52 | oModel. ... input') |
+| webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.xml:6:5:8:28 | value={/input} |
+| webapp/controller/app.controller.js:15:17:15:52 | input | webapp/controller/app.controller.js:17:34:17:38 | input |
+| webapp/controller/app.controller.js:15:25:15:52 | oModel. ... input') | webapp/controller/app.controller.js:15:17:15:52 | input |
+| webapp/view/app.view.xml:6:5:8:28 | value={/input} | webapp/controller/app.controller.js:9:17:9:27 | input: null |
+| webapp/view/app.view.xml:6:5:8:28 | value={/input} | webapp/controller/app.controller.js:12:26:12:45 | new JSONModel(oData) |
+#select
+| webapp/controller/app.controller.js:17:34:17:38 | input | webapp/view/app.view.xml:6:5:8:28 | value={/input} | webapp/controller/app.controller.js:17:34:17:38 | input | Log entry depends on a $@. | webapp/view/app.view.xml:6:5:8:28 | value={/input} | user-provided value |

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-notifications/UI5LogsUsed.qlref

@@ -0,0 +1 @@
+UI5LogInjection/UI5LogsUsed.ql

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-notifications/UI5Xss.qlref

@@ -0,0 +1 @@
+UI5Xss/UI5Xss.ql

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-remote/UI5LogInjection.expected

@@ -0,0 +1,19 @@
+nodes
+| webapp/controller/app.controller.js:8:11:8:21 | input: null |
+| webapp/controller/app.controller.js:14:13:14:48 | input |
+| webapp/controller/app.controller.js:14:21:14:48 | oModel. ... input") |
+| webapp/controller/app.controller.js:16:30:16:34 | input |
+| webapp/view/app.view.xml:5:5:7:28 | value={/input} |
+| webapp/view/app.view.xml:8:5:8:37 | content={/output} |
+edges
+| webapp/controller/app.controller.js:8:11:8:21 | input: null | webapp/controller/app.controller.js:14:21:14:48 | oModel. ... input") |
+| webapp/controller/app.controller.js:8:11:8:21 | input: null | webapp/view/app.view.xml:5:5:7:28 | value={/input} |
+| webapp/controller/app.controller.js:9:11:9:22 | output: null | webapp/view/app.view.xml:8:5:8:37 | content={/output} |
+| webapp/controller/app.controller.js:11:22:11:41 | new JSONModel(oData) | webapp/view/app.view.xml:8:5:8:37 | content={/output} |
+| webapp/controller/app.controller.js:14:13:14:48 | input | webapp/controller/app.controller.js:16:30:16:34 | input |
+| webapp/controller/app.controller.js:14:21:14:48 | oModel. ... input") | webapp/controller/app.controller.js:14:13:14:48 | input |
+| webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:8:11:8:21 | input: null |
+| webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:11:22:11:41 | new JSONModel(oData) |
+| webapp/view/app.view.xml:8:5:8:37 | content={/output} | webapp/controller/app.controller.js:9:11:9:22 | output: null |
+#select
+| webapp/controller/app.controller.js:16:30:16:34 | input | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:16:30:16:34 | input | Log entry depends on a $@. | webapp/view/app.view.xml:5:5:7:28 | value={/input} | user-provided value |

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-remote/UI5LogsToHttp.expected

@@ -0,0 +1,36 @@
+nodes
+| webapp/controller/app.controller.js:8:11:8:21 | input: null |
+| webapp/controller/app.controller.js:14:13:14:48 | input |
+| webapp/controller/app.controller.js:14:21:14:48 | oModel. ... input") |
+| webapp/controller/app.controller.js:16:30:16:34 | input |
+| webapp/utils/CustomLogListener.js:9:29:9:34 | oEvent |
+| webapp/utils/CustomLogListener.js:13:19:13:24 | oEvent |
+| webapp/utils/CustomLogListener.js:13:19:13:32 | oEvent.message |
+| webapp/utils/LogEntriesToHttp.js:7:13:7:52 | message |
+| webapp/utils/LogEntriesToHttp.js:7:23:7:41 | Log.getLogEntries() |
+| webapp/utils/LogEntriesToHttp.js:7:23:7:44 | Log.get ... es()[0] |
+| webapp/utils/LogEntriesToHttp.js:7:23:7:52 | Log.get ... message |
+| webapp/utils/LogEntriesToHttp.js:11:19:11:25 | message |
+| webapp/view/app.view.xml:5:5:7:28 | value={/input} |
+| webapp/view/app.view.xml:8:5:8:37 | content={/output} |
+edges
+| webapp/controller/app.controller.js:8:11:8:21 | input: null | webapp/controller/app.controller.js:14:21:14:48 | oModel. ... input") |
+| webapp/controller/app.controller.js:8:11:8:21 | input: null | webapp/view/app.view.xml:5:5:7:28 | value={/input} |
+| webapp/controller/app.controller.js:9:11:9:22 | output: null | webapp/view/app.view.xml:8:5:8:37 | content={/output} |
+| webapp/controller/app.controller.js:11:22:11:41 | new JSONModel(oData) | webapp/view/app.view.xml:8:5:8:37 | content={/output} |
+| webapp/controller/app.controller.js:14:13:14:48 | input | webapp/controller/app.controller.js:16:30:16:34 | input |
+| webapp/controller/app.controller.js:14:21:14:48 | oModel. ... input") | webapp/controller/app.controller.js:14:13:14:48 | input |
+| webapp/controller/app.controller.js:16:30:16:34 | input | webapp/utils/CustomLogListener.js:9:29:9:34 | oEvent |
+| webapp/controller/app.controller.js:16:30:16:34 | input | webapp/utils/LogEntriesToHttp.js:7:23:7:41 | Log.getLogEntries() |
+| webapp/utils/CustomLogListener.js:9:29:9:34 | oEvent | webapp/utils/CustomLogListener.js:13:19:13:24 | oEvent |
+| webapp/utils/CustomLogListener.js:13:19:13:24 | oEvent | webapp/utils/CustomLogListener.js:13:19:13:32 | oEvent.message |
+| webapp/utils/LogEntriesToHttp.js:7:13:7:52 | message | webapp/utils/LogEntriesToHttp.js:11:19:11:25 | message |
+| webapp/utils/LogEntriesToHttp.js:7:23:7:41 | Log.getLogEntries() | webapp/utils/LogEntriesToHttp.js:7:23:7:44 | Log.get ... es()[0] |
+| webapp/utils/LogEntriesToHttp.js:7:23:7:44 | Log.get ... es()[0] | webapp/utils/LogEntriesToHttp.js:7:23:7:52 | Log.get ... message |
+| webapp/utils/LogEntriesToHttp.js:7:23:7:52 | Log.get ... message | webapp/utils/LogEntriesToHttp.js:7:13:7:52 | message |
+| webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:8:11:8:21 | input: null |
+| webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:11:22:11:41 | new JSONModel(oData) |
+| webapp/view/app.view.xml:8:5:8:37 | content={/output} | webapp/controller/app.controller.js:9:11:9:22 | output: null |
+#select
+| webapp/utils/CustomLogListener.js:13:19:13:32 | oEvent.message | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/utils/CustomLogListener.js:13:19:13:32 | oEvent.message | Outbound network request depends on $@ log data. | webapp/view/app.view.xml:5:5:7:28 | value={/input} | user-provided |
+| webapp/utils/LogEntriesToHttp.js:11:19:11:25 | message | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/utils/LogEntriesToHttp.js:11:19:11:25 | message | Outbound network request depends on $@ log data. | webapp/view/app.view.xml:5:5:7:28 | value={/input} | user-provided |

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-remote/UI5LogsToHttp.qlref

@@ -0,0 +1 @@
+UI5LogInjection/UI5LogsToHttp.ql

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-remote/UI5LogsUsed.expected

@@ -0,0 +1,20 @@
+nodes
+| webapp/controller/app.controller.js:8:11:8:21 | input: null |
+| webapp/controller/app.controller.js:14:13:14:48 | input |
+| webapp/controller/app.controller.js:14:21:14:48 | oModel. ... input") |
+| webapp/controller/app.controller.js:16:30:16:34 | input |
+| webapp/view/app.view.xml:5:5:7:28 | value={/input} |
+| webapp/view/app.view.xml:8:5:8:37 | content={/output} |
+edges
+| webapp/controller/app.controller.js:8:11:8:21 | input: null | webapp/controller/app.controller.js:14:21:14:48 | oModel. ... input") |
+| webapp/controller/app.controller.js:8:11:8:21 | input: null | webapp/view/app.view.xml:5:5:7:28 | value={/input} |
+| webapp/controller/app.controller.js:9:11:9:22 | output: null | webapp/view/app.view.xml:8:5:8:37 | content={/output} |
+| webapp/controller/app.controller.js:11:22:11:41 | new JSONModel(oData) | webapp/view/app.view.xml:8:5:8:37 | content={/output} |
+| webapp/controller/app.controller.js:14:13:14:48 | input | webapp/controller/app.controller.js:16:30:16:34 | input |
+| webapp/controller/app.controller.js:14:21:14:48 | oModel. ... input") | webapp/controller/app.controller.js:14:13:14:48 | input |
+| webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:8:11:8:21 | input: null |
+| webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:11:22:11:41 | new JSONModel(oData) |
+| webapp/view/app.view.xml:8:5:8:37 | content={/output} | webapp/controller/app.controller.js:9:11:9:22 | output: null |
+#select
+| webapp/controller/app.controller.js:16:30:16:34 | input | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:16:30:16:34 | input | Log entry depends on a $@ and is $@. | webapp/view/app.view.xml:5:5:7:28 | value={/input} | user-provided value | webapp/utils/CustomLogListener.js:9:29:9:34 | use moduleImport("sap/base/Log").getMember("exports").getMember("addLogListener").getParameter(0).getMember("onLogEntry").getParameter(0) | further accessed |
+| webapp/controller/app.controller.js:16:30:16:34 | input | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:16:30:16:34 | input | Log entry depends on a $@ and is $@. | webapp/view/app.view.xml:5:5:7:28 | value={/input} | user-provided value | webapp/utils/LogEntriesToHttp.js:7:23:7:41 | use moduleImport("sap/base/Log").getMember("exports").getMember("getLogEntries").getReturn() | further accessed |

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-remote/UI5LogsUsed.qlref

@@ -0,0 +1 @@
+UI5LogInjection/UI5LogsUsed.ql

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-remote/webapp/utils/LogEntriesToHttp.js

@@ -0,0 +1,15 @@
+sap.ui.define(
+  ["sap/ui/base/Object", "sap/base/Log"],
+  function (BaseObject, Log) {
+    "use strict";
+    return BaseObject.extend("codeql-sap-js.log.CustomLogListener", {
+      constructor: function () {
+        let message = Log.getLogEntries()[0].message;
+        const http = new XMLHttpRequest();
+        const url = "https://some.remote.server/location";
+        http.open("POST", url);
+        http.send(message);
+      },
+    });
+  },
+);