Skip to content

Commit 8ae6fa5

Browse files
MathiasVPMathiasVP
committed
C++: Add a new query 'cpp/type-confusion' for detecting type confusion vulnerabilities.
1 parent dcc6f83 commit 8ae6fa5

File tree

8 files changed

+515
-0
lines changed

8 files changed

+515
-0
lines changed

‎cpp/ql/src/Security/CWE/CWE-843/TypeConfusion.qhelp

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>
6+
Certain casts in C and C++ places no restrictions on the target type. For
7+
example, C style casts such as <code>(MyClass*)p</code> allows the programmer
8+
to cast any pointer <code>p</code> to an expression of type <code>MyClass*</code>.
9+
If the runtime type of <code>p</code> turns out to be a type that's incompatible
10+
with <code>MyClass</code>, this results in undefined behavior.
11+
</p>
12+
</overview>
13+
14+
<recommendation>
15+
<p>
16+
If possible, use <code>dynamic_cast</code> to safely cast between polymorphic types.
17+
If <code>dynamic_cast</code> is not an option, use <code>static_cast</code> to restrict
18+
the kinds of conversions that the compiler is allowed to perform. If C++ style casts is
19+
not an option, carefully check that all casts are safe.
20+
</p>
21+
</recommendation>
22+
23+
<example>
24+
<p>
25+
Consider the following class hierachy where we define a base class <code>Shape</code> and two
26+
derived classes <code>Circle</code> and <code>Square</code> that are mutually incompatible:
27+
</p>
28+
<sample src="TypeConfusionCommon.cpp"/>
29+
30+
<p>
31+
The following code demonstrates a type confusion vulnerability where the programmer
32+
assumes that the runtime type of <code>p</code> is always a <code>Square</code>.
33+
However, if <code>p</code> is a <code>Circle</code>, the cast will result in undefined behavior.
34+
</p>
35+
<sample src="TypeConfusionBad.cpp"/>
36+
37+
<p>
38+
The following code fixes the vulnerability by using <code>dynamic_cast</code> to
39+
safely cast between polymorphic types. If the cast fails, <code>dynamic_cast</code>
40+
returns a null pointer, which can be checked for and handled appropriately.
41+
</p>
42+
<sample src="TypeConfusionGood.cpp"/>
43+
</example>
44+
45+
<references>
46+
</references>
47+
</qhelp>

‎cpp/ql/src/Security/CWE/CWE-843/TypeConfusion.ql

Lines changed: 251 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,251 @@
1+
/**
2+
* @name Type confusion
3+
* @description Casting a value to an incompatible type can lead to undefined behavior.
4+
* @kind path-problem
5+
* @problem.severity warning
6+
* @security-severity 9.3
7+
* @precision medium
8+
* @id cpp/type-confusion
9+
* @tags security
10+
* external/cwe/cwe-843
11+
*/
12+
13+
import cpp
14+
import semmle.code.cpp.dataflow.new.DataFlow
15+
import BadFlow::PathGraph
16+
17+
/**
18+
* Holds if `f` is a field located at byte offset `offset` in `c`.
19+
*
20+
* Note that predicate is recursive, so that given the following:
21+
* ```cpp
22+
* struct S1 {
23+
* int a;
24+
* void* b;
25+
* };
26+
*
27+
* struct S2 {
28+
* S1 s1;
29+
* char c;
30+
* };
31+
* ```
32+
* both `hasAFieldWithOffset(S2, s1, 0)` and `hasAFieldWithOffset(S2, a, 0)`
33+
* holds.
34+
*/
35+
predicate hasAFieldWithOffset(Class c, Field f, int offset) {
36+
// Base case: `f` is a field in `c`.
37+
f = c.getAField() and
38+
offset = f.getByteOffset() and
39+
not f.getUnspecifiedType().(Class).hasDefinition()
40+
or
41+
// Otherwise, we find the struct that is a field of `c` which then has
42+
// the field `f` as a member.
43+
exists(Field g |
44+
g = c.getAField() and
45+
// Find the field with the largest offset that's less than or equal to
46+
// offset. That's the struct we need to search recursively.
47+
g =
48+
max(Field cand, int candOffset |
49+
cand = c.getAField() and
50+
candOffset = cand.getByteOffset() and
51+
offset >= candOffset
52+
|
53+
cand order by candOffset
54+
) and
55+
hasAFieldWithOffset(g.getUnspecifiedType(), f, offset - g.getByteOffset())
56+
)
57+
}
58+
59+
/** Holds if `f` is the last field of its declaring class. */
60+
predicate lastField(Field f) {
61+
exists(Class c | c = f.getDeclaringType() |
62+
f =
63+
max(Field cand, int byteOffset |
64+
cand.getDeclaringType() = c and byteOffset = f.getByteOffset()
65+
|
66+
cand order by byteOffset
67+
)
68+
)
69+
}
70+
71+
/**
72+
* Holds if there exists a field in `c2` at offset `offset` that's compatible
73+
* with `f1`.
74+
*/
75+
bindingset[f1, offset, c2]
76+
pragma[inline_late]
77+
predicate hasCompatibleFieldAtOffset(Field f1, int offset, Class c2) {
78+
exists(Field f2 | hasAFieldWithOffset(c2, f2, offset) |
79+
// Let's not deal with bit-fields for now.
80+
f2 instanceof BitField
81+
or
82+
f1.getUnspecifiedType().getSize() = f2.getUnspecifiedType().getSize()
83+
or
84+
lastField(f1) and
85+
f1.getUnspecifiedType().getSize() <= f2.getUnspecifiedType().getSize()
86+
)
87+
}
88+
89+
/**
90+
* Holds if `c1` is a prefix of `c2`.
91+
*/
92+
bindingset[c1, c2]
93+
pragma[inline_late]
94+
predicate prefix(Class c1, Class c2) {
95+
not c1.isPolymorphic() and
96+
not c2.isPolymorphic() and
97+
if c1 instanceof Union
98+
then
99+
// If it's a union we just verify that one of it's variants is compatible with the other class
100+
exists(Field f1, int offset |
101+
// Let's not deal with bit-fields for now.
102+
not f1 instanceof BitField and
103+
hasAFieldWithOffset(c1, f1, offset)
104+
|
105+
hasCompatibleFieldAtOffset(f1, offset, c2)
106+
)
107+
else
108+
forall(Field f1, int offset |
109+
// Let's not deal with bit-fields for now.
110+
not f1 instanceof BitField and
111+
hasAFieldWithOffset(c1, f1, offset)
112+
|
113+
hasCompatibleFieldAtOffset(f1, offset, c2)
114+
)
115+
}
116+
117+
/**
118+
* An unsafe cast is any explicit cast that is not
119+
* a `dynamic_cast`.
120+
*/
121+
class UnsafeCast extends Cast {
122+
private Class toType;
123+
124+
UnsafeCast() {
125+
(
126+
this instanceof CStyleCast
127+
or
128+
this instanceof StaticCast
129+
or
130+
this instanceof ReinterpretCast
131+
) and
132+
toType = this.getExplicitlyConverted().getUnspecifiedType().stripType() and
133+
not this.isImplicit() and
134+
exists(TypeDeclarationEntry tde |
135+
tde = toType.getDefinition() and
136+
not tde.isFromUninstantiatedTemplate(_)
137+
)
138+
}
139+
140+
Class getConvertedType() { result = toType }
141+
142+
bindingset[this, t]
143+
pragma[inline_late]
144+
predicate compatibleWith(Type t) {
145+
t.stripType() = this.getConvertedType()
146+
or
147+
prefix(this.getConvertedType(), t.stripType())
148+
or
149+
t.stripType().(Class).getABaseClass+() = this.getConvertedType()
150+
or
151+
t.stripType() = this.getConvertedType().getABaseClass+()
152+
}
153+
}
154+
155+
/**
156+
* Holds if `source` is an allocation that allocates a value of type `state`.
157+
*/
158+
predicate isSourceImpl(DataFlow::Node source, Class state) {
159+
state = source.asExpr().(AllocationExpr).getAllocatedElementType().stripType() and
160+
exists(TypeDeclarationEntry tde |
161+
tde = state.getDefinition() and
162+
not tde.isFromUninstantiatedTemplate(_)
163+
)
164+
}
165+
166+
module RelevantStateConfig implements DataFlow::ConfigSig {
167+
predicate isSource(DataFlow::Node source) { isSourceImpl(source, _) }
168+
169+
predicate isBarrier(DataFlow::Node node) {
170+
// We disable flow through global variables to reduce FPs from infeasible paths
171+
node instanceof DataFlow::VariableNode
172+
or
173+
exists(Class c | c = node.getType().stripType() |
174+
not c.hasDefinition()
175+
or
176+
exists(TypeDeclarationEntry tde |
177+
tde = c.getDefinition() and
178+
tde.isFromUninstantiatedTemplate(_)
179+
)
180+
)
181+
}
182+
183+
predicate isSink(DataFlow::Node sink) {
184+
exists(UnsafeCast cast | sink.asExpr() = cast.getUnconverted())
185+
}
186+
}
187+
188+
module RelevantStateFlow = DataFlow::Global<RelevantStateConfig>;
189+
190+
predicate relevantState(DataFlow::Node sink, Class state) {
191+
exists(DataFlow::Node source |
192+
RelevantStateFlow::flow(source, sink) and
193+
isSourceImpl(source, state)
194+
)
195+
}
196+
197+
predicate isSinkImpl(DataFlow::Node sink, Class state, Type convertedType, boolean compatible) {
198+
exists(UnsafeCast cast |
199+
relevantState(sink, state) and
200+
sink.asExpr() = cast.getUnconverted() and
201+
convertedType = cast.getConvertedType()
202+
|
203+
if cast.compatibleWith(state) then compatible = true else compatible = false
204+
)
205+
}
206+
207+
module BadConfig implements DataFlow::StateConfigSig {
208+
class FlowState extends Class {
209+
FlowState() { isSourceImpl(_, this) }
210+
}
211+
212+
predicate isSource(DataFlow::Node source, FlowState state) { isSourceImpl(source, state) }
213+
214+
predicate isBarrier(DataFlow::Node node) { RelevantStateConfig::isBarrier(node) }
215+
216+
predicate isSink(DataFlow::Node sink, FlowState state) { isSinkImpl(sink, state, _, false) }
217+
218+
predicate isBarrierOut(DataFlow::Node sink, FlowState state) { isSink(sink, state) }
219+
}
220+
221+
module BadFlow = DataFlow::GlobalWithState<BadConfig>;
222+
223+
module GoodConfig implements DataFlow::StateConfigSig {
224+
class FlowState = BadConfig::FlowState;
225+
226+
predicate isSource(DataFlow::Node source, FlowState state) { BadConfig::isSource(source, state) }
227+
228+
predicate isBarrier(DataFlow::Node node) { BadConfig::isBarrier(node) }
229+
230+
predicate isSink(DataFlow::Node sink, FlowState state) {
231+
isSinkImpl(sink, state, _, true) and
232+
BadFlow::flowTo(sink)
233+
}
234+
}
235+
236+
module GoodFlow = DataFlow::GlobalWithState<GoodConfig>;
237+
238+
from
239+
BadFlow::PathNode source, BadFlow::PathNode sink, Type sourceType, Type sinkType,
240+
DataFlow::Node sinkNode
241+
where
242+
BadFlow::flowPath(source, sink) and
243+
sinkNode = sink.getNode() and
244+
// If there is any flow that would result in a valid cast then we don't
245+
// report an alert here. This reduces the number of FPs from infeasible paths
246+
// significantly.
247+
not GoodFlow::flowTo(sinkNode) and
248+
isSourceImpl(source.getNode(), sourceType) and
249+
isSinkImpl(sinkNode, _, sinkType, false)
250+
select sinkNode, source, sink, "Conversion from $@ to $@ is invalid.", sourceType,
251+
sourceType.toString(), sinkType, sinkType.toString()

‎cpp/ql/src/Security/CWE/CWE-843/TypeConfusionBad.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
void allocate_and_draw_bad() {
2+
Shape* shape = new Circle;
3+
// ...
4+
// BAD: Assumes that shape is always a Square
5+
Square* square = static_cast<Square*>(shape);
6+
int length = square->getLength();
7+
}

‎cpp/ql/src/Security/CWE/CWE-843/TypeConfusionCommon.cpp

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
struct Shape {
2+
virtual ~Shape();
3+
4+
virtual void draw() = 0;
5+
};
6+
7+
struct Circle : public Shape {
8+
Circle();
9+
10+
void draw() override {
11+
/* ... */
12+
}
13+
14+
int getRadius();
15+
};
16+
17+
struct Square : public Shape {
18+
Square();
19+
20+
void draw() override {
21+
/* ... */
22+
}
23+
24+
int getLength();
25+
};

‎cpp/ql/src/Security/CWE/CWE-843/TypeConfusionGood.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
void allocate_and_draw_good() {
2+
Shape* shape = new Circle;
3+
// ...
4+
// GOOD: Dynamically checks if shape is a Square
5+
Square* square = dynamic_cast<Square*>(shape);
6+
if(square) {
7+
int length = square->getLength();
8+
} else {
9+
// handle error
10+
}
11+
}

‎cpp/ql/test/query-tests/Security/CWE/CWE-843/TypeConfusion.expected

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
edges
2+
| test.cpp:27:13:27:18 | new | test.cpp:28:25:28:55 | p | provenance | |
3+
| test.cpp:32:13:32:30 | new | test.cpp:33:12:33:30 | p | provenance | |
4+
| test.cpp:66:15:66:21 | new | test.cpp:67:12:67:31 | a | provenance | |
5+
| test.cpp:85:9:85:15 | new | test.cpp:88:14:88:33 | a | provenance | |
6+
| test.cpp:127:12:127:17 | new | test.cpp:128:24:128:59 | s2 | provenance | |
7+
| test.cpp:143:14:143:19 | new | test.cpp:145:28:145:68 | s1_2 | provenance | |
8+
nodes
9+
| test.cpp:27:13:27:18 | new | semmle.label | new |
10+
| test.cpp:28:25:28:55 | p | semmle.label | p |
11+
| test.cpp:32:13:32:30 | new | semmle.label | new |
12+
| test.cpp:33:12:33:30 | p | semmle.label | p |
13+
| test.cpp:66:15:66:21 | new | semmle.label | new |
14+
| test.cpp:67:12:67:31 | a | semmle.label | a |
15+
| test.cpp:85:9:85:15 | new | semmle.label | new |
16+
| test.cpp:88:14:88:33 | a | semmle.label | a |
17+
| test.cpp:127:12:127:17 | new | semmle.label | new |
18+
| test.cpp:128:24:128:59 | s2 | semmle.label | s2 |
19+
| test.cpp:143:14:143:19 | new | semmle.label | new |
20+
| test.cpp:145:28:145:68 | s1_2 | semmle.label | s1_2 |
21+
subpaths
22+
#select
23+
| test.cpp:28:25:28:55 | p | test.cpp:27:13:27:18 | new | test.cpp:28:25:28:55 | p | Conversion from $@ to $@ is invalid. | test.cpp:1:8:1:9 | S1 | S1 | test.cpp:11:8:11:21 | Not_S1_wrapper | Not_S1_wrapper |
24+
| test.cpp:33:12:33:30 | p | test.cpp:32:13:32:30 | new | test.cpp:33:12:33:30 | p | Conversion from $@ to $@ is invalid. | test.cpp:11:8:11:21 | Not_S1_wrapper | Not_S1_wrapper | test.cpp:1:8:1:9 | S1 | S1 |
25+
| test.cpp:67:12:67:31 | a | test.cpp:66:15:66:21 | new | test.cpp:67:12:67:31 | a | Conversion from $@ to $@ is invalid. | test.cpp:55:8:55:10 | Cat | Cat | test.cpp:60:8:60:10 | Dog | Dog |
26+
| test.cpp:128:24:128:59 | s2 | test.cpp:127:12:127:17 | new | test.cpp:128:24:128:59 | s2 | Conversion from $@ to $@ is invalid. | test.cpp:102:8:102:9 | S2 | S2 | test.cpp:119:8:119:20 | Not_S2_prefix | Not_S2_prefix |
27+
| test.cpp:145:28:145:68 | s1_2 | test.cpp:143:14:143:19 | new | test.cpp:145:28:145:68 | s1_2 | Conversion from $@ to $@ is invalid. | test.cpp:1:8:1:9 | S1 | S1 | test.cpp:131:8:131:23 | HasSomeBitFields | HasSomeBitFields |

‎cpp/ql/test/query-tests/Security/CWE/CWE-843/TypeConfusion.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE/CWE-843/TypeConfusion.ql

0 commit comments

Comments
 (0)