Skip to content

Commit 721e9d5

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#2704 from rdmarsh2/rdmarsh/cpp/ir-flow-through-outparams
C++: IR dataflow edges through outparams
2 parents 9f18a15 + 8779177 commit 721e9d5

File tree

24 files changed

+557
-138
lines changed

24 files changed

+557
-138
lines changed

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,27 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
199199
// Flow through pointer dereference
200200
i2.(LoadInstruction).getSourceAddress() = i1
201201
or
202-
i2.(UnaryInstruction).getUnary() = i1
202+
// Flow through partial reads of arrays and unions
203+
i2.(LoadInstruction).getSourceValueOperand().getAnyDef() = i1 and
204+
not i1.isResultConflated() and
205+
(
206+
i1.getResultType() instanceof ArrayType or
207+
i1.getResultType() instanceof Union
208+
)
209+
or
210+
// Unary instructions tend to preserve enough information in practice that we
211+
// want taint to flow through.
212+
// The exception is `FieldAddressInstruction`. Together with the rule for
213+
// `LoadInstruction` above and for `ChiInstruction` below, flow through
214+
// `FieldAddressInstruction` could cause flow into one field to come out an
215+
// unrelated field. This would happen across function boundaries, where the IR
216+
// would not be able to match loads to stores.
217+
i2.(UnaryInstruction).getUnary() = i1 and
218+
(
219+
not i2 instanceof FieldAddressInstruction
220+
or
221+
i2.(FieldAddressInstruction).getField().getDeclaringType() instanceof Union
222+
)
203223
or
204224
// Flow out of definition-by-reference
205225
i2.(ChiInstruction).getPartial() = i1.(WriteSideEffectInstruction) and
@@ -213,7 +233,7 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
213233
or
214234
t instanceof ArrayType
215235
or
216-
// Buffers or unknown size
236+
// Buffers of unknown size
217237
t instanceof UnknownType
218238
)
219239
or

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

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,7 @@ private module VirtualDispatch {
7070
// Call return
7171
exists(DataFlowCall call, ReturnKind returnKind |
7272
other = getAnOutNode(call, returnKind) and
73-
src.(ReturnNode).getKind() = returnKind and
74-
call.getStaticCallTarget() = src.getEnclosingCallable()
73+
returnNodeWithKindAndEnclosingCallable(src, returnKind, call.getStaticCallTarget())
7574
) and
7675
allowFromArg = false
7776
or
@@ -125,6 +124,18 @@ private module VirtualDispatch {
125124
}
126125
}
127126

127+
/**
128+
* A ReturnNode with its ReturnKind and its enclosing callable.
129+
*
130+
* Used to fix a join ordering issue in flowsFrom.
131+
*/
132+
private predicate returnNodeWithKindAndEnclosingCallable(
133+
ReturnNode node, ReturnKind kind, DataFlowCallable callable
134+
) {
135+
node.getKind() = kind and
136+
node.getEnclosingCallable() = callable
137+
}
138+
128139
/** Call through a function pointer. */
129140
private class DataSensitiveExprCall extends DataSensitiveCall {
130141
DataSensitiveExprCall() { not exists(this.getStaticCallTarget()) }

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

Lines changed: 62 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,31 +24,86 @@ class ArgumentNode extends InstructionNode {
2424
DataFlowCall getCall() { this.argumentOf(result, _) }
2525
}
2626

27-
private newtype TReturnKind = TNormalReturnKind()
27+
private newtype TReturnKind =
28+
TNormalReturnKind() or
29+
TIndirectReturnKind(ParameterIndex index)
2830

2931
/**
3032
* A return kind. A return kind describes how a value can be returned
3133
* from a callable. For C++, this is simply a function return.
3234
*/
3335
class ReturnKind extends TReturnKind {
3436
/** Gets a textual representation of this return kind. */
35-
string toString() { result = "return" }
37+
abstract string toString();
38+
}
39+
40+
private class NormalReturnKind extends ReturnKind, TNormalReturnKind {
41+
override string toString() { result = "return" }
42+
}
43+
44+
private class IndirectReturnKind extends ReturnKind, TIndirectReturnKind {
45+
ParameterIndex index;
46+
47+
IndirectReturnKind() { this = TIndirectReturnKind(index) }
48+
49+
override string toString() { result = "outparam[" + index.toString() + "]" }
3650
}
3751

3852
/** A data flow node that occurs as the result of a `ReturnStmt`. */
3953
class ReturnNode extends InstructionNode {
40-
ReturnNode() { exists(ReturnValueInstruction ret | this.getInstruction() = ret.getReturnValue()) }
54+
Instruction primary;
55+
56+
ReturnNode() {
57+
exists(ReturnValueInstruction ret | instr = ret.getReturnValue() and primary = ret)
58+
or
59+
exists(ReturnIndirectionInstruction rii |
60+
instr = rii.getSideEffectOperand().getAnyDef() and primary = rii
61+
)
62+
}
4163

4264
/** Gets the kind of this returned value. */
43-
ReturnKind getKind() { result = TNormalReturnKind() }
65+
abstract ReturnKind getKind();
66+
}
67+
68+
class ReturnValueNode extends ReturnNode {
69+
override ReturnValueInstruction primary;
70+
71+
override ReturnKind getKind() { result = TNormalReturnKind() }
72+
}
73+
74+
class ReturnIndirectionNode extends ReturnNode {
75+
override ReturnIndirectionInstruction primary;
76+
77+
override ReturnKind getKind() { result = TIndirectReturnKind(primary.getParameter().getIndex()) }
4478
}
4579

4680
/** A data flow node that represents the output of a call. */
4781
class OutNode extends InstructionNode {
48-
override CallInstruction instr;
82+
OutNode() {
83+
instr instanceof CallInstruction or
84+
instr instanceof WriteSideEffectInstruction
85+
}
4986

5087
/** Gets the underlying call. */
51-
DataFlowCall getCall() { result = instr }
88+
abstract DataFlowCall getCall();
89+
90+
abstract ReturnKind getReturnKind();
91+
}
92+
93+
private class CallOutNode extends OutNode {
94+
override CallInstruction instr;
95+
96+
override DataFlowCall getCall() { result = instr }
97+
98+
override ReturnKind getReturnKind() { result instanceof NormalReturnKind }
99+
}
100+
101+
private class SideEffectOutNode extends OutNode {
102+
override WriteSideEffectInstruction instr;
103+
104+
override DataFlowCall getCall() { result = instr.getPrimaryInstruction() }
105+
106+
override ReturnKind getReturnKind() { result = TIndirectReturnKind(instr.getIndex()) }
52107
}
53108

54109
/**
@@ -57,7 +112,7 @@ class OutNode extends InstructionNode {
57112
*/
58113
OutNode getAnOutNode(DataFlowCall call, ReturnKind kind) {
59114
result.getCall() = call and
60-
kind = TNormalReturnKind()
115+
result.getReturnKind() = kind
61116
}
62117

63118
/**

‎cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -525,7 +525,7 @@ class ReturnValueInstruction extends ReturnInstruction {
525525
final Instruction getReturnValue() { result = getReturnValueOperand().getDef() }
526526
}
527527

528-
class ReturnIndirectionInstruction extends Instruction {
528+
class ReturnIndirectionInstruction extends VariableInstruction {
529529
ReturnIndirectionInstruction() { getOpcode() instanceof Opcode::ReturnIndirection }
530530

531531
final SideEffectOperand getSideEffectOperand() { result = getAnOperand() }
@@ -535,6 +535,12 @@ class ReturnIndirectionInstruction extends Instruction {
535535
final AddressOperand getSourceAddressOperand() { result = getAnOperand() }
536536

537537
final Instruction getSourceAddress() { result = getSourceAddressOperand().getDef() }
538+
539+
/**
540+
* Gets the parameter for which this instruction reads the final pointed-to value within the
541+
* function.
542+
*/
543+
final Language::Parameter getParameter() { result = var.(IRUserVariable).getVariable() }
538544
}
539545

540546
class CopyInstruction extends Instruction {

‎cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -525,7 +525,7 @@ class ReturnValueInstruction extends ReturnInstruction {
525525
final Instruction getReturnValue() { result = getReturnValueOperand().getDef() }
526526
}
527527

528-
class ReturnIndirectionInstruction extends Instruction {
528+
class ReturnIndirectionInstruction extends VariableInstruction {
529529
ReturnIndirectionInstruction() { getOpcode() instanceof Opcode::ReturnIndirection }
530530

531531
final SideEffectOperand getSideEffectOperand() { result = getAnOperand() }
@@ -535,6 +535,12 @@ class ReturnIndirectionInstruction extends Instruction {
535535
final AddressOperand getSourceAddressOperand() { result = getAnOperand() }
536536

537537
final Instruction getSourceAddress() { result = getSourceAddressOperand().getDef() }
538+
539+
/**
540+
* Gets the parameter for which this instruction reads the final pointed-to value within the
541+
* function.
542+
*/
543+
final Language::Parameter getParameter() { result = var.(IRUserVariable).getVariable() }
538544
}
539545

540546
class CopyInstruction extends Instruction {

‎cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedFunction.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -744,4 +744,9 @@ class TranslatedReadEffect extends TranslatedElement, TTranslatedReadEffect {
744744
operandTag = sideEffectOperand() and
745745
result = getUnknownType()
746746
}
747+
748+
final override IRVariable getInstructionVariable(InstructionTag tag) {
749+
tag = OnlyInstructionTag() and
750+
result = getIRUserVariable(getFunction(), param)
751+
}
747752
}

‎cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -525,7 +525,7 @@ class ReturnValueInstruction extends ReturnInstruction {
525525
final Instruction getReturnValue() { result = getReturnValueOperand().getDef() }
526526
}
527527

528-
class ReturnIndirectionInstruction extends Instruction {
528+
class ReturnIndirectionInstruction extends VariableInstruction {
529529
ReturnIndirectionInstruction() { getOpcode() instanceof Opcode::ReturnIndirection }
530530

531531
final SideEffectOperand getSideEffectOperand() { result = getAnOperand() }
@@ -535,6 +535,12 @@ class ReturnIndirectionInstruction extends Instruction {
535535
final AddressOperand getSourceAddressOperand() { result = getAnOperand() }
536536

537537
final Instruction getSourceAddress() { result = getSourceAddressOperand().getDef() }
538+
539+
/**
540+
* Gets the parameter for which this instruction reads the final pointed-to value within the
541+
* function.
542+
*/
543+
final Language::Parameter getParameter() { result = var.(IRUserVariable).getVariable() }
538544
}
539545

540546
class CopyInstruction extends Instruction {

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,4 +86,14 @@ namespace std {
8686

8787
void test_std_move() {
8888
sink(std::move(getenv("VAR")));
89+
}
90+
91+
void flow_to_outparam(char ** ret, char *arg) {
92+
*ret = arg;
93+
}
94+
95+
void test_outparams() {
96+
char *p2 = nullptr;
97+
flow_to_outparam(&p2, getenv("VAR"));
98+
sink(p2); // tainted
8999
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,14 @@
101101
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:23 | call to getenv |
102102
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:30 | (reference to) |
103103
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
104+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
105+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:91:42:91:44 | arg |
106+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:12:92:14 | arg |
107+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:96:11:96:12 | p2 |
108+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:97:27:97:32 | call to getenv |
109+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | (const char *)... |
110+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | p2 |
111+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
104112
| globals.cpp:5:20:5:25 | call to getenv | globals.cpp:2:17:2:25 | sinkParam |
105113
| globals.cpp:5:20:5:25 | call to getenv | globals.cpp:5:12:5:16 | local |
106114
| globals.cpp:5:20:5:25 | call to getenv | globals.cpp:5:20:5:25 | call to getenv |

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,14 @@
1515
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:8:88:32 | (reference dereference) | IR only |
1616
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:88:18:88:30 | (reference to) | IR only |
1717
| defaulttainttracking.cpp:88:18:88:23 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | IR only |
18+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | IR only |
19+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:91:31:91:33 | ret | AST only |
20+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:5:92:8 | * ... | AST only |
21+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:6:92:8 | ret | AST only |
22+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:96:11:96:12 | p2 | IR only |
23+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | (const char *)... | IR only |
24+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | p2 | IR only |
25+
| defaulttainttracking.cpp:97:27:97:32 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | IR only |
1826
| globals.cpp:13:15:13:20 | call to getenv | globals.cpp:13:5:13:11 | global1 | AST only |
1927
| globals.cpp:23:15:23:20 | call to getenv | globals.cpp:23:5:23:11 | global2 | AST only |
2028
| test_diff.cpp:104:12:104:15 | argv | test_diff.cpp:104:11:104:20 | (...) | IR only |

0 commit comments

Comments
 (0)