Remove FPs of js/ui5-log-injection-to-http

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

@@ -23,9 +23,7 @@ extensions:
       - ["SapLogger", "sap/base/Log", ""]
       - ["SapLogger", "global", "Member[sap].Member[base].Member[Log]"]
       - ["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[addLogListener].Argument[0].Member[onLogEntry].Parameter[0]"]
       - ["SapLogEntries", "SapLogger", "Member[getLogEntries].ReturnValue"]
       - ["ResourceBundle", "ResourceBundle", "Instance"]
       - ["ResourceBundle", "sap/base/i18n/ResourceBundle", ""]

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll

@@ -155,7 +155,8 @@ abstract class UserModule extends CallExpr {
 class SapDefineModule extends AmdModuleDefinition::Range, MethodCallExpr, UserModule {
   SapDefineModule() {
     /*
-     * NOTE: This only matches a call to the dot expression `sap.ui.define`, and does not consider a flow among `sap`, `ui`, and `define`.
+     * NOTE: This only matches a call to the dot expression `sap.ui.define`, and does not
+     * consider a flow among `sap`, `ui`, and `define`.
      */
 
     exists(GlobalVarAccess sap, DotExpr sapUi, DotExpr sapUiDefine |
@@ -220,50 +221,11 @@ class JQueryDefineModule extends UserModule, MethodCallExpr {
   }
 }
 
-private SourceNode sapControl(TypeTracker t) {
-  t.start() and
-  exists(UserModule d, string dependencyType |
-    dependencyType = ["sap/ui/core/Control", "sap.ui.core.Control"]
-  |
-    d.getADependency() = dependencyType and
-    result = d.getRequiredObject(dependencyType).asSourceNode()
-  )
-  or
-  exists(TypeTracker t2 | result = sapControl(t2).track(t2, t))
-}
-
-private SourceNode sapControl() { result = sapControl(TypeTracker::end()) }
-
-private SourceNode sapController(TypeTracker t) {
-  t.start() and
-  exists(UserModule d, string dependencyType |
-    dependencyType = ["sap/ui/core/mvc/Controller", "sap.ui.core.mvc.Controller"]
-  |
-    d.getADependency() = dependencyType and
-    result = d.getRequiredObject(dependencyType).asSourceNode()
-  )
-  or
-  exists(TypeTracker t2 | result = sapController(t2).track(t2, t))
-}
-
-private SourceNode sapController() { result = sapController(TypeTracker::end()) }
-
-private SourceNode sapRenderer(TypeTracker t) {
-  t.start() and
-  exists(UserModule d, string dependencyType |
-    dependencyType = ["sap/ui/core/Renderer", "sap.ui.core.Renderer"]
-  |
-    d.getADependency() = dependencyType and
-    result = d.getRequiredObject(dependencyType).asSourceNode()
-  )
-  or
-  exists(TypeTracker t2 | result = sapController(t2).track(t2, t))
-}
-
-private SourceNode sapRenderer() { result = sapRenderer(TypeTracker::end()) }
-
-private class Renderer extends SapExtendCall {
-  Renderer() { this.getReceiver().getALocalSource() = sapRenderer() }
+class Renderer extends SapExtendCall {
+  Renderer() {
+    this.getReceiver().getALocalSource() =
+      TypeTrackers::hasDependency(["sap/ui/core/Renderer", "sap.ui.core.Renderer"])
+  }
 
   FunctionNode getRenderer() {
     /* 1. Old API */
@@ -276,7 +238,8 @@ private class Renderer extends SapExtendCall {
 
 class CustomControl extends SapExtendCall {
   CustomControl() {
-    this.getReceiver().getALocalSource() = sapControl() or
+    this.getReceiver().getALocalSource() =
+      TypeTrackers::hasDependency(["sap/ui/core/mvc/Controller", "sap.ui.core.mvc.Controller"]) or
     exists(SapDefineModule sapModule | this.getDefine() = sapModule.getExtendingModule())
   }
 
@@ -450,7 +413,8 @@ class CustomController extends SapExtendCall {
   string name;
 
   CustomController() {
-    this.getReceiver().getALocalSource() = sapController() and
+    this.getReceiver().getALocalSource() =
+      TypeTrackers::hasDependency(["sap/ui/core/mvc/Controller", "sap.ui.core.mvc.Controller"]) and
     name = this.getFile().getBaseName().regexpCapture("([a-zA-Z0-9]+).[cC]ontroller.js", 1)
   }
 
@@ -772,35 +736,24 @@ abstract class UI5InternalModel extends UI5Model, NewNode {
   abstract string getPathString(Property property);
 }
 
-/**
- * Represents models that are loaded from an external source, e.g. OData service.
- * It is the value flowing to a `setModel` call in a handler of a `CustomController` (which is represented by `ControllerHandler`), since it is the closest we can get to the actual model itself.
- */
-private SourceNode sapComponent(TypeTracker t) {
-  t.start() and
-  exists(UserModule d, string dependencyType |
-    dependencyType =
-      [
-        "sap/ui/core/mvc/Component", "sap.ui.core.mvc.Component", "sap/ui/core/UIComponent",
-        "sap.ui.core.UIComponent"
-      ]
-  |
-    d.getADependency() = dependencyType and
-    result = d.getRequiredObject(dependencyType).asSourceNode()
-  )
-  or
-  exists(TypeTracker t2 | result = sapComponent(t2).track(t2, t))
-}
-
-private SourceNode sapComponent() { result = sapComponent(TypeTracker::end()) }
-
 import ManifestJson
 
 /**
  * A UI5 Component that may contain other controllers or controls.
  */
 class Component extends SapExtendCall {
-  Component() { this.getReceiver().getALocalSource() = sapComponent() }
+  Component() {
+    this.getReceiver().getALocalSource() =
+      /*
+       * Represents models that are loaded from an external source, e.g. OData service.
+       * It is the value flowing to a `setModel` call in a handler of a `CustomController` (which is represented by `ControllerHandler`), since it is the closest we can get to the actual model itself.
+       */
+
+      TypeTrackers::hasDependency([
+          "sap/ui/core/mvc/Component", "sap.ui.core.mvc.Component", "sap/ui/core/UIComponent",
+          "sap.ui.core.UIComponent"
+        ])
+  }
 
   string getId() { result = this.getName().regexpCapture("([a-zA-Z0-9.]+).Component", 1) }
 
@@ -1490,3 +1443,19 @@ class PropertyMetadata extends ObjectLiteralNode {
     inSameWebApp(this.getFile(), result.getFile())
   }
 }
+
+module TypeTrackers {
+  private SourceNode hasDependency(TypeTracker t, string dependencyPath) {
+    t.start() and
+    exists(UserModule d |
+      d.getADependency() = dependencyPath and
+      result = d.getRequiredObject(dependencyPath).asSourceNode()
+    )
+    or
+    exists(TypeTracker t2 | result = hasDependency(t2, dependencyPath).track(t2, t))
+  }
+
+  SourceNode hasDependency(string dependencyPath) {
+    result = hasDependency(TypeTracker::end(), dependencyPath)
+  }
+}

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogInjectionQuery.qll

@@ -0,0 +1,12 @@
+import javascript
+import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
+private 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()
+  }
+}

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

@@ -342,19 +342,23 @@ 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) {
-    inSameWebApp(start.getFile(), end.getFile()) and
-    start =
-      ModelOutput::getATypeNode("SapLogger")
-          .getMember(["debug", "error", "fatal", "info", "trace", "warning"])
-          .getACall()
-          .getAnArgument() and
-    end = ModelOutput::getATypeNode("SapLogEntries").asSource()
-  }
-}
+// /**
+//  * 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 {
+//   deprecated override predicate step(
+//     DataFlow::Node start, DataFlow::Node end, DataFlow::FlowLabel preLabel,
+//     DataFlow::FlowLabel postLabel
+//   ) {
+//     inSameWebApp(start.getFile(), end.getFile()) and
+//     start =
+//       ModelOutput::getATypeNode("SapLogger")
+//           .getMember(["debug", "error", "fatal", "info", "trace", "warning"])
+//           .getACall()
+//           .getAnArgument() and
+//     end = ModelOutput::getATypeNode("SapLogEntries").asSource() and
+//     preLabel = "logged" and
+//     postLabel = "accessed"
+//   }
+// }

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

@@ -12,17 +12,7 @@
  */
 
 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()
-  }
-}
+import advanced_security.javascript.frameworks.ui5.UI5LogInjectionQuery
 
 from
   UI5LogInjectionConfiguration cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource

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

@@ -13,24 +13,80 @@
 
 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
+import semmle.javascript.frameworks.data.internal.ApiGraphModels
+import advanced_security.javascript.frameworks.ui5.UI5LogInjectionQuery
 
-class UI5LogInjectionConfiguration extends LogInjection::LogInjectionConfiguration {
-  override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node node) {
+class ClientRequestInjectionVector extends DataFlow::Node {
+  ClientRequestInjectionVector() {
     exists(ClientRequest req |
-      node = req.getUrl() or
-      node = req.getADataNode()
+      this = req.getUrl() or
+      this = 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,
+class UI5LogEntryFlowState extends DataFlow::FlowLabel {
+  UI5LogEntryFlowState() {
+    this = ["not-logged-not-accessed", "logged-not-accessed", "logged-and-accessed"]
+  }
+}
+
+class UI5LogEntryToHttp extends TaintTracking::Configuration {
+  UI5LogEntryToHttp() { this = "UI5 Log Entry included in an outbound HTTP request" }
+
+  override predicate isSource(DataFlow::Node node, DataFlow::FlowLabel state) {
+    node instanceof RemoteFlowSource and
+    state = "not-logged-not-accessed"
+  }
+
+  override predicate isAdditionalFlowStep(
+    DataFlow::Node start, DataFlow::Node end, DataFlow::FlowLabel preState,
+    DataFlow::FlowLabel postState
+  ) {
+    exists(UI5LogInjectionConfiguration cfg |
+      cfg.isAdditionalFlowStep(start, end) and
+      preState = postState
+    )
+    or
+    inSameWebApp(start.getFile(), end.getFile()) and
+    start =
+      ModelOutput::getATypeNode("SapLogger")
+          .getMember(["debug", "error", "fatal", "info", "trace", "warning"])
+          .getACall()
+          .getAnArgument() and
+    end = ModelOutput::getATypeNode("SapLogEntries").asSource() and
+    preState = "not-logged-not-accessed" and
+    postState = "logged-and-accessed"
+  }
+
+  override predicate isSink(DataFlow::Node node, DataFlow::FlowLabel state) {
+    node instanceof ClientRequestInjectionVector and
+    state = "logged-and-accessed"
+  }
+}
+
+/**
+ * Config without states for sanity check
+ */
+module UI5LogEntryToHttp implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
+
+  predicate isAdditionalFlowStep(DataFlow::Node start, DataFlow::Node end) {
+    inSameWebApp(start.getFile(), end.getFile()) and
+    start =
+      ModelOutput::getATypeNode("SapLogger")
+          .getMember(["debug", "error", "fatal", "info", "trace", "warning"])
+          .getACall()
+          .getAnArgument() and
+    end = ModelOutput::getATypeNode("SapLogEntries").asSource()
+  }
+
+  predicate isSink(DataFlow::Node node) { node instanceof ClientRequestInjectionVector }
+}
+
+import DataFlow::PathGraph
+
+from UI5LogEntryToHttp cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink, source, sink, "Outbound network request depends on $@ log data.", source,
   "user-provided"

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

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

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-http-getLogEntries/package.json

@@ -0,0 +1,5 @@
+{
+  "name": "sap-ui5-xss",
+  "version": "1.0.0",
+  "main": "index.js"
+}

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-http-getLogEntries/ui5.yaml

@@ -0,0 +1,7 @@
+specVersion: '3.0'
+metadata:
+  name: sap-ui5-xss
+type: application
+framework:
+  name: SAPUI5
+  version: "1.115.0"

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-http-getLogEntries/webapp/controller/app.controller.js

@@ -0,0 +1,22 @@
+sap.ui.define(
+  [
+    "sap/ui/core/mvc/Controller",
+    "sap/ui/model/json/JSONModel",
+  ],
+  function (Controller, JSONModel) {
+    "use strict";
+    return Controller.extend("codeql-sap-js.controller.app", {
+      onInit: function () {
+        var oData = {
+          input: null,
+          output: null,
+        };
+        var oModel = new JSONModel(oData);
+        this.getView().setModel(oModel);
+
+        var input = oModel.getProperty("/input");
+        jQuery.sap.log.debug(input); //log-injection sink
+      },
+    });
+  },
+);