Skip to content

Commit ba67217

Browse files
jketemajketema
authored
Merge pull request github#14571 from MathiasVP/fix-indirect-taint
C++: Fix indirect taint
2 parents 26f7670 + 1fce265 commit ba67217

File tree

7 files changed

+53
-26
lines changed

7 files changed

+53
-26
lines changed

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

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -32,25 +32,34 @@ DataFlow::Node callInput(CallInstruction call, FunctionInput input) {
3232
}
3333

3434
/**
35-
* Gets the instruction that holds the `output` for `call`.
35+
* Gets the node that represents the output of `call` with kind `output` at
36+
* indirection index `indirectionIndex`.
3637
*/
37-
Node callOutput(CallInstruction call, FunctionOutput output) {
38+
private Node callOutputWithIndirectionIndex(
39+
CallInstruction call, FunctionOutput output, int indirectionIndex
40+
) {
3841
// The return value
3942
simpleOutNode(result, call) and
40-
output.isReturnValue()
43+
output.isReturnValue() and
44+
indirectionIndex = 0
4145
or
4246
// The side effect of a call on the value pointed to by an argument or qualifier
43-
exists(int index, int indirectionIndex |
47+
exists(int index |
4448
result.(IndirectArgumentOutNode).getArgumentIndex() = index and
45-
result.(IndirectArgumentOutNode).getIndirectionIndex() = indirectionIndex and
49+
result.(IndirectArgumentOutNode).getIndirectionIndex() = indirectionIndex - 1 and
4650
result.(IndirectArgumentOutNode).getCallInstruction() = call and
47-
output.isParameterDerefOrQualifierObject(index, indirectionIndex)
51+
output.isParameterDerefOrQualifierObject(index, indirectionIndex - 1)
4852
)
4953
or
50-
exists(int ind |
51-
result = getIndirectReturnOutNode(call, ind) and
52-
output.isReturnValueDeref(ind)
53-
)
54+
result = getIndirectReturnOutNode(call, indirectionIndex) and
55+
output.isReturnValueDeref(indirectionIndex)
56+
}
57+
58+
/**
59+
* Gets the instruction that holds the `output` for `call`.
60+
*/
61+
Node callOutput(CallInstruction call, FunctionOutput output) {
62+
result = callOutputWithIndirectionIndex(call, output, _)
5463
}
5564

5665
DataFlow::Node callInput(CallInstruction call, FunctionInput input, int d) {
@@ -76,19 +85,15 @@ private IndirectReturnOutNode getIndirectReturnOutNode(CallInstruction call, int
7685
*/
7786
bindingset[d]
7887
Node callOutput(CallInstruction call, FunctionOutput output, int d) {
79-
exists(DataFlow::Node n | n = callOutput(call, output) and d > 0 |
88+
exists(DataFlow::Node n, int indirectionIndex |
89+
n = callOutputWithIndirectionIndex(call, output, indirectionIndex) and d > 0
90+
|
8091
// The return value
81-
result = getIndirectReturnOutNode(n.asInstruction(), d)
92+
result = callOutputWithIndirectionIndex(call, output, indirectionIndex + d)
8293
or
8394
// If there isn't an indirect out node for the call with indirection `d` then
8495
// we conflate this with the underlying `CallInstruction`.
85-
not exists(getIndirectReturnOutNode(call, d)) and
96+
not exists(getIndirectReturnOutNode(call, indirectionIndex + d)) and
8697
n = result
87-
or
88-
// The side effect of a call on the value pointed to by an argument or qualifier
89-
exists(Operand operand, int indirectionIndex |
90-
Ssa::outNodeHasAddressAndIndex(n, operand, indirectionIndex) and
91-
Ssa::outNodeHasAddressAndIndex(result, operand, indirectionIndex + d)
92-
)
9398
)
9499
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ private class PointerWrapperTypeIndirection extends Indirection instanceof Point
228228
override predicate isAdditionalDereference(Instruction deref, Operand address) {
229229
exists(CallInstruction call |
230230
operandForFullyConvertedCall(getAUse(deref), call) and
231-
this = call.getStaticCallTarget().getClassAndName("operator*") and
231+
this = call.getStaticCallTarget().getClassAndName(["operator*", "operator->", "get"]) and
232232
address = call.getThisArgumentOperand()
233233
)
234234
}

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,8 @@ reverseRead
2020
argHasPostUpdate
2121
postWithInFlow
2222
| test.cpp:384:10:384:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
23-
| test.cpp:384:10:384:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
24-
| test.cpp:391:10:391:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
2523
| test.cpp:391:10:391:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
2624
| test.cpp:400:10:400:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
27-
| test.cpp:400:10:400:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
28-
| test.cpp:407:10:407:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
2925
| test.cpp:407:10:407:13 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
3026
viableImplInCallContextTooLarge
3127
uniqueParameterNodeAtPosition

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,6 @@ reverseRead
4444
argHasPostUpdate
4545
postWithInFlow
4646
| realistic.cpp:54:16:54:47 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
47-
| realistic.cpp:54:16:54:47 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
48-
| realistic.cpp:60:16:60:18 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
4947
| realistic.cpp:60:16:60:18 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
5048
viableImplInCallContextTooLarge
5149
uniqueParameterNodeAtPosition

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2199,6 +2199,19 @@ WARNING: Module TaintTracking has been deprecated and may be removed in future (
21992199
| map.cpp:436:55:436:59 | def | map.cpp:436:19:436:60 | call to pair | TAINT |
22002200
| map.cpp:436:63:436:67 | first | map.cpp:436:7:436:67 | call to iterator | |
22012201
| map.cpp:437:7:437:9 | m35 | map.cpp:437:7:437:9 | call to unordered_map | |
2202+
| map.cpp:446:23:446:23 | call to map | map.cpp:448:3:448:3 | m | |
2203+
| map.cpp:446:23:446:23 | call to map | map.cpp:449:12:449:12 | m | |
2204+
| map.cpp:446:23:446:23 | call to map | map.cpp:451:1:451:1 | m | |
2205+
| map.cpp:447:12:447:26 | call to indirect_source | map.cpp:448:10:448:10 | p | |
2206+
| map.cpp:448:3:448:3 | m | map.cpp:448:4:448:4 | call to operator[] | TAINT |
2207+
| map.cpp:448:3:448:3 | ref arg m | map.cpp:449:12:449:12 | m | |
2208+
| map.cpp:448:3:448:3 | ref arg m | map.cpp:451:1:451:1 | m | |
2209+
| map.cpp:448:3:448:10 | ... = ... | map.cpp:448:4:448:4 | call to operator[] [post update] | |
2210+
| map.cpp:448:4:448:4 | call to operator[] [post update] | map.cpp:448:3:448:3 | ref arg m | TAINT |
2211+
| map.cpp:448:10:448:10 | p | map.cpp:448:3:448:10 | ... = ... | |
2212+
| map.cpp:449:12:449:12 | m | map.cpp:449:13:449:13 | call to operator[] | TAINT |
2213+
| map.cpp:449:12:449:12 | ref arg m | map.cpp:451:1:451:1 | m | |
2214+
| map.cpp:449:13:449:13 | call to operator[] | map.cpp:450:8:450:8 | q | |
22022215
| movableclass.cpp:8:2:8:15 | this | movableclass.cpp:8:27:8:31 | constructor init of field v [pre-this] | |
22032216
| movableclass.cpp:8:21:8:22 | _v | movableclass.cpp:8:29:8:30 | _v | |
22042217
| movableclass.cpp:8:29:8:30 | _v | movableclass.cpp:8:27:8:31 | constructor init of field v | TAINT |

‎cpp/ql/test/library-tests/dataflow/taint-tests/map.cpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,3 +436,16 @@ void test_unordered_map()
436436
sink(m35.emplace(std::pair<char *, char *>(source(), "def")).first); // $ MISSING: ast,ir
437437
sink(m35); // $ MISSING: ast,ir
438438
}
439+
440+
namespace {
441+
int* indirect_source();
442+
void indirect_sink(int*);
443+
}
444+
445+
void test_indirect_taint() {
446+
std::map<int, int*> m;
447+
int* p = indirect_source();
448+
m[1] = p;
449+
int* q = m[1];
450+
sink(q); // $ ir MISSING: ast
451+
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,8 @@ module IRTest {
8484
or
8585
source.asIndirectExpr().(FunctionCall).getTarget().getName() = "source"
8686
or
87+
source.asIndirectExpr().(FunctionCall).getTarget().getName() = "indirect_source"
88+
or
8789
source.asParameter().getName().matches("source%")
8890
or
8991
exists(FunctionCall fc |

0 commit comments

Comments
 (0)