Merge pull request #190 from advanced-security/jeongsoolee09/fix-ui5-loginjection

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

Author: jeongsoolee09

.github/workflows/javascript.sarif.expected

@@ -22,10 +22,9 @@ extensions:
       - ["Assert", "global", "Member[jQuery].Member[sap].Member[assert]"]
       - ["SapLogger", "sap/base/Log", ""]
       - ["SapLogger", "global", "Member[sap].Member[base].Member[Log]"]
+      - ["SapLogger", "SapLogger", "Member[getLogger].ReturnValue"]
       - ["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/ext/ui5.model.yml

@@ -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/Control", "sap.ui.core.Control"]) 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/UI5.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/UI5LogInjectionQuery.qll

@@ -12,17 +12,8 @@
  */
 
 import javascript
-import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
+import advanced_security.javascript.frameworks.ui5.UI5LogInjectionQuery
 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

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

@@ -13,22 +13,66 @@
 
 import javascript
 import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
+import semmle.javascript.frameworks.data.internal.ApiGraphModels
+import advanced_security.javascript.frameworks.ui5.UI5LogInjectionQuery
 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) {
+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
+class UI5LogEntryFlowState extends DataFlow::FlowLabel {
+  UI5LogEntryFlowState() { this = ["not-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
+    /*
+     * NOTE: This disjunct is a labeled version of LogArgumentToListener in
+     * FlowSteps.qll, a DataFlow::SharedFlowStep. As the class is considered
+     * legacy on version 2.4.0, we leave the two here (labeled) and there
+     * (unlabeled). This is something we should also tidy up when we migrate
+     * to the newer APIs.
+     */
+
+    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"
+  }
+}
+
+from UI5LogEntryToHttp cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource
 where
   cfg.hasFlowPath(source.getPathNode(), sink.getPathNode()) and
   primarySource = source.getAPrimarySource()

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

@@ -1,4 +1,3 @@
-WARNING: type 'LogInjectionConfiguration' has been deprecated and may be removed in future (UI5LogInjection.ql:19,44-83)
 nodes
 | LogInjectionTest.js:6:9:6:50 | value |
 | LogInjectionTest.js:6:17:6:50 | jQuery. ... param") |

javascript/frameworks/ui5/test/queries/UI5LogInjection/avoid-duplicate-alerts/UI5LogInjection.expected

@@ -1,4 +1,3 @@
-WARNING: type 'LogInjectionConfiguration' has been deprecated and may be removed in future (UI5LogInjection.ql:19,44-83)
 nodes
 | webapp/control/xss.js:7:23:7:37 | { type: "int" } |
 | webapp/control/xss.js:13:38:13:55 | oControl.getText() |

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-custom-control-property-sanitized/UI5LogInjection.expected

@@ -1,4 +1,3 @@
-WARNING: type 'LogInjectionConfiguration' has been deprecated and may be removed in future (UI5LogInjection.ql:19,44-83)
 nodes
 | webapp/control/xss.js:8:23:8:40 | { type: "string" } |
 | webapp/control/xss.js:15:21:15:46 | value |

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-custom-control-sanitized/UI5LogInjection.expected

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

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

@@ -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/package-lock.json

@@ -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/package.json

@@ -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
+      },
+    });
+  },
+);

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

@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+
+<head>
+
+    <meta charset="utf-8">
+    <title>SAPUI5 XSS</title>
+    <script src="https://sdk.openui5.org/resources/sap-ui-core.js" 
+        data-sap-ui-libs="sap.m"
+        data-sap-ui-onInit="module:codeql-sap-js/index" 
+        data-sap-ui-resourceroots='{
+            "codeql-sap-js": "./"
+        }'>
+        </script>
+</head>
+
+<body class="sapUiBody" id="content">
+
+</body>
+
+</html>

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

@@ -0,0 +1,11 @@
+sap.ui.define([
+    "sap/ui/core/mvc/XMLView"
+], function (XMLView) {
+    "use strict";
+    XMLView.create({
+        viewName: "codeql-sap-js.view.app"
+    }).then(function (oView) {
+        oView.placeAt("content");
+    });
+
+}); 

javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-http-getLogEntries/webapp/index.html

@@ -0,0 +1,5 @@
+{
+    "sap.app": {
+        "id": "sap-ui5-xss"
+    }
+}