Skip to content

Commit c36405e

Browse files
data-douserdata-douser
committed
UI5 XSS detect default OData model w/ bindElement
The setModel check in UI5BindingPath.getASource() was globally scoped, causing detection to fail when apps with explicit setModel calls were in the same database as apps using default OData models from manifest.json. Changes: - Bindings.qll: Add getBindElementCall() method to Binding class - RemoteFlowSources.qll: Fix DefaultODataServiceModel.asBinding() to use getBindElementCall() for proper context binding matching - UI5View.qll: Scope setModel check to same webapp using inSameWebApp() - Add test case xss-fragment-odata-default-model validating the fix All 26 UI5Xss tests pass.
1 parent 9fac18b commit c36405e

14 files changed

Lines changed: 180 additions & 3 deletions

File tree

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -748,4 +748,9 @@ class Binding extends TBinding {
748748
BindingPath getBindingPath() { result.getBinding() = this }
749749

750750
BindingTarget getBindingTarget() { result.getBinding() = this }
751+
752+
/**
753+
* Gets the `BindElementMethodCallNode` for this binding, if it is a context binding via `bindElement`.
754+
*/
755+
BindElementMethodCallNode getBindElementCall() { this = TLateJavaScriptContextBinding(result, _) }
751756
}

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,12 @@ class DefaultODataServiceModel extends UI5ExternalModel {
126126

127127
override string getName() { result = "" }
128128

129-
Binding asBinding() { result.getBindingTarget().asDataFlowNode() = this }
129+
/**
130+
* Gets bindings associated with this default OData model source.
131+
* Since `DefaultODataServiceModel` represents a `bindElement` call,
132+
* we match context bindings whose `bindElement` call is this node.
133+
*/
134+
Binding asBinding() { result.getBindElementCall() = this }
130135
}
131136

132137
/** Model which gains content from an SAP OData service. */

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,10 +133,13 @@ abstract class UI5BindingPath extends BindingPath {
133133
not exists(this.getModelName())
134134
)
135135
or
136-
/* 5. There is no call to `setModel` at all and a default model exists that is related to the binding path this refers to */
136+
/* 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 */
137137
exists(DefaultODataServiceModel defaultModel |
138138
result = defaultModel and
139-
not exists(MethodCallNode viewSetModelCall | viewSetModelCall.getMethodName() = "setModel") and
139+
not exists(MethodCallNode viewSetModelCall |
140+
viewSetModelCall.getMethodName() = "setModel" and
141+
inSameWebApp(this.getLocation().getFile(), viewSetModelCall.getFile())
142+
) and
140143
/*
141144
* this binding path can occur in a fragment that is the receiver object for the bindElement model approximation
142145
* i.e. checks that the default model is relevant

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-odata-default-model/UI5Xss.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
nodes
2+
| webapp/controller/App.controller.js:21:17:21:53 | oFragme ... es(1)") |
3+
| webapp/fragments/DataDisplay.fragment.xml:7:9:7:42 | content={message} |
4+
edges
5+
| webapp/controller/App.controller.js:21:17:21:53 | oFragme ... es(1)") | webapp/fragments/DataDisplay.fragment.xml:7:9:7:42 | content={message} |
6+
| webapp/fragments/DataDisplay.fragment.xml:7:9:7:42 | content={message} | webapp/controller/App.controller.js:21:17:21:53 | oFragme ... es(1)") |
7+
#select
8+
| webapp/fragments/DataDisplay.fragment.xml:7:9:7:42 | content={message} | webapp/controller/App.controller.js:21:17:21:53 | oFragme ... es(1)") | webapp/fragments/DataDisplay.fragment.xml:7:9:7:42 | content={message} | XSS vulnerability due to $@. | webapp/controller/App.controller.js:21:17:21:53 | oFragme ... es(1)") | user-provided value |

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-odata-default-model/UI5Xss.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
UI5Xss/UI5Xss.ql

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-odata-default-model/package.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"name": "xss-fragment-odata-default-model",
3+
"version": "1.0.0",
4+
"description": "Test case for XSS vulnerability via default OData model in fragment with bindElement"
5+
}

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-odata-default-model/ui5.yaml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
specVersion: "3.1"
2+
type: application
3+
metadata:
4+
name: xss-fragment-odata-default-model
5+
framework:
6+
name: SAPUI5
7+
version: "1.120.0"

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-odata-default-model/webapp/Component.js‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
sap.ui.define([
2+
"sap/ui/core/UIComponent"
3+
], function (UIComponent) {
4+
"use strict";
5+
return UIComponent.extend("xss.fragment.odata.defaultmodel.Component", {
6+
metadata: {
7+
manifest: "json"
8+
},
9+
init: function () {
10+
UIComponent.prototype.init.apply(this, arguments);
11+
}
12+
});
13+
});

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-odata-default-model/webapp/controller/App.controller.js‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
sap.ui.define([
2+
"sap/ui/core/mvc/Controller",
3+
"sap/ui/core/Fragment"
4+
], function (Controller, Fragment) {
5+
"use strict";
6+
7+
return Controller.extend("xss.fragment.odata.defaultmodel.controller.App", {
8+
onInit: function () {
9+
// XSS vulnerability pattern:
10+
// 1. OData model is configured as default model in manifest.json
11+
// 2. Fragment is loaded dynamically
12+
// 3. Fragment is bound to OData entity via bindElement
13+
// 4. Fragment contains HTML control that renders OData content
14+
15+
Fragment.load({
16+
id: this.getView().getId(),
17+
name: "xss.fragment.odata.defaultmodel.fragments.DataDisplay",
18+
controller: this
19+
}).then(function (oFragment) {
20+
// Bind fragment to an OData entity - vulnerability is in the backend data
21+
oFragment.bindElement("/Messages(1)");
22+
23+
// Add the fragment to the page content
24+
this.byId("page").addContent(oFragment);
25+
}.bind(this));
26+
}
27+
});
28+
});

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-odata-default-model/webapp/fragments/DataDisplay.fragment.xml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<core:FragmentDefinition
2+
xmlns="sap.m"
3+
xmlns:core="sap.ui.core">
4+
<VBox class="sapUiSmallMargin">
5+
<Label text="Message from OData:" />
6+
<!-- XSS vulnerability: HTML content bound to OData property containing unsanitized data -->
7+
<core:HTML content="{message}" />
8+
</VBox>
9+
</core:FragmentDefinition>

0 commit comments

Comments
 (0)