Skip to content

Commit 51508a1

Browse files
jeongsoolee09jeongsoolee09
authored
Merge pull request #67 from advanced-security/jeongsoolee09/unknown-remote-model
Detect XSS involving server-side models and controller handler parameters
2 parents 802e323 + 43d4b77 commit 51508a1

File tree

50 files changed

+2636
-1159
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

50 files changed

+2636
-1159
lines changed

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

-1
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,6 @@ extensions:
6666
- ["UI5Control", "Member[getValue].ReturnValue", "remote"]
6767
- ["UI5CodeEditor", "Member[value]", "remote"]
6868
- ["UI5CodeEditor", "Member[getCurrentValue].ReturnValue", "remote"]
69-
- ["global", "Member[jQuery].Member[sap].Member[getUriParameters].ReturnValue.Member[get].ReturnValue", "remote"]
7069
- ["global", "Member[jQuery].Member[sap].Member[syncHead,syncGet,syncGetText,syncPost,syncPostText].ReturnValue", "remote"]
7170
- ["UI5URIParameters", "Member[get].ReturnValue", "remote"]
7271
- ["UI5URIParameters", "Member[getAll].ReturnValue", "remote"]

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

+37-27
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,8 @@ abstract private class LateJavaScriptPropertyBinding extends DataFlow::Node {
193193
*/
194194
private predicate earlyPropertyBinding(
195195
DataFlow::NewNode newNode, DataFlow::PropWrite bindingTarget, DataFlow::Node binding,
196-
DataFlow::Node bindingPath) {
196+
DataFlow::Node bindingPath
197+
) {
197198
// Property binding via an object literal binding with property `path`.
198199
// This assumes the value assigned to `path` is a binding, even if we cannot
199200
// statically determine it is a binding.
@@ -205,9 +206,10 @@ private predicate earlyPropertyBinding(
205206
if exists(binding.getALocalSource())
206207
then binding.getALocalSource() = bindingPath
207208
else binding = bindingPath // e.g., path: "/" + someVar
208-
) and not bindingPath.getStringValue() instanceof BindingString
209+
) and
210+
not bindingPath.getStringValue() instanceof BindingString
209211
or
210-
// Propery binding of an arbitrary property for which we can statically determined
212+
// Property binding of an arbitrary property for which we can statically determined
211213
// the value written to the property is a binding path.
212214
exists(DataFlow::SourceNode objectLiteral |
213215
newNode.getAnArgument().getALocalSource() = objectLiteral and
@@ -339,9 +341,7 @@ private newtype TBinding =
339341
* with a property `parts` assigned a value, or
340342
* an object literal that is assigned a string value that is a binding path.
341343
*/
342-
TEarlyJavaScriptPropertyBinding(
343-
DataFlow::PropWrite bindingTarget, DataFlow::ValueNode binding
344-
) {
344+
TEarlyJavaScriptPropertyBinding(DataFlow::PropWrite bindingTarget, DataFlow::ValueNode binding) {
345345
earlyPropertyBinding(_, bindingTarget, binding, _)
346346
} or
347347
// Property binding via a call to `bindProperty` or `bindValue`.
@@ -415,29 +415,34 @@ private newtype TBindingPath =
415415
)
416416
} or
417417
TDynamicBindingPath(Binding binding, DataFlow::Node dynamicBinding, DataFlow::Node bindingPath) {
418-
(exists(DataFlow::PropWrite bindingTarget |
419-
binding = TEarlyJavaScriptPropertyBinding(bindingTarget, dynamicBinding) and
420-
earlyPropertyBinding(_, bindingTarget, dynamicBinding, bindingPath)
421-
)
422-
or
423-
exists(LateJavaScriptPropertyBinding lateJavaScriptPropertyBinding |
424-
// Property binding via a call to `bindProperty` or `bindValue`.
425-
binding = TLateJavaScriptPropertyBinding(lateJavaScriptPropertyBinding, dynamicBinding) and
426-
latePropertyBinding(lateJavaScriptPropertyBinding, dynamicBinding, bindingPath)
427-
)
428-
or
429-
exists(BindElementMethodCallNode bindElementMethodCall |
430-
// Element binding via a call to `bindElement`.
431-
binding = TLateJavaScriptContextBinding(bindElementMethodCall, dynamicBinding) and
432-
lateContextBinding(bindElementMethodCall, dynamicBinding, bindingPath)
433-
)) and
418+
(
419+
exists(DataFlow::PropWrite bindingTarget |
420+
binding = TEarlyJavaScriptPropertyBinding(bindingTarget, dynamicBinding) and
421+
earlyPropertyBinding(_, bindingTarget, dynamicBinding, bindingPath)
422+
)
423+
or
424+
exists(LateJavaScriptPropertyBinding lateJavaScriptPropertyBinding |
425+
// Property binding via a call to `bindProperty` or `bindValue`.
426+
binding = TLateJavaScriptPropertyBinding(lateJavaScriptPropertyBinding, dynamicBinding) and
427+
latePropertyBinding(lateJavaScriptPropertyBinding, dynamicBinding, bindingPath)
428+
)
429+
or
430+
exists(BindElementMethodCallNode bindElementMethodCall |
431+
// Element binding via a call to `bindElement`.
432+
binding = TLateJavaScriptContextBinding(bindElementMethodCall, dynamicBinding) and
433+
lateContextBinding(bindElementMethodCall, dynamicBinding, bindingPath)
434+
)
435+
) and
434436
not dynamicBinding.mayHaveStringValue(_)
435437
}
436438

437439
/**
438440
* A class representing a binding path.
439441
*/
440442
class BindingPath extends TBindingPath {
443+
/**
444+
* For debugging purposes (pretty-printing in result table)
445+
*/
441446
string toString() {
442447
exists(BindingStringParser::BindingPath path |
443448
this = TStaticBindingPath(_, _, path) and
@@ -460,6 +465,11 @@ class BindingPath extends TBindingPath {
460465
this = TStaticBindingPath(_, _, path) and
461466
result = path.toString()
462467
)
468+
or
469+
exists(DataFlow::Node pathValue |
470+
this = TDynamicBindingPath(_, _, pathValue) and
471+
result = pathValue.asExpr().(StringLiteral).getValue().regexpCapture("\\{(.*)\\}", 1)
472+
)
463473
}
464474

465475
Location getLocation() {
@@ -616,8 +626,8 @@ class BindingTarget extends TBindingTarget {
616626
Binding getBinding() {
617627
this = TXmlPropertyBindingTarget(_, result) or
618628
this = TXmlContextBindingTarget(_, result) or
619-
this = TLateJavaScriptBindingTarget(_, result) or
620629
this = TEarlyJavaScriptPropertyBindingTarget(_, result) or
630+
this = TLateJavaScriptBindingTarget(_, result) or
621631
this = TJsonPropertyBindingTarget(_, _, result)
622632
}
623633
}
@@ -650,7 +660,9 @@ class Binding extends TBinding {
650660
or
651661
exists(DataFlow::PropWrite bindingTarget, DataFlow::Node binding |
652662
this = TEarlyJavaScriptPropertyBinding(bindingTarget, binding) and
653-
result = "Early JavaScript property binding: " + bindingTarget.getPropertyNameExpr() + " to " + binding
663+
result =
664+
"Early JavaScript property binding: " + bindingTarget.getPropertyNameExpr() + " to " +
665+
binding
654666
)
655667
or
656668
exists(LateJavaScriptPropertyBinding lateJavaScriptPropertyBinding, DataFlow::Node binding |
@@ -710,9 +722,7 @@ class Binding extends TBinding {
710722
)
711723
}
712724

713-
BindingPath getBindingPath() {
714-
result.getBinding() = this
715-
}
725+
BindingPath getBindingPath() { result.getBinding() = this }
716726

717727
BindingTarget getBindingTarget() { result.getBinding() = this }
718728
}

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

+135
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
import javascript
2+
import advanced_security.javascript.frameworks.ui5.UI5
3+
import advanced_security.javascript.frameworks.ui5.UI5View
4+
private import semmle.javascript.frameworks.data.internal.ApiGraphModelsExtensions as ApiGraphModelsExtensions
5+
6+
private class DataFromRemoteControlReference extends RemoteFlowSource, MethodCallNode {
7+
DataFromRemoteControlReference() {
8+
exists(UI5Control sourceControl, string typeAlias, ControlReference controlReference |
9+
ApiGraphModelsExtensions::typeModel(typeAlias, sourceControl.getImportPath(), _) and
10+
ApiGraphModelsExtensions::sourceModel(typeAlias, _, "remote") and
11+
sourceControl.getAReference() = controlReference and
12+
controlReference.flowsTo(this.getReceiver()) and
13+
this.getMethodName() = "getValue"
14+
)
15+
}
16+
17+
override string getSourceType() { result = "Data from a remote control" }
18+
}
19+
20+
class LocalModelContentBoundBidirectionallyToSourceControl extends RemoteFlowSource {
21+
UI5BindingPath bindingPath;
22+
UI5Control controlDeclaration;
23+
24+
LocalModelContentBoundBidirectionallyToSourceControl() {
25+
exists(UI5InternalModel internalModel |
26+
this = bindingPath.getNode() and
27+
(
28+
this instanceof PropWrite and
29+
internalModel.getArgument(0).getALocalSource().asExpr() =
30+
this.(PropWrite).getPropertyNameExpr().getParent+()
31+
or
32+
this.asExpr() instanceof StringLiteral and
33+
internalModel.asExpr() = this.asExpr().getParent()
34+
) and
35+
any(UI5View view).getASource() = bindingPath and
36+
internalModel.(JsonModel).isTwoWayBinding() and
37+
controlDeclaration = bindingPath.getControlDeclaration()
38+
)
39+
}
40+
41+
override string getSourceType() {
42+
result = "Local model bidirectionally bound to a input control"
43+
}
44+
45+
UI5BindingPath getBindingPath() { result = bindingPath }
46+
47+
UI5Control getControlDeclaration() { result = controlDeclaration }
48+
}
49+
50+
abstract class UI5ExternalModel extends UI5Model, RemoteFlowSource {
51+
abstract string getName();
52+
}
53+
54+
/** Model which gains content from an SAP OData service. */
55+
class ODataServiceModel extends UI5ExternalModel {
56+
string modelName;
57+
58+
override string getSourceType() { result = "ODataServiceModel" }
59+
60+
ODataServiceModel() {
61+
/*
62+
* e.g. this.getView().setModel(this.getOwnerComponent().getModel("booking_nobatch"))
63+
*/
64+
65+
exists(MethodCallNode setModelCall, CustomController controller |
66+
/*
67+
* 1. This flows from a DF node corresponding to the parent component's model to the `this.setModel` call
68+
* i.e. Aims to capture something like `this.getOwnerComponent().getModel("someModelName")` as in
69+
* `this.getView().setModel(this.getOwnerComponent().getModel("someModelName"))`
70+
*/
71+
72+
modelName = this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() and
73+
this.getCalleeName() = "getModel" and
74+
controller.getOwnerComponentRef().flowsTo(this.(MethodCallNode).getReceiver()) and
75+
this.flowsTo(setModelCall.getArgument(0)) and
76+
setModelCall.getMethodName() = "setModel" and
77+
setModelCall.getReceiver() = controller.getAViewReference() and
78+
/* 2. The component's manifest.json declares the DataSource as being of OData type */
79+
controller.getOwnerComponent().getExternalModelDef(modelName).getDataSource() instanceof
80+
ODataDataSourceManifest
81+
)
82+
or
83+
/*
84+
* A constructor call to sap.ui.model.odata.v2.ODataModel.
85+
*/
86+
87+
this instanceof NewNode and
88+
(
89+
exists(RequiredObject oDataModel |
90+
oDataModel.flowsTo(this.getCalleeNode()) and
91+
oDataModel.getDependencyType() = "sap/ui/model/odata/v2/ODataModel"
92+
)
93+
or
94+
this.getCalleeName() = "ODataModel"
95+
) and
96+
modelName = "<no name>"
97+
}
98+
99+
override string getName() { result = modelName }
100+
}
101+
102+
private class RouteParameterAccess extends RemoteFlowSource instanceof PropRead {
103+
override string getSourceType() { result = "RouteParameterAccess" }
104+
105+
RouteParameterAccess() {
106+
exists(
107+
ControllerHandler handler, RouteManifest routeManifest, ParameterNode handlerParameter,
108+
MethodCallNode getParameterCall
109+
|
110+
handler.isAttachedToRoute(routeManifest.getName()) and
111+
this.asExpr().getEnclosingFunction() = handler.getFunction() and
112+
handlerParameter = handler.getParameter(0) and
113+
getParameterCall.getMethodName() = "getParameter" and
114+
getParameterCall.getReceiver().getALocalSource() = handlerParameter and
115+
(
116+
routeManifest.matchesPathString(this.getPropertyName()) and
117+
this.getBase().getALocalSource() = getParameterCall
118+
or
119+
/* TODO: Why does `routeManifest.matchesPathString` not work for propertyName?? */
120+
this.getBase().(PropRead).getBase().getALocalSource() = getParameterCall
121+
)
122+
)
123+
}
124+
}
125+
126+
/**
127+
* Method calls that fetch a piece of data either from a library control capable of accepting user input, or from a URI parameter.
128+
*/
129+
private class UI5ExtRemoteSource extends RemoteFlowSource {
130+
UI5ExtRemoteSource() { this = ModelOutput::getASourceNode("remote").asSource() }
131+
132+
override string getSourceType() {
133+
result = "Remote flow" // Don't discriminate between UI5-specific remote flows and vanilla ones
134+
}
135+
}

0 commit comments

Comments
 (0)