Skip to content

Commit fb69b74

Browse files
jeongsoolee09jeongsoolee09
authored
Merge pull request #216 from advanced-security/jeongsoolee09/port-UI5PathGraph
Port `UI5PathGraph` to use the newer data flow API
2 parents 78e9d85 + 909ce39 commit fb69b74

File tree

30 files changed

+206
-218
lines changed

30 files changed

+206
-218
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ class SapDefineModule extends AmdModuleDefinition::Range, MethodCallExpr, UserMo
168168
)
169169
}
170170

171-
string getDependency(int i) { result = this.(AmdModuleDefinition).getDependency(i).getValue() }
171+
string getDependency(int i) { result = this.(AmdModuleDefinition).getDependencyExpr(i).getStringValue() }
172172

173173
override string getADependency() { result = this.getDependency(_) }
174174

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,13 @@ import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
88
private class StoragePutCall extends CallNode {
99
StoragePutCall() {
1010
/* 1. This is a call to `sap.ui.util.Storage.put` */
11-
// 1-1. Required from `sap/ui/util/Storage`
11+
/* 1-1. Required from `sap/ui/util/Storage` */
1212
exists(RequiredObject storageClass |
1313
this.getReceiver().getALocalSource() = storageClass.asSourceNode() and
1414
this.getCalleeName() = "put"
1515
)
1616
or
17-
// 1-2. Direct call to `sap.ui.util.Storage.put`
17+
/* 1-2. Direct call to `sap.ui.util.Storage.put` */
1818
this =
1919
globalVarRef("sap")
2020
.getAPropertyRead("ui")
@@ -109,12 +109,10 @@ private class FileSaveCall extends CallNode {
109109
}
110110
}
111111

112-
class UI5FormulaInjectionConfiguration extends TaintTracking::Configuration {
113-
UI5FormulaInjectionConfiguration() { this = "UI5 Formula Injection" }
112+
module UI5FormulaInjection implements DataFlow::ConfigSig {
113+
predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
114114

115-
override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
116-
117-
override predicate isSink(DataFlow::Node node) {
115+
predicate isSink(DataFlow::Node node) {
118116
exists(StoragePutCall storagePutCall | node = storagePutCall.getArgument(1))
119117
or
120118
exists(FileSaveCall fileSaveCall |

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
import javascript
22
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
3-
private import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph
4-
import semmle.javascript.security.dataflow.LogInjectionQuery as LogInjection
3+
import semmle.javascript.security.dataflow.LogInjectionQuery
54

6-
class UI5LogInjectionConfiguration extends LogInjection::LogInjectionConfiguration {
7-
override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
5+
module UI5LogInjection implements DataFlow::ConfigSig {
6+
predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
87

9-
override predicate isSink(DataFlow::Node node) {
8+
predicate isBarrier(DataFlow::Node node) { LogInjectionConfig::isBarrier(node) }
9+
10+
predicate isSink(DataFlow::Node node) {
1011
node = ModelOutput::getASinkNode("ui5-log-injection").asSink()
1112
}
1213
}

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

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import javascript
2+
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
3+
import advanced_security.javascript.frameworks.ui5.UI5LogInjectionQuery
4+
5+
class ClientRequestInjectionVector extends DataFlow::Node {
6+
ClientRequestInjectionVector() {
7+
exists(ClientRequest req |
8+
this = req.getUrl() or
9+
this = req.getADataNode()
10+
)
11+
}
12+
}
13+
14+
class UI5LogEntryFlowState extends string {
15+
UI5LogEntryFlowState() { this = ["not-logged-not-accessed", "logged-and-accessed"] }
16+
}
17+
18+
module UI5LogEntryToHttp implements DataFlow::StateConfigSig {
19+
class FlowState = UI5LogEntryFlowState;
20+
21+
predicate isSource(DataFlow::Node node, FlowState state) {
22+
node instanceof RemoteFlowSource and
23+
state = "not-logged-not-accessed"
24+
}
25+
26+
predicate isAdditionalFlowStep(
27+
DataFlow::Node start, FlowState preState, DataFlow::Node end, FlowState postState
28+
) {
29+
UI5LogInjection::isAdditionalFlowStep(start, end) and
30+
preState = postState
31+
or
32+
exists(LogArgumentToListener logArgumentToListener |
33+
logArgumentToListener.step(start, end) and
34+
preState = "not-logged-not-accessed" and
35+
postState = "logged-and-accessed"
36+
)
37+
}
38+
39+
predicate isSink(DataFlow::Node node, FlowState state) {
40+
node instanceof ClientRequestInjectionVector and
41+
state = "logged-and-accessed"
42+
}
43+
}

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
import javascript
2+
3+
module UI5PathInjection implements DataFlow::ConfigSig {
4+
predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
5+
6+
predicate isSink(DataFlow::Node node) {
7+
node = ModelOutput::getASinkNode("ui5-path-injection").asSink()
8+
}
9+
}

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
import javascript
2+
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
3+
import semmle.javascript.security.dataflow.LogInjectionQuery
4+
5+
module UI5UnsafeLogAccess implements DataFlow::ConfigSig {
6+
predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
7+
8+
predicate isBarrier(DataFlow::Node node) { LogInjectionConfig::isBarrier(node) }
9+
10+
predicate isSink(DataFlow::Node node) {
11+
node = ModelOutput::getASinkNode("ui5-log-injection").asSink()
12+
}
13+
}
14+
15+
private newtype TLogEntriesNode =
16+
TDataFlowNode(DataFlow::Node node) {
17+
node = ModelOutput::getATypeNode("SapLogEntries").getInducingNode()
18+
} or
19+
TUI5ControlNode(UI5Control control) { control.getImportPath() = "sap/ui/vk/Notifications" }
20+
21+
class LogEntriesNode extends TLogEntriesNode {
22+
DataFlow::Node asDataFlowNode() { this = TDataFlowNode(result) }
23+
24+
UI5Control asUI5ControlNode() { this = TUI5ControlNode(result) }
25+
26+
File getFile() {
27+
result = this.asDataFlowNode().getFile()
28+
or
29+
result = this.asUI5ControlNode().getView()
30+
}
31+
32+
string toString() {
33+
result = this.asDataFlowNode().toString()
34+
or
35+
result = this.asUI5ControlNode().toString()
36+
}
37+
38+
predicate hasLocationInfo(
39+
string filepath, int startline, int startcolumn, int endline, int endcolumn
40+
) {
41+
this.asDataFlowNode().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
42+
or
43+
this.asUI5ControlNode().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
44+
}
45+
}

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

Lines changed: 25 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,39 +1,18 @@
11
import javascript
22
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow as UI5DataFlow
33
import advanced_security.javascript.frameworks.ui5.UI5View
4-
import semmle.javascript.security.dataflow.DomBasedXssQuery as DomBasedXss
4+
private import semmle.javascript.security.dataflow.DomBasedXssQuery as DomBasedXss
55

6-
class Configuration extends DomBasedXss::Configuration {
7-
override predicate isSource(DataFlow::Node start) {
8-
super.isSource(start)
6+
module UI5Xss implements DataFlow::ConfigSig {
7+
predicate isSource(DataFlow::Node start) {
8+
DomBasedXss::DomBasedXssConfig::isSource(start, _)
99
or
1010
start instanceof RemoteFlowSource
1111
}
1212

13-
override predicate isAdditionalFlowStep(
14-
DataFlow::Node start, DataFlow::Node end, DataFlow::FlowLabel inLabel,
15-
DataFlow::FlowLabel outLabel
16-
) {
17-
/* Already an additional flow step defined in `DomBasedXssQuery::Configuration` */
18-
super.isAdditionalFlowStep(start, end, inLabel, outLabel)
19-
or
20-
/* TODO: Legacy code */
21-
/* Handler argument node to handler parameter */
22-
exists(UI5Handler h |
23-
start = h.getBindingPath().getNode() and
24-
/*
25-
* Ideally we would like to show an intermediate node where
26-
* the handler is bound to a control, but there is no sourceNode there
27-
* `end = h.getBindingPath() or start = h.getBindingPath()`
28-
*/
29-
30-
end = h.getParameter(0)
31-
)
32-
}
33-
34-
override predicate isBarrier(DataFlow::Node node) {
13+
predicate isBarrier(DataFlow::Node node) {
3514
/* 1. Already a sanitizer defined in `DomBasedXssQuery::Configuration` */
36-
super.isSanitizer(node)
15+
DomBasedXss::DomBasedXssConfig::isBarrier(node)
3716
or
3817
/* 2. Value read from a non-string control property */
3918
exists(PropertyMetadata m | not m.isUnrestrictedStringType() | node = m)
@@ -53,10 +32,28 @@ class Configuration extends DomBasedXss::Configuration {
5332
["encodeCSS", "encodeJS", "encodeURL", "encodeURLParameters", "encodeXML", "encodeHTML"]
5433
}
5534

56-
override predicate isSink(DataFlow::Node node) {
35+
predicate isSink(DataFlow::Node node) {
5736
node instanceof UI5ExtHtmlISink or
5837
node instanceof UI5ModelHtmlISink
5938
}
39+
40+
predicate isAdditionalFlowStep(DataFlow::Node start, DataFlow::Node end) {
41+
/* Already an additional flow step defined in `DomBasedXssQuery::Configuration` */
42+
DomBasedXss::DomBasedXssConfig::isAdditionalFlowStep(start, _, end, _)
43+
or
44+
/* TODO: Legacy code */
45+
/* Handler argument node to handler parameter */
46+
exists(UI5Handler h |
47+
start = h.getBindingPath().getNode() and
48+
/*
49+
* Ideally we would like to show an intermediate node where
50+
* the handler is bound to a control, but there is no sourceNode there
51+
* `end = h.getBindingPath() or start = h.getBindingPath()`
52+
*/
53+
54+
end = h.getParameter(0)
55+
)
56+
}
6057
}
6158

6259
/**

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

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import advanced_security.javascript.frameworks.ui5.UI5
44
import advanced_security.javascript.frameworks.ui5.UI5View
55
import advanced_security.javascript.frameworks.ui5.RemoteFlowSources
66
import advanced_security.javascript.frameworks.ui5.dataflow.FlowSteps
7-
private import StdLibDataFlow::DataFlow::PathGraph as DataFlowPathGraph
87
private import PatchDataFlow
98

109
/**
@@ -60,7 +59,7 @@ class LocalModelContentBoundBidirectionallyToHtmlISinkControl extends DomBasedXs
6059
UI5Control getControlDeclaration() { result = controlDeclaration }
6160
}
6261

63-
module UI5PathGraph {
62+
module UI5PathGraph<PathNodeSig ConfigPathNode, PathGraphSig<ConfigPathNode> ConfigPathGraph> {
6463
private newtype TNode =
6564
TUI5BindingPathNode(UI5BindingPath path) or
6665
TDataFlowNode(DataFlow::Node node)
@@ -92,7 +91,7 @@ module UI5PathGraph {
9291
.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
9392
}
9493

95-
DataFlow::PathNode getPathNode() { result.getNode() = this.asDataFlowNode() }
94+
ConfigPathNode getPathNode() { result.getNode() = this.asDataFlowNode() }
9695

9796
UI5PathNode getAPrimarySource() {
9897
if this.asDataFlowNode() instanceof LocalModelContentBoundBidirectionallyToSourceControl
@@ -124,9 +123,9 @@ module UI5PathGraph {
124123
query predicate nodes(UI5PathNode ui5PathNode) {
125124
exists(ui5PathNode.asUI5BindingPathNode())
126125
or
127-
exists(DataFlow::PathNode pathNode |
126+
exists(ConfigPathNode pathNode |
128127
pathNode.getNode() = ui5PathNode.asDataFlowNode() and
129-
DataFlowPathGraph::nodes(pathNode)
128+
ConfigPathGraph::nodes(pathNode, _, _)
130129
)
131130
}
132131

@@ -182,10 +181,10 @@ module UI5PathGraph {
182181

183182
query predicate edges(UI5PathNode ui5PathNodePred, UI5PathNode ui5PathNodeSucc) {
184183
/* Include all existing dataflow edges */
185-
exists(DataFlow::PathNode pathNodeFrom, DataFlow::PathNode pathNodeTo |
184+
exists(ConfigPathNode pathNodeFrom, ConfigPathNode pathNodeTo |
186185
pathNodeFrom.getNode() = ui5PathNodePred.asDataFlowNode() and
187186
pathNodeTo.getNode() = ui5PathNodeSucc.asDataFlowNode() and
188-
DataFlowPathGraph::edges(pathNodeFrom, pathNodeTo)
187+
ConfigPathGraph::edges(pathNodeFrom, pathNodeTo, _, _)
189188
) and
190189
/* ========= TODO: Legacy code ========= */
191190
/* Exclude duplicate edge from model to handler parameter */

‎javascript/frameworks/ui5/src/UI5FormulaInjection/UI5FormulaInjection.ql‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,21 @@
1313

1414
import javascript
1515
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
16-
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph
1716
import advanced_security.javascript.frameworks.ui5.UI5FormulaInjectionQuery
1817

18+
module UI5FormulaInjectionFlow = TaintTracking::Global<UI5FormulaInjection>;
19+
20+
module UI5FormulaInjectionUI5PathGraph =
21+
UI5PathGraph<UI5FormulaInjectionFlow::PathNode, UI5FormulaInjectionFlow::PathGraph>;
22+
23+
import UI5FormulaInjectionUI5PathGraph
24+
1925
from
20-
UI5FormulaInjectionConfiguration config, UI5PathNode source, UI5PathNode sink,
21-
UI5PathNode primarySource
26+
UI5FormulaInjectionUI5PathGraph::UI5PathNode source,
27+
UI5FormulaInjectionUI5PathGraph::UI5PathNode sink,
28+
UI5FormulaInjectionUI5PathGraph::UI5PathNode primarySource
2229
where
23-
config.hasFlowPath(source.getPathNode(), sink.getPathNode()) and
30+
UI5FormulaInjectionFlow::flowPath(source.getPathNode(), sink.getPathNode()) and
2431
primarySource = source.getAPrimarySource()
2532
select sink, primarySource, sink, "The content of a saved file depends on a $@.", primarySource,
2633
"user-provided value"

‎javascript/frameworks/ui5/src/UI5LogInjection/UI5LogInjection.ql‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,19 @@
1313

1414
import javascript
1515
import advanced_security.javascript.frameworks.ui5.UI5LogInjectionQuery
16-
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph
16+
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
17+
18+
module UI5LogInjectionFlow = TaintTracking::Global<UI5LogInjection>;
19+
20+
module UI5LogInjectionUI5PathGraph =
21+
UI5PathGraph<UI5LogInjectionFlow::PathNode, UI5LogInjectionFlow::PathGraph>;
22+
23+
import UI5LogInjectionUI5PathGraph
1724

1825
from
19-
UI5LogInjectionConfiguration cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource
26+
UI5LogInjectionUI5PathGraph::UI5PathNode source, UI5LogInjectionUI5PathGraph::UI5PathNode sink,
27+
UI5LogInjectionUI5PathGraph::UI5PathNode primarySource
2028
where
21-
cfg.hasFlowPath(source.getPathNode(), sink.getPathNode()) and
29+
UI5LogInjectionFlow::flowPath(source.getPathNode(), sink.getPathNode()) and
2230
primarySource = source.getAPrimarySource()
2331
select sink, primarySource, sink, "Log entry depends on a $@.", primarySource, "user-provided value"

0 commit comments

Comments
 (0)