Skip to content

Commit 530df03

Browse files
data-douserdata-douser
committed
Improve UI5Xss detection for fragments_samples 1
1 parent b0c1aef commit 530df03

13 files changed

Lines changed: 145 additions & 8 deletions

File tree

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,10 @@ extensions:
7171
- ["UI5ClientStorage", "global", "Member[jQuery].Member[sap].Member[storage]"]
7272
- ["UI5ClientStorage", "sap/ui/core/util/File", ""]
7373
- ["UI5ClientStorage", "global", "Member[sap].Member[ui].Member[core].Member[util].Member[File]"]
74+
# Fragment API - byId returns a Control (models static Fragment.byId() calls)
75+
- ["Fragment", "Fragment", "Instance"]
76+
- ["Fragment", "sap/ui/core/Fragment", ""]
77+
- ["UI5InputControl", "Fragment", "Member[byId].ReturnValue"]
7478

7579
- addsTo:
7680
pack: codeql/javascript-all

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

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,16 @@ class SapUiCore extends MethodCallNode {
141141
SapUiCore() { this = globalVarRef("sap").getAPropertyRead("ui").getAMethodCall("getCore") }
142142
}
143143

144+
/**
145+
* A reference to the Fragment module (`sap/ui/core/Fragment`).
146+
* Used for static methods like `Fragment.byId(viewId, controlId)`.
147+
*/
148+
class FragmentModule extends DataFlow::SourceNode {
149+
FragmentModule() {
150+
this = DataFlow::moduleImport("sap/ui/core/Fragment")
151+
}
152+
}
153+
144154
/**
145155
* A user-defined module through `sap.ui.define` or `jQuery.sap.declare`.
146156
*/
@@ -334,14 +344,25 @@ class ControlReference extends Reference {
334344
string controlId;
335345

336346
ControlReference() {
337-
this.getArgument(0).getALocalSource().getStringValue() = controlId and
338347
(
339-
exists(CustomController controller |
340-
this = controller.getAViewReference().getAMemberCall("byId") or
341-
this = controller.getAThisNode().getAMemberCall("byId")
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"))
342357
)
343-
or
344-
exists(SapUiCore sapUiCore | this = sapUiCore.getAMemberCall("byId"))
358+
)
359+
or
360+
// 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"))
345366
)
346367
}
347368

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

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -890,11 +890,23 @@ class UI5Control extends TUI5Control {
890890

891891
/**
892892
* Gets a reference to this control. Currently supports only such references made through `byId`.
893+
* Handles both:
894+
* - `this.byId("controlId")` or `this.getView().byId("controlId")` - ID in argument 0
895+
* - `Fragment.byId(viewId, "controlId")` - ID in argument 1
893896
*/
894897
ControlReference getAReference() {
895898
result.getMethodName() = "byId" and
896-
result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() =
897-
this.getProperty("id").getValue()
899+
(
900+
// Standard byId: ID in first argument
901+
result.getNumArgument() = 1 and
902+
result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() =
903+
this.getProperty("id").getValue()
904+
or
905+
// Fragment.byId: ID in second argument
906+
result.getNumArgument() = 2 and
907+
result.getArgument(1).getALocalSource().asExpr().(StringLiteral).getValue() =
908+
this.getProperty("id").getValue()
909+
)
898910
}
899911

900912
/** Gets a property of this control having the name. */

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/UI5Xss.expected‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
nodes
2+
| webapp/controller/Main.controller.js:20:17:20:29 | sAttackerData |
3+
| webapp/controller/Main.controller.js:20:33:20:97 | Fragmen ... Value() |
4+
| webapp/controller/Main.controller.js:24:34:24:69 | "<span> ... /span>" |
5+
| webapp/controller/Main.controller.js:24:45:24:57 | sAttackerData |
6+
| webapp/view/Main.view.html:4:10:4:34 | data-content={/payload} |
7+
edges
8+
| webapp/controller/Main.controller.js:20:17:20:29 | sAttackerData | webapp/controller/Main.controller.js:24:45:24:57 | sAttackerData |
9+
| webapp/controller/Main.controller.js:20:33:20:97 | Fragmen ... Value() | webapp/controller/Main.controller.js:20:17:20:29 | sAttackerData |
10+
| webapp/controller/Main.controller.js:24:45:24:57 | sAttackerData | webapp/controller/Main.controller.js:24:34:24:69 | "<span> ... /span>" |
11+
#select
12+
| webapp/controller/Main.controller.js:24:34:24:69 | "<span> ... /span>" | webapp/controller/Main.controller.js:20:33:20:97 | Fragmen ... Value() | webapp/controller/Main.controller.js:24:34:24:69 | "<span> ... /span>" | XSS vulnerability due to $@. | webapp/controller/Main.controller.js:20:33:20:97 | Fragmen ... Value() | user-provided value |

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"name": "ui5-xss-fragment-static-byid",
3+
"version": "1.0.0",
4+
"main": "index.js"
5+
}

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/ui5.yaml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
specVersion: '3.0'
2+
metadata:
3+
name: ui5-xss-fragment-static-byid
4+
type: application
5+
framework:
6+
name: SAPUI5
7+
version: "1.115.0"

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/controller/Main.controller.js‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
sap.ui.define([
2+
"sap/ui/core/mvc/Controller",
3+
"sap/ui/core/Fragment"
4+
], function (Controller, Fragment) {
5+
"use strict";
6+
return Controller.extend("ui5-xss-fragment-static-byid.controller.Main", {
7+
onInit: function () {
8+
// Load fragment with view ID prefix
9+
Fragment.load({
10+
id: this.getView().getId(),
11+
name: "ui5-xss-fragment-static-byid.view.PayloadForm",
12+
controller: this
13+
}).then(function (oFragment) {
14+
this.byId("fragmentArea").addContent(oFragment);
15+
}.bind(this));
16+
},
17+
18+
onSubmitPayload: function () {
19+
// XSS vulnerability: Fragment.byId() static method to access fragment controls
20+
var sAttackerData = Fragment.byId(this.getView().getId(), "attackerInput").getValue();
21+
var oHtmlSink = Fragment.byId(this.getView().getId(), "vulnerableOutput");
22+
23+
// Sink: setContent with unsanitized user input
24+
oHtmlSink.setContent("<span>" + sAttackerData + "</span>");
25+
}
26+
});
27+
});

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/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 Fragment Static ById 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-fragment-static-byid/index"
9+
data-sap-ui-resourceroots='{
10+
"ui5-xss-fragment-static-byid": "./"
11+
}'>
12+
</script>
13+
</head>
14+
<body class="sapUiBody" id="content">
15+
</body>
16+
</html>

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/index.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
sap.ui.define([
2+
"sap/ui/core/mvc/HTMLView"
3+
], function (HTMLView) {
4+
"use strict";
5+
HTMLView.create({
6+
viewName: "ui5-xss-fragment-static-byid.view.Main"
7+
}).then(function (oView) {
8+
oView.placeAt("content");
9+
});
10+
});

0 commit comments

Comments
 (0)