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,10 +1,10 @@
 /**
- * @name UI5 Log injection
+ * @name UI5 client-side Log injection
  * @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 error
- * @security-severity 7.8
+ * @problem.severity recommendation
+ * @security-severity 3.5
  * @precision medium
  * @id js/ui5-log-injection
  * @tags security

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,63 @@
+/**
+ * @name Access to user-controlled UI5 Logs
+ * @description Log entries from user-controlled sources should not be further processed.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 5
+ * @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()
+  }
+}
+
+newtype TLogEntriesNode =
+  TDataFlowNode(DataFlow::Node node) or
+  TUI5ControlNode(UI5Control node)
+
+class LogEntriesNode extends TLogEntriesNode {
+  DataFlow::Node asDataFlowNode() {
+    this = TDataFlowNode(result) and
+    result = ModelOutput::getATypeNode("SapLogEntries").getInducingNode()
+  }
+
+  UI5Control asUI5ControlNode() {
+    this = TUI5ControlNode(result) and
+    result.getImportPath() = "sap/ui/vk/Notifications"
+  }
+
+  string toString() {
+    result = this.asDataFlowNode().toString()
+    or
+    result = "UI5 control " + this.asUI5ControlNode().toString()
+  }
+
+  predicate hasLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    this.asDataFlowNode().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    or
+    this.asUI5ControlNode().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+  }
+}
+
+from
+  UI5LogInjectionConfiguration cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource,
+  LogEntriesNode logEntries
+where
+  cfg.hasFlowPath(source.getPathNode(), sink.getPathNode()) and
+  primarySource = source.getAPrimarySource()
+select logEntries, primarySource, sink, "Processing UI5 log entries that depend on $@.",
+  primarySource, "user-provided data", logEntries, logEntries.toString()

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.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/view/app.view.xml:5:9:24:9 | UI5 control Notifications | webapp/view/app.view.xml:6:5:8:28 | value={/input} | webapp/controller/app.controller.js:17:34:17:38 | input | Processing UI5 log entries that depend on $@. | webapp/view/app.view.xml:6:5:8:28 | value={/input} | user-provided data | webapp/view/app.view.xml:5:9:24:9 | UI5 control Notifications | UI5 control Notifications |

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/webapp/view/app.view.xml

@@ -6,5 +6,5 @@
     <Input placeholder="Enter Payload" 
         description="Try: &lt;img src=x onerror=alert(&quot;XSS&quot;)&gt;" 
         value="{/input}" />  <!--User input source sap.m.Input.value -->
-    <vk:Notifications/>
+    <vk:Notifications/> <!-- js/ui5-unsafe-log-access -->
 </mvc:View>

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/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:15:30:15: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:15:30:15: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:15:30:15:34 | input | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:15:30:15: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-sinks/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:15:30:15: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:15:30:15: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:15:30:15:34 | input | webapp/utils/CustomLogListener.js:9:29:9:34 | oEvent |
+| webapp/controller/app.controller.js:15:30:15: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-sinks/UI5LogsToHttp.qlref

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