Merge pull request #283 from advanced-security/dd/default-odata-model-fragments

Improve `UI5Xss.ql` query to detect default OData model with `bindElement`

Author: data-douser

.github/workflows/javascript.sarif.expected

@@ -748,4 +748,9 @@ class Binding extends TBinding {
   BindingPath getBindingPath() { result.getBinding() = this }
 
   BindingTarget getBindingTarget() { result.getBinding() = this }
+
+  /**
+   * Gets the `BindElementMethodCallNode` for this binding, if it is a context binding via `bindElement`.
+   */
+  BindElementMethodCallNode getBindElementCall() { this = TLateJavaScriptContextBinding(result, _) }
 }

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

@@ -114,19 +114,26 @@ abstract class UI5ExternalModel extends UI5Model, RemoteFlowSource {
 class DefaultODataServiceModel extends UI5ExternalModel {
   DefaultODataServiceModel() {
     exists(ExternalModelManifest model |
-      //an OData default model exists
+      // An OData default model exists.
       model.getName() = "" and
       model.getDataSource() instanceof ODataDataSourceManifest and
-      //therefore the bindElement calls that exist may be sources and also approximates the model itself
-      this.getCalleeName() = "bindElement"
+      // A bindElement call bound to the default OData model represents a source of data.
+      this.getCalleeName() = "bindElement" and
+      // The bindElement call must be in the same webapp as the manifest that declares the default model.
+      inSameWebApp(this.getFile(), model.getJsonFile())
     )
   }
 
   override string getSourceType() { result = "DefaultODataServiceModel" }
 
   override string getName() { result = "" }
 
-  Binding asBinding() { result.getBindingTarget().asDataFlowNode() = this }
+  /**
+   * Gets bindings associated with this default OData model source.
+   * Since `DefaultODataServiceModel` represents a `bindElement` call,
+   * we match context bindings whose `bindElement` call is this node.
+   */
+  Binding asBinding() { result.getBindElementCall() = this }
 }
 
 /** Model which gains content from an SAP OData service. */

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

@@ -88,6 +88,10 @@ bindingset[f1, f2]
 pragma[inline_late]
 predicate inSameWebApp(File f1, File f2) {
   exists(WebApp webApp | webApp.getAResource() = f1 and webApp.getAResource() = f2)
+  or
+  exists(WebApp webApp | webApp.getManifest() = f1 and webApp.getAResource() = f2)
+  or
+  exists(WebApp webApp | webApp.getManifest() = f2 and webApp.getAResource() = f1)
 }
 
 /** A UI5 bootstrapped web application. */
@@ -944,16 +948,23 @@ module ManifestJson {
       exists(JsonObject models |
         this = models.getPropValue(modelName) and
         dataSourceName = this.getPropStringValue("dataSource") and
-        /* This data source can be found in the "dataSources" property */
-        exists(DataSourceManifest dataSource | dataSource.getName() = dataSourceName)
+        /* This data source can be found in the "dataSources" property of the same manifest */
+        exists(DataSourceManifest dataSource |
+          dataSource.getName() = dataSourceName and
+          dataSource.getManifestJson() = this.getJsonFile()
+        )
       )
     }
 
     string getName() { result = modelName }
 
     string getDataSourceName() { result = dataSourceName }
 
-    DataSourceManifest getDataSource() { result.getName() = dataSourceName }
+    /** Gets the data source for this external model from the same manifest file. */
+    DataSourceManifest getDataSource() {
+      result.getName() = dataSourceName and
+      result.getManifestJson() = this.getJsonFile()
+    }
   }
 
   class ManifestJson extends File {

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

@@ -133,10 +133,13 @@ abstract class UI5BindingPath extends BindingPath {
         not exists(this.getModelName())
       )
       or
-      /* 5.  There is no call to `setModel` at all and a default model exists that is related to the binding path this refers to */
+      /* 5.  There is no call to `setModel` in the same webapp and a default model exists that is related to the binding path this refers to */
       exists(DefaultODataServiceModel defaultModel |
         result = defaultModel and
-        not exists(MethodCallNode viewSetModelCall | viewSetModelCall.getMethodName() = "setModel") and
+        not exists(MethodCallNode viewSetModelCall |
+          viewSetModelCall.getMethodName() = "setModel" and
+          inSameWebApp(this.getLocation().getFile(), viewSetModelCall.getFile())
+        ) and
         /*
          * this binding path can occur in a fragment that is the receiver object for the bindElement model approximation
          * i.e. checks that the default model is relevant
@@ -147,7 +150,9 @@ abstract class UI5BindingPath extends BindingPath {
           load.getNameArgument()
               .getStringValue()
               .matches("%" +
-                  this.getLocation().getFile().getBaseName().replaceAll(".fragment.xml", "") + "%")
+                  this.getLocation().getFile().getBaseName().replaceAll(".fragment.xml", "") + "%") and
+          // The fragment load call must be in the same webapp as the fragment file
+          inSameWebApp(this.getLocation().getFile(), load.getFile())
         )
       )
     )

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

@@ -0,0 +1,13 @@
+nodes
+| webapp_a/controller/App.controller.js:21:17:21:54 | oFragme ... sA(1)") |
+| webapp_a/fragments/DataDisplay.fragment.xml:7:9:7:43 | content={messageA} |
+| webapp_b/controller/App.controller.js:21:17:21:54 | oFragme ... sB(1)") |
+| webapp_b/fragments/DataDisplay.fragment.xml:7:9:7:43 | content={messageB} |
+edges
+| webapp_a/controller/App.controller.js:21:17:21:54 | oFragme ... sA(1)") | webapp_a/fragments/DataDisplay.fragment.xml:7:9:7:43 | content={messageA} |
+| webapp_a/fragments/DataDisplay.fragment.xml:7:9:7:43 | content={messageA} | webapp_a/controller/App.controller.js:21:17:21:54 | oFragme ... sA(1)") |
+| webapp_b/controller/App.controller.js:21:17:21:54 | oFragme ... sB(1)") | webapp_b/fragments/DataDisplay.fragment.xml:7:9:7:43 | content={messageB} |
+| webapp_b/fragments/DataDisplay.fragment.xml:7:9:7:43 | content={messageB} | webapp_b/controller/App.controller.js:21:17:21:54 | oFragme ... sB(1)") |
+#select
+| webapp_a/fragments/DataDisplay.fragment.xml:7:9:7:43 | content={messageA} | webapp_a/controller/App.controller.js:21:17:21:54 | oFragme ... sA(1)") | webapp_a/fragments/DataDisplay.fragment.xml:7:9:7:43 | content={messageA} | XSS vulnerability due to $@. | webapp_a/controller/App.controller.js:21:17:21:54 | oFragme ... sA(1)") | user-provided value |
+| webapp_b/fragments/DataDisplay.fragment.xml:7:9:7:43 | content={messageB} | webapp_b/controller/App.controller.js:21:17:21:54 | oFragme ... sB(1)") | webapp_b/fragments/DataDisplay.fragment.xml:7:9:7:43 | content={messageB} | XSS vulnerability due to $@. | webapp_b/controller/App.controller.js:21:17:21:54 | oFragme ... sB(1)") | user-provided value |

javascript/frameworks/ui5/test/queries/UI5Xss/xss-cross-app-isolation/UI5Xss.expected

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

javascript/frameworks/ui5/test/queries/UI5Xss/xss-cross-app-isolation/UI5Xss.qlref

@@ -0,0 +1,5 @@
+{
+  "name": "xss-cross-app-isolation",
+  "version": "1.0.0",
+  "description": "Test case for ensuring XSS detection does not cross webapp boundaries with default OData models"
+}

javascript/frameworks/ui5/test/queries/UI5Xss/xss-cross-app-isolation/package.json

@@ -0,0 +1,7 @@
+specVersion: "3.1"
+type: application
+metadata:
+  name: xss-cross-app-isolation
+framework:
+  name: SAPUI5
+  version: "1.120.0"

javascript/frameworks/ui5/test/queries/UI5Xss/xss-cross-app-isolation/ui5.yaml

@@ -0,0 +1,13 @@
+sap.ui.define([
+    "sap/ui/core/UIComponent"
+], function (UIComponent) {
+    "use strict";
+    return UIComponent.extend("app.a.Component", {
+        metadata: {
+            manifest: "json"
+        },
+        init: function () {
+            UIComponent.prototype.init.apply(this, arguments);
+        }
+    });
+});