Skip to content

Commit a87ceed

Browse files
hvitvedhvitved
authored
Merge pull request #16394 from hvitved/dataflow/synth-param-ret-node
Data flow: Synthesize parameter return nodes
2 parents 8a22e22 + bebcd67 commit a87ceed

38 files changed

Lines changed: 978 additions & 467 deletions

File tree

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ProductFlow.qll‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -546,7 +546,7 @@ module ProductFlow {
546546
Flow1::PathGraph::edges(pred1, succ1, _, _) and
547547
exists(ReturnKindExt returnKind |
548548
succ1.getNode() = returnKind.getAnOutNode(call) and
549-
pred1.getNode().(ReturnNodeExt).getKind() = returnKind
549+
paramReturnNode(_, pred1.asParameterReturnNode(), _, returnKind)
550550
)
551551
}
552552

@@ -574,7 +574,7 @@ module ProductFlow {
574574
Flow2::PathGraph::edges(pred2, succ2, _, _) and
575575
exists(ReturnKindExt returnKind |
576576
succ2.getNode() = returnKind.getAnOutNode(call) and
577-
pred2.getNode().(ReturnNodeExt).getKind() = returnKind
577+
paramReturnNode(_, pred2.asParameterReturnNode(), _, returnKind)
578578
)
579579
}
580580

‎cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected‎

Lines changed: 120 additions & 52 deletions
Large diffs are not rendered by default.

‎cpp/ql/test/library-tests/dataflow/fields/path-flow.expected‎

Lines changed: 131 additions & 63 deletions
Large diffs are not rendered by default.

‎cpp/ql/test/query-tests/Security/CWE/CWE-078/semmle/ExecTainted/ExecTainted.expected‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@ edges
5151
| test.cpp:187:18:187:25 | *filename | test.cpp:187:11:187:15 | strncat output argument | provenance | TaintFunction |
5252
| test.cpp:188:11:188:17 | strncat output argument | test.cpp:186:19:186:25 | *command | provenance | |
5353
| test.cpp:188:11:188:17 | strncat output argument | test.cpp:186:19:186:25 | *command | provenance | |
54+
| test.cpp:188:11:188:17 | strncat output argument | test.cpp:186:19:186:25 | *command [Return] | provenance | |
55+
| test.cpp:188:11:188:17 | strncat output argument | test.cpp:186:19:186:25 | *command [Return] | provenance | |
5456
| test.cpp:188:20:188:24 | *flags | test.cpp:188:11:188:17 | strncat output argument | provenance | |
5557
| test.cpp:188:20:188:24 | *flags | test.cpp:188:11:188:17 | strncat output argument | provenance | TaintFunction |
5658
| test.cpp:194:9:194:16 | fread output argument | test.cpp:196:26:196:33 | *filename | provenance | |
@@ -125,6 +127,8 @@ nodes
125127
| test.cpp:183:32:183:38 | *command | semmle.label | *command |
126128
| test.cpp:186:19:186:25 | *command | semmle.label | *command |
127129
| test.cpp:186:19:186:25 | *command | semmle.label | *command |
130+
| test.cpp:186:19:186:25 | *command [Return] | semmle.label | *command [Return] |
131+
| test.cpp:186:19:186:25 | *command [Return] | semmle.label | *command [Return] |
128132
| test.cpp:186:47:186:54 | *filename | semmle.label | *filename |
129133
| test.cpp:187:11:187:15 | strncat output argument | semmle.label | strncat output argument |
130134
| test.cpp:187:11:187:15 | strncat output argument | semmle.label | strncat output argument |
@@ -151,8 +155,8 @@ nodes
151155
subpaths
152156
| test.cpp:196:26:196:33 | *filename | test.cpp:186:47:186:54 | *filename | test.cpp:186:19:186:25 | *command | test.cpp:196:10:196:16 | concat output argument |
153157
| test.cpp:196:26:196:33 | *filename | test.cpp:186:47:186:54 | *filename | test.cpp:186:19:186:25 | *command | test.cpp:196:10:196:16 | concat output argument |
154-
| test.cpp:196:26:196:33 | *filename | test.cpp:186:47:186:54 | *filename | test.cpp:188:11:188:17 | strncat output argument | test.cpp:196:10:196:16 | concat output argument |
155-
| test.cpp:196:26:196:33 | *filename | test.cpp:186:47:186:54 | *filename | test.cpp:188:11:188:17 | strncat output argument | test.cpp:196:10:196:16 | concat output argument |
158+
| test.cpp:196:26:196:33 | *filename | test.cpp:186:47:186:54 | *filename | test.cpp:186:19:186:25 | *command [Return] | test.cpp:196:10:196:16 | concat output argument |
159+
| test.cpp:196:26:196:33 | *filename | test.cpp:186:47:186:54 | *filename | test.cpp:186:19:186:25 | *command [Return] | test.cpp:196:10:196:16 | concat output argument |
156160
#select
157161
| test.cpp:23:12:23:19 | command1 | test.cpp:15:27:15:30 | **argv | test.cpp:23:12:23:19 | *command1 | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:15:27:15:30 | **argv | user input (a command-line argument) | test.cpp:22:13:22:20 | sprintf output argument | sprintf output argument |
158162
| test.cpp:51:10:51:16 | command | test.cpp:47:21:47:26 | *call to getenv | test.cpp:51:10:51:16 | *command | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:47:21:47:26 | *call to getenv | user input (an environment variable) | test.cpp:50:11:50:17 | sprintf output argument | sprintf output argument |

‎cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/OverrunWriteProductFlow.expected‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ edges
5353
| test.cpp:228:27:228:54 | call to malloc | test.cpp:228:27:228:54 | call to malloc | provenance | |
5454
| test.cpp:228:27:228:54 | call to malloc | test.cpp:232:10:232:15 | buffer | provenance | |
5555
| test.cpp:235:40:235:45 | buffer | test.cpp:236:5:236:26 | ... = ... | provenance | |
56+
| test.cpp:236:5:236:9 | *p_str [post update] [string] | test.cpp:235:27:235:31 | *p_str [Return] [string] | provenance | |
5657
| test.cpp:236:5:236:9 | *p_str [post update] [string] | test.cpp:235:27:235:31 | *p_str [string] | provenance | |
5758
| test.cpp:236:5:236:26 | ... = ... | test.cpp:236:5:236:9 | *p_str [post update] [string] | provenance | |
5859
| test.cpp:241:20:241:38 | call to malloc | test.cpp:241:20:241:38 | call to malloc | provenance | |
@@ -128,6 +129,7 @@ nodes
128129
| test.cpp:228:27:228:54 | call to malloc | semmle.label | call to malloc |
129130
| test.cpp:228:27:228:54 | call to malloc | semmle.label | call to malloc |
130131
| test.cpp:232:10:232:15 | buffer | semmle.label | buffer |
132+
| test.cpp:235:27:235:31 | *p_str [Return] [string] | semmle.label | *p_str [Return] [string] |
131133
| test.cpp:235:27:235:31 | *p_str [string] | semmle.label | *p_str [string] |
132134
| test.cpp:235:40:235:45 | buffer | semmle.label | buffer |
133135
| test.cpp:236:5:236:9 | *p_str [post update] [string] | semmle.label | *p_str [post update] [string] |
@@ -150,8 +152,8 @@ nodes
150152
| test.cpp:264:13:264:30 | call to malloc | semmle.label | call to malloc |
151153
| test.cpp:266:12:266:12 | p | semmle.label | p |
152154
subpaths
155+
| test.cpp:242:22:242:27 | buffer | test.cpp:235:40:235:45 | buffer | test.cpp:235:27:235:31 | *p_str [Return] [string] | test.cpp:242:16:242:19 | set_string output argument [string] |
153156
| test.cpp:242:22:242:27 | buffer | test.cpp:235:40:235:45 | buffer | test.cpp:235:27:235:31 | *p_str [string] | test.cpp:242:16:242:19 | set_string output argument [string] |
154-
| test.cpp:242:22:242:27 | buffer | test.cpp:235:40:235:45 | buffer | test.cpp:236:5:236:9 | *p_str [post update] [string] | test.cpp:242:16:242:19 | set_string output argument [string] |
155157
#select
156158
| test.cpp:42:5:42:11 | call to strncpy | test.cpp:18:19:18:24 | call to malloc | test.cpp:42:18:42:23 | string | This write may overflow $@ by 1 element. | test.cpp:42:18:42:23 | string | string |
157159
| test.cpp:72:9:72:15 | call to strncpy | test.cpp:18:19:18:24 | call to malloc | test.cpp:72:22:72:27 | string | This write may overflow $@ by 1 element. | test.cpp:72:22:72:27 | string | string |

‎cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/ExposedSystemData.expected‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ edges
1212
| tests2.cpp:111:14:111:15 | *c1 [*ptr] | tests2.cpp:111:14:111:19 | *ptr | provenance | |
1313
| tests2.cpp:111:14:111:15 | *c1 [*ptr] | tests2.cpp:111:17:111:19 | *ptr | provenance | |
1414
| tests2.cpp:111:17:111:19 | *ptr | tests2.cpp:111:14:111:19 | *ptr | provenance | |
15-
| tests2.cpp:120:5:120:21 | [summary param] 1 indirection in zmq_msg_init_data | tests2.cpp:120:5:120:21 | [summary] to write: Argument[0 indirection] in zmq_msg_init_data | provenance | |
15+
| tests2.cpp:120:5:120:21 | [summary param] 1 indirection in zmq_msg_init_data | tests2.cpp:120:5:120:21 | [summary param] 0 indirection in zmq_msg_init_data [Return] | provenance | |
1616
| tests2.cpp:134:2:134:30 | *... = ... | tests2.cpp:138:23:138:34 | *message_data | provenance | |
1717
| tests2.cpp:134:2:134:30 | *... = ... | tests2.cpp:143:34:143:45 | *message_data | provenance | |
1818
| tests2.cpp:134:17:134:22 | *call to getenv | tests2.cpp:134:2:134:30 | *... = ... | provenance | |
@@ -52,8 +52,8 @@ nodes
5252
| tests2.cpp:111:14:111:15 | *c1 [*ptr] | semmle.label | *c1 [*ptr] |
5353
| tests2.cpp:111:14:111:19 | *ptr | semmle.label | *ptr |
5454
| tests2.cpp:111:17:111:19 | *ptr | semmle.label | *ptr |
55+
| tests2.cpp:120:5:120:21 | [summary param] 0 indirection in zmq_msg_init_data [Return] | semmle.label | [summary param] 0 indirection in zmq_msg_init_data [Return] |
5556
| tests2.cpp:120:5:120:21 | [summary param] 1 indirection in zmq_msg_init_data | semmle.label | [summary param] 1 indirection in zmq_msg_init_data |
56-
| tests2.cpp:120:5:120:21 | [summary] to write: Argument[0 indirection] in zmq_msg_init_data | semmle.label | [summary] to write: Argument[0 indirection] in zmq_msg_init_data |
5757
| tests2.cpp:134:2:134:30 | *... = ... | semmle.label | *... = ... |
5858
| tests2.cpp:134:17:134:22 | *call to getenv | semmle.label | *call to getenv |
5959
| tests2.cpp:138:23:138:34 | *message_data | semmle.label | *message_data |
@@ -74,7 +74,7 @@ nodes
7474
| tests_sysconf.cpp:36:21:36:27 | confstr output argument | semmle.label | confstr output argument |
7575
| tests_sysconf.cpp:39:19:39:25 | *pathbuf | semmle.label | *pathbuf |
7676
subpaths
77-
| tests2.cpp:143:34:143:45 | *message_data | tests2.cpp:120:5:120:21 | [summary param] 1 indirection in zmq_msg_init_data | tests2.cpp:120:5:120:21 | [summary] to write: Argument[0 indirection] in zmq_msg_init_data | tests2.cpp:143:24:143:31 | zmq_msg_init_data output argument |
77+
| tests2.cpp:143:34:143:45 | *message_data | tests2.cpp:120:5:120:21 | [summary param] 1 indirection in zmq_msg_init_data | tests2.cpp:120:5:120:21 | [summary param] 0 indirection in zmq_msg_init_data [Return] | tests2.cpp:143:24:143:31 | zmq_msg_init_data output argument |
7878
#select
7979
| tests2.cpp:63:13:63:26 | *call to getenv | tests2.cpp:63:13:63:26 | *call to getenv | tests2.cpp:63:13:63:26 | *call to getenv | This operation exposes system data from $@. | tests2.cpp:63:13:63:26 | *call to getenv | *call to getenv |
8080
| tests2.cpp:64:13:64:26 | *call to getenv | tests2.cpp:64:13:64:26 | *call to getenv | tests2.cpp:64:13:64:26 | *call to getenv | This operation exposes system data from $@. | tests2.cpp:64:13:64:26 | *call to getenv | *call to getenv |

‎csharp/ql/src/utils/modelgenerator/internal/CaptureModels.qll‎

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,29 @@
66
private import CaptureModelsSpecific
77
private import CaptureModelsPrinting
88

9+
/**
10+
* A node from which flow can return to the caller. This is either a regular
11+
* `ReturnNode` or a `PostUpdateNode` corresponding to the value of a parameter.
12+
*/
13+
private class ReturnNodeExt extends DataFlow::Node {
14+
private DataFlowImplCommon::ReturnKindExt kind;
15+
16+
ReturnNodeExt() {
17+
kind = DataFlowImplCommon::getValueReturnPosition(this).getKind() or
18+
kind = DataFlowImplCommon::getParamReturnPosition(this, _).getKind()
19+
}
20+
21+
string getOutput() {
22+
kind instanceof DataFlowImplCommon::ValueReturnKind and
23+
result = "ReturnValue"
24+
or
25+
exists(ParameterPosition pos |
26+
pos = kind.(DataFlowImplCommon::ParamUpdateReturnKind).getPosition() and
27+
result = paramReturnNodeAsOutput(returnNodeEnclosingCallable(this), pos)
28+
)
29+
}
30+
}
31+
932
class DataFlowTargetApi extends TargetApiSpecific {
1033
DataFlowTargetApi() { not isUninterestingForDataFlowModels(this) }
1134
}
@@ -65,7 +88,7 @@ string asInputArgument(DataFlow::Node source) { result = asInputArgumentSpecific
6588
* Gets the summary model of `api`, if it follows the `fluent` programming pattern (returns `this`).
6689
*/
6790
string captureQualifierFlow(TargetApiSpecific api) {
68-
exists(DataFlowImplCommon::ReturnNodeExt ret |
91+
exists(ReturnNodeExt ret |
6992
api = returnNodeEnclosingCallable(ret) and
7093
isOwnInstanceAccessNode(ret)
7194
) and
@@ -130,7 +153,7 @@ module ThroughFlowConfig implements DataFlow::StateConfigSig {
130153
}
131154

132155
predicate isSink(DataFlow::Node sink, FlowState state) {
133-
sink instanceof DataFlowImplCommon::ReturnNodeExt and
156+
sink instanceof ReturnNodeExt and
134157
not isOwnInstanceAccessNode(sink) and
135158
not exists(captureQualifierFlow(sink.asExpr().getEnclosingCallable())) and
136159
(state instanceof TaintRead or state instanceof TaintStore)
@@ -171,14 +194,11 @@ private module ThroughFlow = TaintTracking::GlobalWithState<ThroughFlowConfig>;
171194
* Gets the summary model(s) of `api`, if there is flow from parameters to return value or parameter.
172195
*/
173196
string captureThroughFlow(DataFlowTargetApi api) {
174-
exists(
175-
DataFlow::ParameterNode p, DataFlowImplCommon::ReturnNodeExt returnNodeExt, string input,
176-
string output
177-
|
197+
exists(DataFlow::ParameterNode p, ReturnNodeExt returnNodeExt, string input, string output |
178198
ThroughFlow::flow(p, returnNodeExt) and
179199
returnNodeExt.(DataFlow::Node).getEnclosingCallable() = api and
180200
input = parameterNodeAsInput(p) and
181-
output = returnNodeAsOutput(returnNodeExt) and
201+
output = returnNodeExt.getOutput() and
182202
input != output and
183203
result = ModelPrinting::asTaintModel(api, input, output)
184204
)
@@ -196,7 +216,7 @@ module FromSourceConfig implements DataFlow::ConfigSig {
196216

197217
predicate isSink(DataFlow::Node sink) {
198218
exists(DataFlowTargetApi c |
199-
sink instanceof DataFlowImplCommon::ReturnNodeExt and
219+
sink instanceof ReturnNodeExt and
200220
sink.getEnclosingCallable() = c
201221
)
202222
}
@@ -214,12 +234,12 @@ private module FromSource = TaintTracking::Global<FromSourceConfig>;
214234
* Gets the source model(s) of `api`, if there is flow from an existing known source to the return of `api`.
215235
*/
216236
string captureSource(DataFlowTargetApi api) {
217-
exists(DataFlow::Node source, DataFlow::Node sink, string kind |
237+
exists(DataFlow::Node source, ReturnNodeExt sink, string kind |
218238
FromSource::flow(source, sink) and
219239
ExternalFlow::sourceNode(source, kind) and
220240
api = sink.getEnclosingCallable() and
221241
isRelevantSourceKind(kind) and
222-
result = ModelPrinting::asSourceModel(api, returnNodeAsOutput(sink), kind)
242+
result = ModelPrinting::asSourceModel(api, sink.getOutput(), kind)
223243
)
224244
}
225245

‎csharp/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll‎

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ private import semmle.code.csharp.frameworks.system.linq.Expressions
1111
import semmle.code.csharp.dataflow.internal.ExternalFlow as ExternalFlow
1212
import semmle.code.csharp.dataflow.internal.DataFlowImplCommon as DataFlowImplCommon
1313
import semmle.code.csharp.dataflow.internal.DataFlowPrivate as DataFlowPrivate
14+
import semmle.code.csharp.dataflow.internal.DataFlowDispatch as DataFlowDispatch
1415

1516
module DataFlow = CS::DataFlow;
1617

@@ -133,32 +134,24 @@ string parameterAccess(CS::Parameter p) {
133134

134135
class InstanceParameterNode = DataFlowPrivate::InstanceParameterNode;
135136

136-
pragma[nomagic]
137-
private CS::Parameter getParameter(DataFlowImplCommon::ReturnNodeExt node, ParameterPosition pos) {
138-
result = node.(DataFlow::Node).getEnclosingCallable().getParameter(pos.getPosition())
139-
}
137+
class ParameterPosition = DataFlowDispatch::ParameterPosition;
140138

141139
/**
142-
* Gets the MaD string representation of the the return node `node`.
140+
* Gets the MaD string representation of return through parameter at position
141+
* `pos` of callable `c`.
143142
*/
144-
string returnNodeAsOutput(DataFlowImplCommon::ReturnNodeExt node) {
145-
if node.getKind() instanceof DataFlowImplCommon::ValueReturnKind
146-
then result = "ReturnValue"
147-
else
148-
exists(ParameterPosition pos |
149-
pos = node.getKind().(DataFlowImplCommon::ParamUpdateReturnKind).getPosition()
150-
|
151-
result = parameterAccess(getParameter(node, pos))
152-
or
153-
pos.isThisParameter() and
154-
result = qualifierString()
155-
)
143+
bindingset[c]
144+
string paramReturnNodeAsOutput(CS::Callable c, ParameterPosition pos) {
145+
result = parameterAccess(c.getParameter(pos.getPosition()))
146+
or
147+
pos.isThisParameter() and
148+
result = qualifierString()
156149
}
157150

158151
/**
159152
* Gets the enclosing callable of `ret`.
160153
*/
161-
CS::Callable returnNodeEnclosingCallable(DataFlowImplCommon::ReturnNodeExt ret) {
154+
CS::Callable returnNodeEnclosingCallable(DataFlow::Node ret) {
162155
result = DataFlowImplCommon::getNodeEnclosingCallable(ret).asCallable()
163156
}
164157

0 commit comments

Comments
 (0)