Skip to content

Commit e762b43

Browse files
knewbury01knewbury01
authored
Merge pull request #281 from advanced-security/knewbury01/dd/ui5-fragments
Improve UI5Xss detection for fragments_samples 1
2 parents 52cab4a + caf2596 commit e762b43

24 files changed

Lines changed: 296 additions & 3 deletions

File tree

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,10 @@ extensions:
7676
- ["UI5ClientStorage", "global", "Member[jQuery].Member[sap].Member[storage]"]
7777
- ["UI5ClientStorage", "sap/ui/core/util/File", ""]
7878
- ["UI5ClientStorage", "global", "Member[sap].Member[ui].Member[core].Member[util].Member[File]"]
79+
# Fragment API - byId returns a Control (models static Fragment.byId() calls)
80+
- ["Fragment", "Fragment", "Instance"]
81+
- ["Fragment", "sap/ui/core/Fragment", ""]
82+
- ["UI5InputControl", "Fragment", "Member[byId].ReturnValue"]
7983
# Publishing and Subscribing to Events
8084
- ["UI5EventBusInstance", "sap/ui/core/EventBus", "Member[getInstance].ReturnValue"]
8185
- ["UI5EventBusPublish", "UI5EventBusInstance", "Member[publish]"]

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

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,10 @@ private module WebAppResourceRootJsonReader implements JsonParser::MakeJsonReade
1212
class JsonReader extends WebApp {
1313
string getJson() {
1414
// We match on the lowercase to cover all the possible variants of writing the attribute name.
15+
// Support both "data-sap-ui-resourceroots" and "data-sap-ui-resource-roots" (with hyphen)
1516
exists(string resourceRootAttributeName |
16-
resourceRootAttributeName.toLowerCase() = "data-sap-ui-resourceroots"
17+
resourceRootAttributeName.toLowerCase() =
18+
["data-sap-ui-resourceroots", "data-sap-ui-resource-roots"]
1719
|
1820
result = this.getCoreScript().getAttributeByName(resourceRootAttributeName).getValue()
1921
)
@@ -151,6 +153,17 @@ class SapUiCore extends MethodCallNode {
151153
SapUiCore() { this = globalVarRef("sap").getAPropertyRead("ui").getAMethodCall("getCore") }
152154
}
153155

156+
/**
157+
* A reference to the Fragment module (`sap/ui/core/Fragment`).
158+
* Used for static methods like `Fragment.byId(viewId, controlId)`.
159+
*
160+
* Use of `DataFlow::moduleImport` may not cover byId references
161+
* coming from sources with es6 style imports of Fragments.
162+
*/
163+
class FragmentModule extends DataFlow::SourceNode {
164+
FragmentModule() { this = DataFlow::moduleImport("sap/ui/core/Fragment") }
165+
}
166+
154167
/**
155168
* A user-defined module through `sap.ui.define` or `jQuery.sap.declare`.
156169
*/
@@ -343,6 +356,7 @@ class ControlReference extends Reference {
343356
string controlId;
344357

345358
ControlReference() {
359+
// Standard byId patterns: this.byId("id"), this.getView().byId("id"), sap.ui.getCore().byId("id")
346360
this.getArgument(0).getALocalSource().getStringValue() = controlId and
347361
(
348362
exists(CustomController controller |
@@ -352,6 +366,11 @@ class ControlReference extends Reference {
352366
or
353367
exists(SapUiCore sapUiCore | this = sapUiCore.getAMemberCall("byId"))
354368
)
369+
or
370+
// Fragment.byId(viewId, controlId) - static method with 2 arguments
371+
this.getNumArgument() = 2 and
372+
this.getArgument(1).getALocalSource().getStringValue() = controlId and
373+
exists(FragmentModule fragment | this = fragment.getAMemberCall("byId"))
355374
}
356375

357376
CustomControl getDefinition() {

‎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
@@ -879,11 +879,23 @@ class UI5Control extends TUI5Control {
879879

880880
/**
881881
* Gets a reference to this control. Currently supports only such references made through `byId`.
882+
* Handles both:
883+
* - `this.byId("controlId")` or `this.getView().byId("controlId")` - ID in argument 0
884+
* - `Fragment.byId(viewId, "controlId")` - ID in argument 1
882885
*/
883886
ControlReference getAReference() {
884887
result.getMethodName() = "byId" and
885-
result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() =
886-
this.getProperty("id").getValue()
888+
(
889+
// Standard byId: ID in first argument
890+
result.getNumArgument() = 1 and
891+
result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() =
892+
this.getProperty("id").getValue()
893+
or
894+
// Fragment.byId: ID in second argument
895+
result.getNumArgument() = 2 and
896+
result.getArgument(1).getALocalSource().asExpr().(StringLiteral).getValue() =
897+
this.getProperty("id").getValue()
898+
)
887899
}
888900

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

‎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-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>

0 commit comments

Comments
 (0)