Skip to content

Commit bb9452e

Browse files
data-douserdata-douser
committed
More potential detection improvements for UI5 XSS
1 parent 530df03 commit bb9452e

13 files changed

Lines changed: 192 additions & 19 deletions

File tree

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,3 +173,22 @@ private class UI5ExtRemoteSource extends RemoteFlowSource {
173173
result = "Remote flow" // Don't discriminate between UI5-specific remote flows and vanilla ones
174174
}
175175
}
176+
177+
/**
178+
* URLSearchParams.get() and getAll() return URL query parameter values which are user-controlled.
179+
* e.g., `new URLSearchParams(window.location.search).get("param")`
180+
*/
181+
private class UrlSearchParamsSource extends RemoteFlowSource {
182+
UrlSearchParamsSource() {
183+
exists(DataFlow::NewNode newCall, DataFlow::MethodCallNode getCall |
184+
// Match: new URLSearchParams(...)
185+
newCall.getCalleeName() = "URLSearchParams" and
186+
// Match: .get() or .getAll() on the URLSearchParams instance
187+
getCall.getMethodName() = ["get", "getAll"] and
188+
newCall.flowsTo(getCall.getReceiver()) and
189+
this = getCall
190+
)
191+
}
192+
193+
override string getSourceType() { result = "URL query parameter" }
194+
}

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

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,10 @@ private module WebAppResourceRootJsonReader implements JsonParser::MakeJsonReade
1111
class JsonReader extends WebApp {
1212
string getJson() {
1313
// We match on the lowercase to cover all the possible variants of writing the attribute name.
14+
// Support both "data-sap-ui-resourceroots" and "data-sap-ui-resource-roots" (with hyphen)
1415
exists(string resourceRootAttributeName |
15-
resourceRootAttributeName.toLowerCase() = "data-sap-ui-resourceroots"
16+
resourceRootAttributeName.toLowerCase() =
17+
["data-sap-ui-resourceroots", "data-sap-ui-resource-roots"]
1618
|
1719
result = this.getCoreScript().getAttributeByName(resourceRootAttributeName).getValue()
1820
)
@@ -146,9 +148,7 @@ class SapUiCore extends MethodCallNode {
146148
* Used for static methods like `Fragment.byId(viewId, controlId)`.
147149
*/
148150
class FragmentModule extends DataFlow::SourceNode {
149-
FragmentModule() {
150-
this = DataFlow::moduleImport("sap/ui/core/Fragment")
151-
}
151+
FragmentModule() { this = DataFlow::moduleImport("sap/ui/core/Fragment") }
152152
}
153153

154154
/**
@@ -344,26 +344,22 @@ class ControlReference extends Reference {
344344
string controlId;
345345

346346
ControlReference() {
347+
// Standard byId patterns: this.byId("id"), this.getView().byId("id"), sap.ui.getCore().byId("id")
348+
this.getArgument(0).getALocalSource().getStringValue() = controlId and
347349
(
348-
// Standard byId patterns: this.byId("id"), this.getView().byId("id"), sap.ui.getCore().byId("id")
349-
this.getArgument(0).getALocalSource().getStringValue() = controlId and
350-
(
351-
exists(CustomController controller |
352-
this = controller.getAViewReference().getAMemberCall("byId") or
353-
this = controller.getAThisNode().getAMemberCall("byId")
354-
)
355-
or
356-
exists(SapUiCore sapUiCore | this = sapUiCore.getAMemberCall("byId"))
350+
exists(CustomController controller |
351+
this = controller.getAViewReference().getAMemberCall("byId") or
352+
this = controller.getAThisNode().getAMemberCall("byId")
357353
)
354+
or
355+
exists(SapUiCore sapUiCore | this = sapUiCore.getAMemberCall("byId"))
358356
)
359357
or
360358
// Fragment.byId(viewId, controlId) - static method with 2 arguments
361-
(
362-
this.getNumArgument() = 2 and
363-
this.getArgument(1).getALocalSource().getStringValue() = controlId and
364-
this.getMethodName() = "byId" and
365-
exists(FragmentModule fragment | this = fragment.getAMemberCall("byId"))
366-
)
359+
this.getNumArgument() = 2 and
360+
this.getArgument(1).getALocalSource().getStringValue() = controlId and
361+
this.getMethodName() = "byId" and
362+
exists(FragmentModule fragment | this = fragment.getAMemberCall("byId"))
367363
}
368364

369365
CustomControl getDefinition() {

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,36 @@
11
import javascript
22
import advanced_security.javascript.frameworks.ui5.UI5
3+
import advanced_security.javascript.frameworks.ui5.UI5View
4+
5+
/**
6+
* Step from a value assigned to a JSONModel property to the binding path that reads it.
7+
* This enables tracking data flowing INTO a model constructor argument and OUT through XML bindings.
8+
*
9+
* e.g. Given:
10+
* ```javascript
11+
* var userInput = getUntrustedData();
12+
* var oModel = new JSONModel({ payload: userInput });
13+
* this.getView().setModel(oModel);
14+
* ```
15+
* and
16+
* ```xml
17+
* <core:HTML content="{/payload}" />
18+
* ```
19+
*
20+
* Creates a flow step from `userInput` (the RHS of the property) to the binding path `{/payload}`.
21+
*/
22+
class JsonModelPropertyValueToBindingStep extends DataFlow::SharedFlowStep {
23+
override predicate step(DataFlow::Node start, DataFlow::Node end) {
24+
exists(UI5BindingPath bindingPath, DataFlow::PropWrite propWrite |
25+
// The binding path resolves to a property write in a JSONModel
26+
bindingPath.getNode() = propWrite and
27+
// The start is the RHS value being assigned to that property
28+
start = propWrite.getRhs() and
29+
// The end is the binding path itself (as a node representing where data flows to)
30+
end = propWrite
31+
)
32+
}
33+
}
334

435
/**
536
* Step from a part of internal model to a relevant control property.

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-urlparams-jsonmodel/UI5Xss.expected‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
nodes
2+
| webapp/controller/Display.controller.js:10:17:10:25 | oUrlQuery |
3+
| webapp/controller/Display.controller.js:10:29:10:71 | new URL ... search) |
4+
| webapp/controller/Display.controller.js:10:49:10:70 | window. ... .search |
5+
| webapp/controller/Display.controller.js:11:17:11:30 | sRemotePayload |
6+
| webapp/controller/Display.controller.js:11:34:11:42 | oUrlQuery |
7+
| webapp/controller/Display.controller.js:11:34:11:57 | oUrlQue ... yload") |
8+
| webapp/controller/Display.controller.js:14:51:14:85 | "<div>" ... </div>" |
9+
| webapp/controller/Display.controller.js:14:61:14:74 | sRemotePayload |
10+
| webapp/controller/Display.controller.js:19:17:19:45 | remoteP ... Payload |
11+
| webapp/controller/Display.controller.js:19:32:19:45 | sRemotePayload |
12+
| webapp/fragments/Content.fragment.xml:7:9:7:49 | content={/remotePayload} |
13+
| webapp/view/Display.view.xml:6:13:6:53 | content={/remotePayload} |
14+
edges
15+
| webapp/controller/Display.controller.js:10:17:10:25 | oUrlQuery | webapp/controller/Display.controller.js:11:34:11:42 | oUrlQuery |
16+
| webapp/controller/Display.controller.js:10:29:10:71 | new URL ... search) | webapp/controller/Display.controller.js:10:17:10:25 | oUrlQuery |
17+
| webapp/controller/Display.controller.js:10:49:10:70 | window. ... .search | webapp/controller/Display.controller.js:10:29:10:71 | new URL ... search) |
18+
| webapp/controller/Display.controller.js:11:17:11:30 | sRemotePayload | webapp/controller/Display.controller.js:14:61:14:74 | sRemotePayload |
19+
| webapp/controller/Display.controller.js:11:17:11:30 | sRemotePayload | webapp/controller/Display.controller.js:19:32:19:45 | sRemotePayload |
20+
| webapp/controller/Display.controller.js:11:34:11:42 | oUrlQuery | webapp/controller/Display.controller.js:11:34:11:57 | oUrlQue ... yload") |
21+
| webapp/controller/Display.controller.js:11:34:11:57 | oUrlQue ... yload") | webapp/controller/Display.controller.js:11:17:11:30 | sRemotePayload |
22+
| webapp/controller/Display.controller.js:14:61:14:74 | sRemotePayload | webapp/controller/Display.controller.js:14:51:14:85 | "<div>" ... </div>" |
23+
| webapp/controller/Display.controller.js:18:30:20:14 | new JSO ... }) | webapp/fragments/Content.fragment.xml:7:9:7:49 | content={/remotePayload} |
24+
| webapp/controller/Display.controller.js:18:30:20:14 | new JSO ... }) | webapp/view/Display.view.xml:6:13:6:53 | content={/remotePayload} |
25+
| webapp/controller/Display.controller.js:19:17:19:45 | remoteP ... Payload | webapp/fragments/Content.fragment.xml:7:9:7:49 | content={/remotePayload} |
26+
| webapp/controller/Display.controller.js:19:17:19:45 | remoteP ... Payload | webapp/view/Display.view.xml:6:13:6:53 | content={/remotePayload} |
27+
| webapp/controller/Display.controller.js:19:32:19:45 | sRemotePayload | webapp/controller/Display.controller.js:19:17:19:45 | remoteP ... Payload |
28+
| webapp/fragments/Content.fragment.xml:7:9:7:49 | content={/remotePayload} | webapp/controller/Display.controller.js:19:17:19:45 | remoteP ... Payload |
29+
| webapp/view/Display.view.xml:6:13:6:53 | content={/remotePayload} | webapp/controller/Display.controller.js:19:17:19:45 | remoteP ... Payload |
30+
#select
31+
| webapp/controller/Display.controller.js:14:51:14:85 | "<div>" ... </div>" | webapp/controller/Display.controller.js:10:49:10:70 | window. ... .search | webapp/controller/Display.controller.js:14:51:14:85 | "<div>" ... </div>" | XSS vulnerability due to $@. | webapp/controller/Display.controller.js:10:49:10:70 | window. ... .search | user-provided value |
32+
| webapp/controller/Display.controller.js:14:51:14:85 | "<div>" ... </div>" | webapp/controller/Display.controller.js:11:34:11:57 | oUrlQue ... yload") | webapp/controller/Display.controller.js:14:51:14:85 | "<div>" ... </div>" | XSS vulnerability due to $@. | webapp/controller/Display.controller.js:11:34:11:57 | oUrlQue ... yload") | user-provided value |
33+
| webapp/fragments/Content.fragment.xml:7:9:7:49 | content={/remotePayload} | webapp/controller/Display.controller.js:10:49:10:70 | window. ... .search | webapp/fragments/Content.fragment.xml:7:9:7:49 | content={/remotePayload} | XSS vulnerability due to $@. | webapp/controller/Display.controller.js:10:49:10:70 | window. ... .search | user-provided value |
34+
| webapp/fragments/Content.fragment.xml:7:9:7:49 | content={/remotePayload} | webapp/controller/Display.controller.js:11:34:11:57 | oUrlQue ... yload") | webapp/fragments/Content.fragment.xml:7:9:7:49 | content={/remotePayload} | XSS vulnerability due to $@. | webapp/controller/Display.controller.js:11:34:11:57 | oUrlQue ... yload") | user-provided value |
35+
| webapp/view/Display.view.xml:6:13:6:53 | content={/remotePayload} | webapp/controller/Display.controller.js:10:49:10:70 | window. ... .search | webapp/view/Display.view.xml:6:13:6:53 | content={/remotePayload} | XSS vulnerability due to $@. | webapp/controller/Display.controller.js:10:49:10:70 | window. ... .search | user-provided value |
36+
| webapp/view/Display.view.xml:6:13:6:53 | content={/remotePayload} | webapp/controller/Display.controller.js:11:34:11:57 | oUrlQue ... yload") | webapp/view/Display.view.xml:6:13:6:53 | content={/remotePayload} | XSS vulnerability due to $@. | webapp/controller/Display.controller.js:11:34:11:57 | oUrlQue ... yload") | user-provided value |

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-urlparams-jsonmodel/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-urlparams-jsonmodel/package.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"name": "ui5-xss-urlparams-jsonmodel",
3+
"version": "1.0.0"
4+
}

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-urlparams-jsonmodel/ui5.yaml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
specVersion: "3.0"
2+
metadata:
3+
name: ui5-xss-urlparams-jsonmodel
4+
type: application

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-urlparams-jsonmodel/webapp/controller/Display.controller.js‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
sap.ui.define([
2+
"sap/ui/core/mvc/Controller",
3+
"sap/ui/model/json/JSONModel",
4+
"sap/ui/core/HTML",
5+
"sap/ui/core/Fragment"
6+
], function (Controller, JSONModel, HTML, Fragment) {
7+
"use strict";
8+
return Controller.extend("ui5-xss-urlparams-jsonmodel.controller.Display", {
9+
onInit: function () {
10+
var oUrlQuery = new URLSearchParams(window.location.search);
11+
var sRemotePayload = oUrlQuery.get("payload");
12+
13+
// Test 1: Direct sink in onInit - tests if URLSearchParams.get() is detected as source
14+
var oDirectHtml = new HTML({ content: "<div>" + sRemotePayload + "</div>" });
15+
this.getView().addContent(oDirectHtml);
16+
17+
// Test 2: Flow through JSONModel to View XML binding
18+
var oViewModel = new JSONModel({
19+
remotePayload: sRemotePayload
20+
});
21+
this.getView().setModel(oViewModel);
22+
23+
// Test 3: Flow through JSONModel to Fragment XML binding (mirrors app6c pattern)
24+
Fragment.load({
25+
id: this.getView().getId(),
26+
name: "ui5-xss-urlparams-jsonmodel.fragments.Content",
27+
controller: this
28+
}).then(function (oFragment) {
29+
this.byId("page").addContent(oFragment);
30+
}.bind(this));
31+
}
32+
});
33+
});

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-urlparams-jsonmodel/webapp/fragments/Content.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 URL:" />
6+
<!-- XSS sink: HTML content bound to user-controlled model property -->
7+
<core:HTML content="{/remotePayload}" />
8+
</VBox>
9+
</core:FragmentDefinition>

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-urlparams-jsonmodel/webapp/index.html‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8">
5+
<title>UI5 XSS URLSearchParams JSONModel Test</title>
6+
<script src="https://sdk.openui5.org/resources/sap-ui-core.js"
7+
data-sap-ui-libs="sap.m"
8+
data-sap-ui-onInit="module:ui5-xss-urlparams-jsonmodel/index"
9+
data-sap-ui-resourceroots='{
10+
"ui5-xss-urlparams-jsonmodel": "./"
11+
}'>
12+
</script>
13+
</head>
14+
<body class="sapUiBody" id="content">
15+
</body>
16+
</html>

0 commit comments

Comments
 (0)