Skip to content

Commit 614220a

Browse files
committed
Finish dynamicWriteToHtmlContent and rename to it
1 parent aad96fd commit 614220a

File tree

15 files changed

+79
-42
lines changed

15 files changed

+79
-42
lines changed

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,14 @@ class ControlReference extends Reference {
352352
)
353353
}
354354

355+
predicate isLibraryControlReference(string importPath) {
356+
exists(XmlView xml, UI5Control control |
357+
control = xml.getControl() and
358+
control.getQualifiedType() = importPath and
359+
controlId = control.getProperty("id").getValue()
360+
)
361+
}
362+
355363
string getId() { result = controlId }
356364

357365
MethodCallNode getARead(string propertyName) {

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

Lines changed: 37 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ module UI5Xss implements DataFlow::ConfigSig {
3636
predicate isSink(DataFlow::Node node) {
3737
node instanceof UI5ExtHtmlISink or
3838
node instanceof UI5ModelHtmlISink or
39-
node instanceof DangerouslySetElementValueOfInstantiatedHTMLControlPlacedAtDom
39+
node instanceof DynamicallySetElementValueOfInstantiatedHTMLControlPlacedAtDom
4040
}
4141

4242
predicate isAdditionalFlowStep(DataFlow::Node start, DataFlow::Node end) {
@@ -71,7 +71,8 @@ class UI5ModelHtmlISink extends DataFlow::Node {
7171
private class UI5ExtHtmlISink extends DataFlow::Node {
7272
UI5ExtHtmlISink() {
7373
this = ModelOutput::getASinkNode("ui5-html-injection").asSink() and
74-
not this.asExpr().getParent+() instanceof NewExpr
74+
/* Exclude property writes to HTML controls; they are covered in a separate class below. */
75+
not this instanceof DynamicallySetElementValueOfInstantiatedHTMLControlPlacedAtDom
7576
}
7677
}
7778

@@ -81,48 +82,48 @@ private class HTMLControlInstantiation extends ElementInstantiation {
8182

8283
private module TrackPlaceAtCallConfigFlow = TaintTracking::Global<TrackPlaceAtCallConfig>;
8384

85+
abstract class DynamicallySetElementValueOfHTML extends DataFlow::Node { }
86+
8487
/**
8588
* The DOM value of a UI5 control that is dynamically generated then placed at
8689
* a certain position in a DOM.
8790
*/
88-
/*
89-
* TODO 1: Needs testing of each branch
90-
* TODO 2: Model the `placeAt`:
91-
*/
92-
93-
class DangerouslySetElementValueOfInstantiatedHTMLControlPlacedAtDom extends DataFlow::Node {
94-
HTMLControlInstantiation htmlControlInstantiation;
91+
class DynamicallySetElementValueOfInstantiatedHTMLControlPlacedAtDom extends DynamicallySetElementValueOfHTML
92+
{
93+
DataFlow::Node root;
9594
ControlPlaceAtCall placeAtCall;
9695

97-
DangerouslySetElementValueOfInstantiatedHTMLControlPlacedAtDom() {
98-
(
99-
/* 1. The content is set via the first argument of the constructor. */
100-
exists(string typeAlias |
101-
typeModel(typeAlias, htmlControlInstantiation.getImportPath(), _) and
102-
/* Double check that the type derives a UI5-XSS sink. */
103-
sinkModel(typeAlias, _, "ui5-html-injection", _) and
104-
exists(PropWrite propWrite |
105-
this = propWrite.getRhs() and
106-
propWrite.getBase() = htmlControlInstantiation.getArgument(0) and
107-
propWrite.getPropertyName() = "content"
108-
)
109-
)
110-
or
111-
/* 2. The content is written to the reference of the control. */
112-
exists(ControlReference controlReference |
113-
htmlControlInstantiation.getId() = controlReference.getId()
114-
|
115-
/*
116-
* 2-1. The content is written directly to the `content` property of the control
117-
* reference.
118-
*/
119-
120-
this = controlReference.getAPropertyWrite("content")
96+
DynamicallySetElementValueOfInstantiatedHTMLControlPlacedAtDom() {
97+
exists(NewNode new | root = new |
98+
new = ModelOutput::getATypeNode("UI5HTMLControl").getAnInstantiation() and
99+
(
100+
this = new.getAnArgument().(ObjectLiteralNode).getAPropertyWrite("content").getRhs()
121101
or
122-
/* 2-2. The content is written using the `setContent` setter. */
123-
this = controlReference.getAMemberCall("content").getArgument(0)
102+
this = new.getAPropertyWrite("content").getRhs()
103+
or
104+
this = new.getAMemberCall("setContent").getAnArgument()
124105
)
125106
) and
126-
TrackPlaceAtCallConfigFlow::flow(htmlControlInstantiation, placeAtCall)
107+
/* Ensure that this is placed somewhere in the DOM. */
108+
TrackPlaceAtCallConfigFlow::flow(root, placeAtCall)
109+
}
110+
}
111+
112+
class DynamicallySetElementValueOfHTMLControlReference extends DynamicallySetElementValueOfHTML {
113+
DynamicallySetElementValueOfHTMLControlReference() {
114+
/* 2. The content is written to the reference of the control. */
115+
exists(ControlReference controlReference |
116+
controlReference.isLibraryControlReference("sap.m.HTML")
117+
|
118+
/*
119+
* 2-1. The content is written directly to the `content` property of the control
120+
* reference.
121+
*/
122+
123+
this = controlReference.getAPropertyWrite("content")
124+
or
125+
/* 2-2. The content is written using the `setContent` setter. */
126+
this = controlReference.getAMemberCall("setContent").getArgument(0)
127+
)
127128
}
128129
}

javascript/frameworks/ui5/test/models/dangerous_write_to_html_content/dangerousWriteToHtmlContent.expected

Whitespace-only changes.

javascript/frameworks/ui5/test/models/dangerous_write_to_html_content/dangerousWriteToHtmlContent.ql

Lines changed: 0 additions & 5 deletions
This file was deleted.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import javascript
2+
import advanced_security.javascript.frameworks.ui5.UI5XssQuery
3+
4+
from DynamicallySetElementValueOfHTML htmlContent
5+
select htmlContent, "Content write to an HTML element "
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
| webapp/controller/app.controller.js:18:9:18:27 | htmlControl.content | Content write to an HTML element |
2+
| webapp/controller/app.controller.js:21:32:21:56 | inputRe ... Value() | Content write to an HTML element |
3+
| webapp/controller/app.controller.js:30:20:30:60 | `<div>$ ... </div>` | Content write to an HTML element |
4+
| webapp/controller/app.controller.js:36:32:36:56 | inputRe ... Value() | Content write to an HTML element |
5+
| webapp/controller/app.controller.js:41:33:41:57 | inputRe ... Value() | Content write to an HTML element |
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import javascript
2+
import advanced_security.javascript.frameworks.ui5.UI5XssQuery
3+
4+
from DynamicallySetElementValueOfHTML htmlContent
5+
select htmlContent, "Content write to an HTML element "

0 commit comments

Comments
 (0)