Skip to content

Commit 83d611d

Browse files
author
Robert Marsh
committed
C++: don't conflate pointers in data flow
1 parent 4617940 commit 83d611d

File tree

6 files changed

+67
-19
lines changed

6 files changed

+67
-19
lines changed

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll‎

Lines changed: 41 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -291,25 +291,50 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
291291
// for now.
292292
iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom
293293
or
294-
// Flow from argument to return value
295-
iTo = any(CallInstruction call |
296-
exists(int indexIn |
297-
modelFlowToReturnValue(call.getStaticCallTarget(), indexIn) and
298-
iFrom = getACallArgumentOrIndirection(call, indexIn)
299-
)
300-
)
301-
or
302-
// Flow from input argument to output argument
303-
// TODO: This won't work in practice as long as all aliased memory is tracked
304-
// together in a single virtual variable.
305-
iTo = any(WriteSideEffectInstruction outNode |
306-
exists(CallInstruction call, int indexIn, int indexOut |
307-
modelFlowToParameter(call.getStaticCallTarget(), indexIn, indexOut) and
308-
iFrom = getACallArgumentOrIndirection(call, indexIn) and
309-
outNode.getIndex() = indexOut and
294+
// Flow through modeled functions
295+
modelFlow(iFrom, iTo)
296+
}
297+
298+
private predicate modelFlow(Instruction iFrom, Instruction iTo) {
299+
exists(
300+
CallInstruction call, DataFlowFunction func, FunctionInput modelIn, FunctionOutput modelOut
301+
|
302+
call.getStaticCallTarget() = func and
303+
func.hasDataFlow(modelIn, modelOut)
304+
|
305+
(
306+
modelOut.isReturnValue() and
307+
iTo = call
308+
or
309+
// TODO: Add write side effects for return values
310+
modelOut.isReturnValueDeref() and
311+
iTo = call
312+
or
313+
exists(WriteSideEffectInstruction outNode |
314+
modelOut.isParameterDeref(outNode.getIndex()) and
315+
iTo = outNode and
310316
outNode.getPrimaryInstruction() = call
311317
)
318+
// TODO: add write side effects for qualifiers
319+
) and
320+
(
321+
exists(int index |
322+
modelIn.isParameter(index) and
323+
iFrom = call.getPositionalArgument(index)
324+
)
325+
or
326+
exists(int index, ReadSideEffectInstruction read |
327+
modelIn.isParameterDeref(index) and
328+
read.getIndex() = index and
329+
read.getPrimaryInstruction() = call and
330+
iFrom = read.getSideEffectOperand().getAnyDef()
331+
)
332+
or
333+
modelIn.isQualifierAddress() and
334+
iFrom = call.getThisArgument()
335+
// TODO: add read side effects for qualifiers
312336
)
337+
)
313338
}
314339

315340
/**

‎cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,4 +77,13 @@ void test_dynamic_cast() {
7777
reinterpret_cast<D2*>(b2)->f(getenv("VAR"));
7878

7979
dynamic_cast<D3*>(b2)->f(getenv("VAR")); // tainted [FALSE POSITIVE]
80+
}
81+
82+
namespace std {
83+
template< class T >
84+
T&& move( T&& t ) noexcept;
85+
}
86+
87+
void test_std_move() {
88+
sink(std::move(getenv("VAR")));
8089
}

‎cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,14 @@
8989
| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:79:30:79:35 | call to getenv |
9090
| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:79:30:79:42 | (const char *)... |
9191
| defaulttainttracking.cpp:79:30:79:35 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
92+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
93+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:84:17:84:17 | t |
94+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:16 | call to move |
95+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:32 | (const char *)... |
96+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:32 | (reference dereference) |
97+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:23 | call to getenv |
98+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:30 | (reference to) |
99+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
92100
| test_diff.cpp:92:10:92:13 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
93101
| test_diff.cpp:92:10:92:13 | argv | test_diff.cpp:1:11:1:20 | p#0 |
94102
| test_diff.cpp:92:10:92:13 | argv | test_diff.cpp:92:10:92:13 | argv |

‎cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,12 @@
99
| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:36:39:61 | (const char *)... | AST only |
1010
| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:51:39:61 | env_pointer | AST only |
1111
| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | IR only |
12+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | IR only |
13+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:16 | call to move | IR only |
14+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:32 | (const char *)... | IR only |
15+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:32 | (reference dereference) | IR only |
16+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:30 | (reference to) | IR only |
17+
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | IR only |
1218
| test_diff.cpp:104:12:104:15 | argv | test_diff.cpp:104:11:104:20 | (...) | IR only |
1319
| test_diff.cpp:108:10:108:13 | argv | test_diff.cpp:36:24:36:24 | p | AST only |
1420
| test_diff.cpp:111:10:111:13 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 | AST only |

‎cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@
3232
| taint.cpp:261:7:261:7 | taint.cpp:258:7:258:12 | AST only |
3333
| taint.cpp:351:7:351:7 | taint.cpp:330:6:330:11 | AST only |
3434
| taint.cpp:352:7:352:7 | taint.cpp:330:6:330:11 | AST only |
35+
| taint.cpp:372:7:372:7 | taint.cpp:365:24:365:29 | AST only |
36+
| taint.cpp:374:7:374:7 | taint.cpp:365:24:365:29 | AST only |
37+
| taint.cpp:391:7:391:7 | taint.cpp:385:27:385:32 | AST only |
3538
| taint.cpp:423:7:423:7 | taint.cpp:422:14:422:19 | AST only |
3639
| taint.cpp:424:9:424:17 | taint.cpp:422:14:422:19 | AST only |
3740
| taint.cpp:429:7:429:7 | taint.cpp:428:13:428:18 | IR only |

‎cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,6 @@
1717
| taint.cpp:291:7:291:7 | y | taint.cpp:275:6:275:11 | call to source |
1818
| taint.cpp:337:7:337:7 | t | taint.cpp:330:6:330:11 | call to source |
1919
| taint.cpp:350:7:350:7 | t | taint.cpp:330:6:330:11 | call to source |
20-
| taint.cpp:372:7:372:7 | a | taint.cpp:365:24:365:29 | source |
21-
| taint.cpp:374:7:374:7 | c | taint.cpp:365:24:365:29 | source |
2220
| taint.cpp:382:7:382:7 | a | taint.cpp:377:23:377:28 | source |
23-
| taint.cpp:391:7:391:7 | a | taint.cpp:385:27:385:32 | source |
2421
| taint.cpp:429:7:429:7 | b | taint.cpp:428:13:428:18 | call to source |
2522
| taint.cpp:430:9:430:14 | member | taint.cpp:428:13:428:18 | call to source |

0 commit comments

Comments
 (0)