Skip to content

Commit e54e3fb

Browse files
mbaludambaluda
authored
Adds a local source for separate Control implementations. (#329)
* Adds a local source for separate Control implementations. * Remove refereence to Controller * Adds test for separate control implementation * fix expected file * Additional flow steps do not cross application boundaries Adds a test comprising of 2 separate UI5 apps * Fix expected file * Fix sarif expected file
1 parent a219d0c commit e54e3fb

File tree

32 files changed

+347
-176
lines changed

32 files changed

+347
-176
lines changed

‎.github/workflows/javascript.sarif.expected‎

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,15 @@ module UI5LogEntryToHttp implements DataFlow::StateConfigSig {
2626
predicate isAdditionalFlowStep(
2727
DataFlow::Node start, FlowState preState, DataFlow::Node end, FlowState postState
2828
) {
29-
UI5LogInjection::isAdditionalFlowStep(start, end) and
30-
preState = postState
31-
or
32-
logArgumentToListener(start, end) and
33-
preState = "not-logged-not-accessed" and
34-
postState = "logged-and-accessed"
29+
inSameWebApp(start.getFile(), end.getFile()) and
30+
(
31+
UI5LogInjection::isAdditionalFlowStep(start, end) and
32+
preState = postState
33+
or
34+
logArgumentToListener(start, end) and
35+
preState = "not-logged-not-accessed" and
36+
postState = "logged-and-accessed"
37+
)
3538
}
3639

3740
predicate isSink(DataFlow::Node node, FlowState state) {

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

Lines changed: 29 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -49,33 +49,36 @@ module UI5Xss implements DataFlow::ConfigSig {
4949
}
5050

5151
predicate isAdditionalFlowStep(DataFlow::Node start, DataFlow::Node end) {
52-
/* Already an additional flow step defined in `DomBasedXssQuery::Configuration` */
53-
DomBasedXss::DomBasedXssConfig::isAdditionalFlowStep(start, _, end, _)
54-
or
55-
/* TODO: Legacy code */
56-
/* Handler argument node to handler parameter */
57-
exists(UI5Handler h |
58-
start = h.getBindingPath().getNode() and
59-
/*
60-
* Ideally we would like to show an intermediate node where
61-
* the handler is bound to a control, but there is no sourceNode there
62-
* `end = h.getBindingPath() or start = h.getBindingPath()`
63-
*/
52+
inSameWebApp(start.getFile(), end.getFile()) and
53+
(
54+
/* Already an additional flow step defined in `DomBasedXssQuery::Configuration` */
55+
DomBasedXss::DomBasedXssConfig::isAdditionalFlowStep(start, _, end, _)
56+
or
57+
/* TODO: Legacy code */
58+
/* Handler argument node to handler parameter */
59+
exists(UI5Handler h |
60+
start = h.getBindingPath().getNode() and
61+
/*
62+
* Ideally we would like to show an intermediate node where
63+
* the handler is bound to a control, but there is no sourceNode there
64+
* `end = h.getBindingPath() or start = h.getBindingPath()`
65+
*/
6466

65-
end = h.getParameter(0)
66-
)
67-
or
68-
/* Flow from `setContent` to `getContent` of a control */
69-
exists(
70-
UI5Control control, DataFlow::MethodCallNode getContent, DataFlow::MethodCallNode setContent
71-
|
72-
control.asJsControl() = setContent.getReceiver().getALocalSource() and
73-
control.asJsControl() = getContent.getReceiver().getALocalSource() and
74-
setContent.getMethodName() = "setContent" and
75-
getContent.getMethodName() = "getContent" and
76-
start = setContent.getArgument(0) and
77-
end = getContent and
78-
not control.isHTMLSanitized()
67+
end = h.getParameter(0)
68+
)
69+
or
70+
/* Flow from `setContent` to `getContent` of a control */
71+
exists(
72+
UI5Control control, DataFlow::MethodCallNode getContent, DataFlow::MethodCallNode setContent
73+
|
74+
control.asJsControl() = setContent.getReceiver().getALocalSource() and
75+
control.asJsControl() = getContent.getReceiver().getALocalSource() and
76+
setContent.getMethodName() = "setContent" and
77+
getContent.getMethodName() = "getContent" and
78+
start = setContent.getArgument(0) and
79+
end = getContent and
80+
not control.isHTMLSanitized()
81+
)
7982
)
8083
}
8184
}

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

Lines changed: 129 additions & 108 deletions
Large diffs are not rendered by default.

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

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,15 @@ class LocalModelContentBoundBidirectionallyToHtmlISinkControl extends DomBasedXs
6161
UI5Control getControlDeclaration() { result = controlDeclaration }
6262
}
6363

64+
/**
65+
* A local source for cases where the Control implementation is separate from the complete UI5 app.
66+
*/
67+
class LocalModelStringPropertySource extends DomBasedXss::Source {
68+
LocalModelStringPropertySource() {
69+
this = any(PropertyMetadata propMeta | propMeta.isUnrestrictedStringType())
70+
}
71+
}
72+
6473
module UI5PathGraph<PathNodeSig ConfigPathNode, PathGraphSig<ConfigPathNode> ConfigPathGraph> {
6574
private newtype TNode =
6675
TUI5BindingPathNode(UI5BindingPath path) or
@@ -254,24 +263,27 @@ module TrackPlaceAtCallConfig implements DataFlow::ConfigSig {
254263
* - [ ] Heuristic by prefix: not an attractive option since heuristics can fail
255264
*/
256265
predicate isAdditionalFlowStep(DataFlow::Node start, DataFlow::Node end) {
257-
exists(DataFlow::PropWrite propWrite |
258-
start = propWrite.getRhs() and
259-
end = propWrite.getBase()
260-
)
261-
or
262-
exists(DataFlow::MethodCallNode maybeAddingChildAPICall |
263-
start = maybeAddingChildAPICall.getAnArgument() and
264-
end = maybeAddingChildAPICall.getReceiver()
265-
)
266-
or
267-
exists(DataFlow::MethodCallNode call |
268-
start = call.getReceiver() and
269-
end = call
270-
)
271-
or
272-
exists(DataFlow::NewNode new |
273-
start = new.getAnArgument() and
274-
end = new
266+
inSameWebApp(start.getFile(), end.getFile()) and
267+
(
268+
exists(DataFlow::PropWrite propWrite |
269+
start = propWrite.getRhs() and
270+
end = propWrite.getBase()
271+
)
272+
or
273+
exists(DataFlow::MethodCallNode maybeAddingChildAPICall |
274+
start = maybeAddingChildAPICall.getAnArgument() and
275+
end = maybeAddingChildAPICall.getReceiver()
276+
)
277+
or
278+
exists(DataFlow::MethodCallNode call |
279+
start = call.getReceiver() and
280+
end = call
281+
)
282+
or
283+
exists(DataFlow::NewNode new |
284+
start = new.getAnArgument() and
285+
end = new
286+
)
275287
)
276288
}
277289
}

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-book-example/UI5Xss.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,4 @@ edges
1313
| webapp/controls/Book.js:133:8:133:26 | oControl.getTitle() | webapp/controls/Book.js:132:7:134:15 | "<div>T ... </div>" |
1414
#select
1515
| webapp/controls/Book.js:132:7:134:15 | "<div>T ... </div>" | webapp/controller/App.Controller.js:23:25:23:47 | oSearch ... Value() | webapp/controls/Book.js:132:7:134:15 | "<div>T ... </div>" | XSS vulnerability due to $@. | webapp/controller/App.Controller.js:23:25:23:47 | oSearch ... Value() | user-provided value |
16+
| webapp/controls/Book.js:132:7:134:15 | "<div>T ... </div>" | webapp/controls/Book.js:17:13:17:30 | { type: "string" } | webapp/controls/Book.js:132:7:134:15 | "<div>T ... </div>" | XSS vulnerability due to $@. | webapp/controls/Book.js:17:13:17:30 | { type: "string" } | user-provided value |

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-custom-control-api1/UI5Xss.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,5 @@ edges
1414
| webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:11:26:11:45 | new JSONModel(oData) |
1515
| webapp/view/app.view.xml:8:5:8:38 | text={/input} | webapp/controller/app.controller.js:9:17:9:27 | input: null |
1616
#select
17+
| webapp/control/xss.js:14:23:14:40 | oControl.getText() | webapp/control/xss.js:7:23:7:40 | { type: "string" } | webapp/control/xss.js:14:23:14:40 | oControl.getText() | XSS vulnerability due to $@. | webapp/control/xss.js:7:23:7:40 | { type: "string" } | user-provided value |
1718
| webapp/control/xss.js:14:23:14:40 | oControl.getText() | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/control/xss.js:14:23:14:40 | oControl.getText() | XSS vulnerability due to $@. | webapp/view/app.view.xml:5:5:7:28 | value={/input} | user-provided value |

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-custom-control-api2/UI5Xss.expected‎

Lines changed: 0 additions & 17 deletions
This file was deleted.

‎javascript/frameworks/ui5/test/queries/UI5Xss/xss-custom-control-apis/UI5Xss.expected‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
nodes
2+
| xss-custom-control-api3/webapp/control/xss.js:7:23:7:40 | { type: "string" } |
3+
| xss-custom-control-api3/webapp/control/xss.js:8:24:8:41 | { type: "string" } |
4+
| xss-custom-control-api3/webapp/control/xss.js:15:32:15:49 | oControl.getText() |
5+
| xss-custom-control-api3/webapp/control/xss.js:16:32:16:50 | oControl.getText2() |
6+
| xss-custom-control-api3/webapp/controller/app.controller.js:9:17:9:27 | input: null |
7+
| xss-custom-control-api3/webapp/view/app.view.xml:5:5:7:28 | value={/input} |
8+
| xss-custom-control-api3/webapp/view/app.view.xml:8:5:8:38 | text={/input} |
9+
| xss-custom-control.api2/webapp/control/xss.js:7:23:7:40 | { type: "string" } |
10+
| xss-custom-control.api2/webapp/control/xss.js:14:32:14:49 | oControl.getText() |
11+
| xss-custom-control.api2/webapp/controller/app.controller.js:9:17:9:27 | input: null |
12+
| xss-custom-control.api2/webapp/view/app.view.xml:5:5:7:28 | value={/input} |
13+
| xss-custom-control.api2/webapp/view/app.view.xml:8:5:8:38 | text={/input} |
14+
edges
15+
| xss-custom-control-api3/webapp/control/xss.js:7:23:7:40 | { type: "string" } | xss-custom-control-api3/webapp/control/xss.js:15:32:15:49 | oControl.getText() |
16+
| xss-custom-control-api3/webapp/control/xss.js:7:23:7:40 | { type: "string" } | xss-custom-control-api3/webapp/controller/app.controller.js:9:17:9:27 | input: null |
17+
| xss-custom-control-api3/webapp/control/xss.js:8:24:8:41 | { type: "string" } | xss-custom-control-api3/webapp/control/xss.js:16:32:16:50 | oControl.getText2() |
18+
| xss-custom-control-api3/webapp/controller/app.controller.js:9:17:9:27 | input: null | xss-custom-control-api3/webapp/control/xss.js:7:23:7:40 | { type: "string" } |
19+
| xss-custom-control-api3/webapp/controller/app.controller.js:9:17:9:27 | input: null | xss-custom-control-api3/webapp/view/app.view.xml:5:5:7:28 | value={/input} |
20+
| xss-custom-control-api3/webapp/controller/app.controller.js:9:17:9:27 | input: null | xss-custom-control-api3/webapp/view/app.view.xml:8:5:8:38 | text={/input} |
21+
| xss-custom-control-api3/webapp/view/app.view.xml:5:5:7:28 | value={/input} | xss-custom-control-api3/webapp/controller/app.controller.js:9:17:9:27 | input: null |
22+
| xss-custom-control-api3/webapp/view/app.view.xml:5:5:7:28 | value={/input} | xss-custom-control-api3/webapp/controller/app.controller.js:11:26:11:45 | new JSONModel(oData) |
23+
| xss-custom-control-api3/webapp/view/app.view.xml:8:5:8:38 | text={/input} | xss-custom-control-api3/webapp/controller/app.controller.js:9:17:9:27 | input: null |
24+
| xss-custom-control.api2/webapp/control/xss.js:7:23:7:40 | { type: "string" } | xss-custom-control.api2/webapp/control/xss.js:14:32:14:49 | oControl.getText() |
25+
| xss-custom-control.api2/webapp/control/xss.js:7:23:7:40 | { type: "string" } | xss-custom-control.api2/webapp/controller/app.controller.js:9:17:9:27 | input: null |
26+
| xss-custom-control.api2/webapp/controller/app.controller.js:9:17:9:27 | input: null | xss-custom-control.api2/webapp/control/xss.js:7:23:7:40 | { type: "string" } |
27+
| xss-custom-control.api2/webapp/controller/app.controller.js:9:17:9:27 | input: null | xss-custom-control.api2/webapp/view/app.view.xml:5:5:7:28 | value={/input} |
28+
| xss-custom-control.api2/webapp/controller/app.controller.js:9:17:9:27 | input: null | xss-custom-control.api2/webapp/view/app.view.xml:8:5:8:38 | text={/input} |
29+
| xss-custom-control.api2/webapp/view/app.view.xml:5:5:7:28 | value={/input} | xss-custom-control.api2/webapp/controller/app.controller.js:9:17:9:27 | input: null |
30+
| xss-custom-control.api2/webapp/view/app.view.xml:5:5:7:28 | value={/input} | xss-custom-control.api2/webapp/controller/app.controller.js:11:26:11:45 | new JSONModel(oData) |
31+
| xss-custom-control.api2/webapp/view/app.view.xml:8:5:8:38 | text={/input} | xss-custom-control.api2/webapp/controller/app.controller.js:9:17:9:27 | input: null |
32+
#select
33+
| xss-custom-control-api3/webapp/control/xss.js:15:32:15:49 | oControl.getText() | xss-custom-control-api3/webapp/control/xss.js:7:23:7:40 | { type: "string" } | xss-custom-control-api3/webapp/control/xss.js:15:32:15:49 | oControl.getText() | XSS vulnerability due to $@. | xss-custom-control-api3/webapp/control/xss.js:7:23:7:40 | { type: "string" } | user-provided value |
34+
| xss-custom-control-api3/webapp/control/xss.js:15:32:15:49 | oControl.getText() | xss-custom-control-api3/webapp/view/app.view.xml:5:5:7:28 | value={/input} | xss-custom-control-api3/webapp/control/xss.js:15:32:15:49 | oControl.getText() | XSS vulnerability due to $@. | xss-custom-control-api3/webapp/view/app.view.xml:5:5:7:28 | value={/input} | user-provided value |
35+
| xss-custom-control-api3/webapp/control/xss.js:16:32:16:50 | oControl.getText2() | xss-custom-control-api3/webapp/control/xss.js:8:24:8:41 | { type: "string" } | xss-custom-control-api3/webapp/control/xss.js:16:32:16:50 | oControl.getText2() | XSS vulnerability due to $@. | xss-custom-control-api3/webapp/control/xss.js:8:24:8:41 | { type: "string" } | user-provided value |
36+
| xss-custom-control.api2/webapp/control/xss.js:14:32:14:49 | oControl.getText() | xss-custom-control.api2/webapp/control/xss.js:7:23:7:40 | { type: "string" } | xss-custom-control.api2/webapp/control/xss.js:14:32:14:49 | oControl.getText() | XSS vulnerability due to $@. | xss-custom-control.api2/webapp/control/xss.js:7:23:7:40 | { type: "string" } | user-provided value |
37+
| xss-custom-control.api2/webapp/control/xss.js:14:32:14:49 | oControl.getText() | xss-custom-control.api2/webapp/view/app.view.xml:5:5:7:28 | value={/input} | xss-custom-control.api2/webapp/control/xss.js:14:32:14:49 | oControl.getText() | XSS vulnerability due to $@. | xss-custom-control.api2/webapp/view/app.view.xml:5:5:7:28 | value={/input} | user-provided value |

javascript/frameworks/ui5/test/queries/UI5Xss/xss-custom-control-api2/UI5Xss.qlref renamed to javascript/frameworks/ui5/test/queries/UI5Xss/xss-custom-control-apis/UI5Xss.qlref

File renamed without changes.

0 commit comments

Comments
 (0)