Skip to content

C++: Taint propagation through dynamic_cast #2713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 28, 2020
Merged

C++: Taint propagation through dynamic_cast #2713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
928b0c5
C++: Add test demonstrating false negative when using dynamic_cast
MathiasVP Jan 28, 2020
46ce228
C++: Add instruction for CheckedConvertOrNull and handle it in alias …
MathiasVP Jan 28, 2020
c1091a0
C++: Accept output
MathiasVP Jan 28, 2020
67d29e3
C#: Sync identical files
MathiasVP Jan 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
iTo.(PhiInstruction).getAnOperand().getDef() = iFrom or
// Treat all conversions as flow, even conversions between different numeric types.
iTo.(ConvertInstruction).getUnary() = iFrom or
iTo.(CheckedConvertOrNullInstruction).getUnary() = iFrom or
iTo.(InheritanceConversionInstruction).getUnary() = iFrom or
// A chi instruction represents a point where a new value (the _partial_
// operand) may overwrite an old value (the _total_ operand), but the alias
Expand Down
4 changes: 4 additions & 0 deletions cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,10 @@ class ConvertInstruction extends UnaryInstruction {
ConvertInstruction() { getOpcode() instanceof Opcode::Convert }
}

class CheckedConvertOrNullInstruction extends UnaryInstruction {
CheckedConvertOrNullInstruction() { getOpcode() instanceof Opcode::CheckedConvertOrNull }
}

/**
* Represents an instruction that converts between two addresses
* related by inheritance.
Expand Down
4 changes: 4 additions & 0 deletions cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ private predicate operandIsPropagated(Operand operand, IntValue bitOffset) {
bitOffset = Ints::mul(convert.getDerivation().getByteOffset(), 8)
)
or
// Conversion using dynamic_cast results in an unknown offset
instr instanceof CheckedConvertOrNullInstruction and
bitOffset = Ints::unknown()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must the offset be unknown in order for the alias analysis to work? I think of dynamic_cast<D*>(b) as meaning roughly (*b instanceof D) ? static_cast<D*>(b) : nullptr, but maybe that's an oversimplification? If the type check doesn't pass, then the returned pointer doesn't point to any object, but that's also sort of true for a static_cast.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "sidecast" case in https://en.cppreference.com/w/cpp/language/dynamic_cast means that the offset is at least sometimes unknown. Even for the downcast case, there isn't an unambiguous offset for a conversion from Y to Base in https://godbolt.org/z/ebWA7x

Copy link
Contributor Author

@MathiasVP MathiasVP Jan 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also the special case of dynamic_cast<void*> which casts to the most specific polymorphic class: https://godbolt.org/z/AZJpxS

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanations. Inheritance is scary!

or
// Converting to a derived class subtracts the offset of the base class.
exists(ConvertToDerivedInstruction convert |
convert = instr and
Expand Down
4 changes: 4 additions & 0 deletions cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,10 @@ class ConvertInstruction extends UnaryInstruction {
ConvertInstruction() { getOpcode() instanceof Opcode::Convert }
}

class CheckedConvertOrNullInstruction extends UnaryInstruction {
CheckedConvertOrNullInstruction() { getOpcode() instanceof Opcode::CheckedConvertOrNull }
}

/**
* Represents an instruction that converts between two addresses
* related by inheritance.
Expand Down
4 changes: 4 additions & 0 deletions cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,10 @@ class ConvertInstruction extends UnaryInstruction {
ConvertInstruction() { getOpcode() instanceof Opcode::Convert }
}

class CheckedConvertOrNullInstruction extends UnaryInstruction {
CheckedConvertOrNullInstruction() { getOpcode() instanceof Opcode::CheckedConvertOrNull }
}

/**
* Represents an instruction that converts between two addresses
* related by inheritance.
Expand Down
4 changes: 4 additions & 0 deletions cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ private predicate operandIsPropagated(Operand operand, IntValue bitOffset) {
bitOffset = Ints::mul(convert.getDerivation().getByteOffset(), 8)
)
or
// Conversion using dynamic_cast results in an unknown offset
instr instanceof CheckedConvertOrNullInstruction and
bitOffset = Ints::unknown()
or
// Converting to a derived class subtracts the offset of the base class.
exists(ConvertToDerivedInstruction convert |
convert = instr and
Expand Down
39 changes: 39 additions & 0 deletions cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,42 @@ void test_indirect_arg_to_model() {
inet_addr_retval a = inet_addr((const char *)&env_pointer);
sink(a);
}

class B {
public:
virtual void f(const char*) = 0;
};

class D1 : public B {};

class D2 : public D1 {
public:
void f(const char* p) override {}
};

class D3 : public D2 {
public:
void f(const char* p) override {
sink(p);
}
};

void test_dynamic_cast() {
B* b = new D3();
b->f(getenv("VAR")); // tainted

((D2*)b)->f(getenv("VAR")); // tainted
static_cast<D2*>(b)->f(getenv("VAR")); // tainted
dynamic_cast<D2*>(b)->f(getenv("VAR")); // tainted
reinterpret_cast<D2*>(b)->f(getenv("VAR")); // tainted

B* b2 = new D2();
b2->f(getenv("VAR"));

((D2*)b2)->f(getenv("VAR"));
static_cast<D2*>(b2)->f(getenv("VAR"));
dynamic_cast<D2*>(b2)->f(getenv("VAR"));
reinterpret_cast<D2*>(b2)->f(getenv("VAR"));

dynamic_cast<D3*>(b2)->f(getenv("VAR")); // tainted [FALSE POSITIVE]
}
62 changes: 62 additions & 0 deletions cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,64 @@
| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:26:39:34 | call to inet_addr |
| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:50:39:61 | & ... |
| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:40:10:40:10 | a |
| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:45:20:45:29 | p#0 |
| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p |
| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p |
| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p |
| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:64:10:64:15 | call to getenv |
| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:64:10:64:22 | (const char *)... |
| defaulttainttracking.cpp:64:10:64:15 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p |
| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p |
| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p |
| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:66:17:66:22 | call to getenv |
| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:66:17:66:29 | (const char *)... |
| defaulttainttracking.cpp:66:17:66:22 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p |
| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p |
| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p |
| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:67:28:67:33 | call to getenv |
| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:67:28:67:40 | (const char *)... |
| defaulttainttracking.cpp:67:28:67:33 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p |
| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p |
| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p |
| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:68:29:68:34 | call to getenv |
| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:68:29:68:41 | (const char *)... |
| defaulttainttracking.cpp:68:29:68:34 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p |
| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p |
| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p |
| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:69:33:69:38 | call to getenv |
| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:69:33:69:45 | (const char *)... |
| defaulttainttracking.cpp:69:33:69:38 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
| defaulttainttracking.cpp:72:11:72:16 | call to getenv | defaulttainttracking.cpp:45:20:45:29 | p#0 |
| defaulttainttracking.cpp:72:11:72:16 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p |
| defaulttainttracking.cpp:72:11:72:16 | call to getenv | defaulttainttracking.cpp:72:11:72:16 | call to getenv |
| defaulttainttracking.cpp:72:11:72:16 | call to getenv | defaulttainttracking.cpp:72:11:72:23 | (const char *)... |
| defaulttainttracking.cpp:74:18:74:23 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p |
| defaulttainttracking.cpp:74:18:74:23 | call to getenv | defaulttainttracking.cpp:74:18:74:23 | call to getenv |
| defaulttainttracking.cpp:74:18:74:23 | call to getenv | defaulttainttracking.cpp:74:18:74:30 | (const char *)... |
| defaulttainttracking.cpp:75:29:75:34 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p |
| defaulttainttracking.cpp:75:29:75:34 | call to getenv | defaulttainttracking.cpp:75:29:75:34 | call to getenv |
| defaulttainttracking.cpp:75:29:75:34 | call to getenv | defaulttainttracking.cpp:75:29:75:41 | (const char *)... |
| defaulttainttracking.cpp:76:30:76:35 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p |
| defaulttainttracking.cpp:76:30:76:35 | call to getenv | defaulttainttracking.cpp:76:30:76:35 | call to getenv |
| defaulttainttracking.cpp:76:30:76:35 | call to getenv | defaulttainttracking.cpp:76:30:76:42 | (const char *)... |
| defaulttainttracking.cpp:77:34:77:39 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p |
| defaulttainttracking.cpp:77:34:77:39 | call to getenv | defaulttainttracking.cpp:77:34:77:39 | call to getenv |
| defaulttainttracking.cpp:77:34:77:39 | call to getenv | defaulttainttracking.cpp:77:34:77:46 | (const char *)... |
| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p |
| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p |
| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:79:30:79:35 | call to getenv |
| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:79:30:79:42 | (const char *)... |
| defaulttainttracking.cpp:79:30:79:35 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
| test_diff.cpp:92:10:92:13 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
| test_diff.cpp:92:10:92:13 | argv | test_diff.cpp:1:11:1:20 | p#0 |
| test_diff.cpp:92:10:92:13 | argv | test_diff.cpp:92:10:92:13 | argv |
Expand Down Expand Up @@ -123,7 +181,11 @@
| test_diff.cpp:126:43:126:46 | argv | test_diff.cpp:126:43:126:46 | argv |
| test_diff.cpp:126:43:126:46 | argv | test_diff.cpp:126:43:126:49 | (const char *)... |
| test_diff.cpp:126:43:126:46 | argv | test_diff.cpp:126:43:126:49 | access to array |
| test_diff.cpp:128:44:128:47 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 |
| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:1:11:1:20 | p#0 |
| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:76:24:76:24 | p |
| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:81:24:81:24 | p |
| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:82:14:82:14 | p |
| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:128:44:128:47 | argv |
| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:128:44:128:50 | (const char *)... |
| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:128:44:128:50 | access to array |
5 changes: 1 addition & 4 deletions cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,11 @@
| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:31:40:31:53 | dotted_address | AST only |
| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:36:39:61 | (const char *)... | AST only |
| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:51:39:61 | env_pointer | AST only |
| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | IR only |
| test_diff.cpp:104:12:104:15 | argv | test_diff.cpp:104:11:104:20 | (...) | IR only |
| test_diff.cpp:108:10:108:13 | argv | test_diff.cpp:36:24:36:24 | p | AST only |
| test_diff.cpp:111:10:111:13 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 | AST only |
| test_diff.cpp:111:10:111:13 | argv | test_diff.cpp:1:11:1:20 | p#0 | AST only |
| test_diff.cpp:111:10:111:13 | argv | test_diff.cpp:29:24:29:24 | p | AST only |
| test_diff.cpp:111:10:111:13 | argv | test_diff.cpp:30:14:30:14 | p | AST only |
| test_diff.cpp:124:19:124:22 | argv | test_diff.cpp:76:24:76:24 | p | IR only |
| test_diff.cpp:128:44:128:47 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 | AST only |
| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:1:11:1:20 | p#0 | AST only |
| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:81:24:81:24 | p | AST only |
| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:82:14:82:14 | p | AST only |
4 changes: 4 additions & 0 deletions csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,10 @@ class ConvertInstruction extends UnaryInstruction {
ConvertInstruction() { getOpcode() instanceof Opcode::Convert }
}

class CheckedConvertOrNullInstruction extends UnaryInstruction {
CheckedConvertOrNullInstruction() { getOpcode() instanceof Opcode::CheckedConvertOrNull }
}

/**
* Represents an instruction that converts between two addresses
* related by inheritance.
Expand Down
4 changes: 4 additions & 0 deletions csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,10 @@ class ConvertInstruction extends UnaryInstruction {
ConvertInstruction() { getOpcode() instanceof Opcode::Convert }
}

class CheckedConvertOrNullInstruction extends UnaryInstruction {
CheckedConvertOrNullInstruction() { getOpcode() instanceof Opcode::CheckedConvertOrNull }
}

/**
* Represents an instruction that converts between two addresses
* related by inheritance.
Expand Down