C++: Add a new query 'cpp/type-confusion' for detecting type confusion vulnerabilities.

2026-04-22 15:25:18 +02:00 · 2024-03-05 16:23:58 -08:00
parent dcc6f83d3b
commit 8ae6fa5366
8 changed files with 515 additions and 0 deletions
--- a/cpp/ql/src/Security/CWE/CWE-843/TypeConfusion.qhelp
+++ b/cpp/ql/src/Security/CWE/CWE-843/TypeConfusion.qhelp
@@ -0,0 +1,47 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Certain casts in C and C++ places no restrictions on the target type. For
+example, C style casts such as <code>(MyClass*)p</code> allows the programmer
+to cast any pointer <code>p</code> to an expression of type <code>MyClass*</code>.
+If the runtime type of <code>p</code> turns out to be a type that's incompatible
+with <code>MyClass</code>, this results in undefined behavior.
+</p>
+</overview>
+
+<recommendation>
+<p>
+If possible, use <code>dynamic_cast</code> to safely cast between polymorphic types.
+If <code>dynamic_cast</code> is not an option, use <code>static_cast</code> to restrict
+the kinds of conversions that the compiler is allowed to perform. If C++ style casts is
+not an option, carefully check that all casts are safe.
+</p>
+</recommendation>
+
+<example>
+<p>
+Consider the following class hierachy where we define a base class <code>Shape</code> and two
+derived classes <code>Circle</code> and <code>Square</code> that are mutually incompatible:
+</p>
+<sample src="TypeConfusionCommon.cpp"/>
+
+<p>
+The following code demonstrates a type confusion vulnerability where the programmer
+assumes that the runtime type of <code>p</code> is always a <code>Square</code>.
+However, if <code>p</code> is a <code>Circle</code>, the cast will result in undefined behavior.
+</p>
+<sample src="TypeConfusionBad.cpp"/>
+
+<p>
+The following code fixes the vulnerability by using <code>dynamic_cast</code> to
+safely cast between polymorphic types. If the cast fails, <code>dynamic_cast</code>
+returns a null pointer, which can be checked for and handled appropriately.
+</p>
+<sample src="TypeConfusionGood.cpp"/>
+</example>
+
+<references>
+</references>
+</qhelp>
--- a/cpp/ql/src/Security/CWE/CWE-843/TypeConfusion.ql
+++ b/cpp/ql/src/Security/CWE/CWE-843/TypeConfusion.ql
@@ -0,0 +1,251 @@
+/**
+ * @name Type confusion
+ * @description Casting a value to an incompatible type can lead to undefined behavior.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 9.3
+ * @precision medium
+ * @id cpp/type-confusion
+ * @tags security
+ *       external/cwe/cwe-843
+ */
+
+import cpp
+import semmle.code.cpp.dataflow.new.DataFlow
+import BadFlow::PathGraph
+
+/**
+ * Holds if `f` is a field located at byte offset `offset` in `c`.
+ *
+ * Note that predicate is recursive, so that given the following:
+ * ```cpp
+ * struct S1 {
+ *   int a;
+ *   void* b;
+ * };
+ *
+ * struct S2 {
+ *   S1 s1;
+ *   char c;
+ * };
+ * ```
+ * both `hasAFieldWithOffset(S2, s1, 0)` and `hasAFieldWithOffset(S2, a, 0)`
+ * holds.
+ */
+predicate hasAFieldWithOffset(Class c, Field f, int offset) {
+  // Base case: `f` is a field in `c`.
+  f = c.getAField() and
+  offset = f.getByteOffset() and
+  not f.getUnspecifiedType().(Class).hasDefinition()
+  or
+  // Otherwise, we find the struct that is a field of `c` which then has
+  // the field `f` as a member.
+  exists(Field g |
+    g = c.getAField() and
+    // Find the field with the largest offset that's less than or equal to
+    // offset. That's the struct we need to search recursively.
+    g =
+      max(Field cand, int candOffset |
+        cand = c.getAField() and
+        candOffset = cand.getByteOffset() and
+        offset >= candOffset
+      |
+        cand order by candOffset
+      ) and
+    hasAFieldWithOffset(g.getUnspecifiedType(), f, offset - g.getByteOffset())
+  )
+}
+
+/** Holds if `f` is the last field of its declaring class. */
+predicate lastField(Field f) {
+  exists(Class c | c = f.getDeclaringType() |
+    f =
+      max(Field cand, int byteOffset |
+        cand.getDeclaringType() = c and byteOffset = f.getByteOffset()
+      |
+        cand order by byteOffset
+      )
+  )
+}
+
+/**
+ * Holds if there exists a field in `c2` at offset `offset` that's compatible
+ * with `f1`.
+ */
+bindingset[f1, offset, c2]
+pragma[inline_late]
+predicate hasCompatibleFieldAtOffset(Field f1, int offset, Class c2) {
+  exists(Field f2 | hasAFieldWithOffset(c2, f2, offset) |
+    // Let's not deal with bit-fields for now.
+    f2 instanceof BitField
+    or
+    f1.getUnspecifiedType().getSize() = f2.getUnspecifiedType().getSize()
+    or
+    lastField(f1) and
+    f1.getUnspecifiedType().getSize() <= f2.getUnspecifiedType().getSize()
+  )
+}
+
+/**
+ * Holds if `c1` is a prefix of `c2`.
+ */
+bindingset[c1, c2]
+pragma[inline_late]
+predicate prefix(Class c1, Class c2) {
+  not c1.isPolymorphic() and
+  not c2.isPolymorphic() and
+  if c1 instanceof Union
+  then
+    // If it's a union we just verify that one of it's variants is compatible with the other class
+    exists(Field f1, int offset |
+      // Let's not deal with bit-fields for now.
+      not f1 instanceof BitField and
+      hasAFieldWithOffset(c1, f1, offset)
+    |
+      hasCompatibleFieldAtOffset(f1, offset, c2)
+    )
+  else
+    forall(Field f1, int offset |
+      // Let's not deal with bit-fields for now.
+      not f1 instanceof BitField and
+      hasAFieldWithOffset(c1, f1, offset)
+    |
+      hasCompatibleFieldAtOffset(f1, offset, c2)
+    )
+}
+
+/**
+ * An unsafe cast is any explicit cast that is not
+ * a `dynamic_cast`.
+ */
+class UnsafeCast extends Cast {
+  private Class toType;
+
+  UnsafeCast() {
+    (
+      this instanceof CStyleCast
+      or
+      this instanceof StaticCast
+      or
+      this instanceof ReinterpretCast
+    ) and
+    toType = this.getExplicitlyConverted().getUnspecifiedType().stripType() and
+    not this.isImplicit() and
+    exists(TypeDeclarationEntry tde |
+      tde = toType.getDefinition() and
+      not tde.isFromUninstantiatedTemplate(_)
+    )
+  }
+
+  Class getConvertedType() { result = toType }
+
+  bindingset[this, t]
+  pragma[inline_late]
+  predicate compatibleWith(Type t) {
+    t.stripType() = this.getConvertedType()
+    or
+    prefix(this.getConvertedType(), t.stripType())
+    or
+    t.stripType().(Class).getABaseClass+() = this.getConvertedType()
+    or
+    t.stripType() = this.getConvertedType().getABaseClass+()
+  }
+}
+
+/**
+ * Holds if `source` is an allocation that allocates a value of type `state`.
+ */
+predicate isSourceImpl(DataFlow::Node source, Class state) {
+  state = source.asExpr().(AllocationExpr).getAllocatedElementType().stripType() and
+  exists(TypeDeclarationEntry tde |
+    tde = state.getDefinition() and
+    not tde.isFromUninstantiatedTemplate(_)
+  )
+}
+
+module RelevantStateConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { isSourceImpl(source, _) }
+
+  predicate isBarrier(DataFlow::Node node) {
+    // We disable flow through global variables to reduce FPs from infeasible paths
+    node instanceof DataFlow::VariableNode
+    or
+    exists(Class c | c = node.getType().stripType() |
+      not c.hasDefinition()
+      or
+      exists(TypeDeclarationEntry tde |
+        tde = c.getDefinition() and
+        tde.isFromUninstantiatedTemplate(_)
+      )
+    )
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(UnsafeCast cast | sink.asExpr() = cast.getUnconverted())
+  }
+}
+
+module RelevantStateFlow = DataFlow::Global<RelevantStateConfig>;
+
+predicate relevantState(DataFlow::Node sink, Class state) {
+  exists(DataFlow::Node source |
+    RelevantStateFlow::flow(source, sink) and
+    isSourceImpl(source, state)
+  )
+}
+
+predicate isSinkImpl(DataFlow::Node sink, Class state, Type convertedType, boolean compatible) {
+  exists(UnsafeCast cast |
+    relevantState(sink, state) and
+    sink.asExpr() = cast.getUnconverted() and
+    convertedType = cast.getConvertedType()
+  |
+    if cast.compatibleWith(state) then compatible = true else compatible = false
+  )
+}
+
+module BadConfig implements DataFlow::StateConfigSig {
+  class FlowState extends Class {
+    FlowState() { isSourceImpl(_, this) }
+  }
+
+  predicate isSource(DataFlow::Node source, FlowState state) { isSourceImpl(source, state) }
+
+  predicate isBarrier(DataFlow::Node node) { RelevantStateConfig::isBarrier(node) }
+
+  predicate isSink(DataFlow::Node sink, FlowState state) { isSinkImpl(sink, state, _, false) }
+
+  predicate isBarrierOut(DataFlow::Node sink, FlowState state) { isSink(sink, state) }
+}
+
+module BadFlow = DataFlow::GlobalWithState<BadConfig>;
+
+module GoodConfig implements DataFlow::StateConfigSig {
+  class FlowState = BadConfig::FlowState;
+
+  predicate isSource(DataFlow::Node source, FlowState state) { BadConfig::isSource(source, state) }
+
+  predicate isBarrier(DataFlow::Node node) { BadConfig::isBarrier(node) }
+
+  predicate isSink(DataFlow::Node sink, FlowState state) {
+    isSinkImpl(sink, state, _, true) and
+    BadFlow::flowTo(sink)
+  }
+}
+
+module GoodFlow = DataFlow::GlobalWithState<GoodConfig>;
+
+from
+  BadFlow::PathNode source, BadFlow::PathNode sink, Type sourceType, Type sinkType,
+  DataFlow::Node sinkNode
+where
+  BadFlow::flowPath(source, sink) and
+  sinkNode = sink.getNode() and
+  // If there is any flow that would result in a valid cast then we don't
+  // report an alert here. This reduces the number of FPs from infeasible paths
+  // significantly.
+  not GoodFlow::flowTo(sinkNode) and
+  isSourceImpl(source.getNode(), sourceType) and
+  isSinkImpl(sinkNode, _, sinkType, false)
+select sinkNode, source, sink, "Conversion from $@ to $@ is invalid.", sourceType,
+  sourceType.toString(), sinkType, sinkType.toString()
--- a/cpp/ql/src/Security/CWE/CWE-843/TypeConfusionBad.cpp
+++ b/cpp/ql/src/Security/CWE/CWE-843/TypeConfusionBad.cpp
@@ -0,0 +1,7 @@
+void allocate_and_draw_bad() {
+  Shape* shape = new Circle;
+  // ...
+  // BAD: Assumes that shape is always a Square
+  Square* square = static_cast<Square*>(shape);
+  int length = square->getLength();
+}
--- a/cpp/ql/src/Security/CWE/CWE-843/TypeConfusionCommon.cpp
+++ b/cpp/ql/src/Security/CWE/CWE-843/TypeConfusionCommon.cpp
@@ -0,0 +1,25 @@
+struct Shape {
+  virtual ~Shape();
+
+  virtual void draw() = 0;
+};
+
+struct Circle : public Shape {
+  Circle();
+
+  void draw() override {
+    /* ... */
+  }
+
+  int getRadius();
+};
+
+struct Square : public Shape {
+  Square();
+
+  void draw() override {
+    /* ... */
+  }
+
+  int getLength();
+};
--- a/cpp/ql/src/Security/CWE/CWE-843/TypeConfusionGood.cpp
+++ b/cpp/ql/src/Security/CWE/CWE-843/TypeConfusionGood.cpp
@@ -0,0 +1,11 @@
+void allocate_and_draw_good() {
+  Shape* shape = new Circle;
+  // ...
+  // GOOD: Dynamically checks if shape is a Square
+  Square* square = dynamic_cast<Square*>(shape);
+  if(square) {
+    int length = square->getLength();
+  } else {
+    // handle error
+  }
+}
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-843/TypeConfusion.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-843/TypeConfusion.expected
@@ -0,0 +1,27 @@
+edges
+| test.cpp:27:13:27:18 | new | test.cpp:28:25:28:55 | p | provenance |  |
+| test.cpp:32:13:32:30 | new | test.cpp:33:12:33:30 | p | provenance |  |
+| test.cpp:66:15:66:21 | new | test.cpp:67:12:67:31 | a | provenance |  |
+| test.cpp:85:9:85:15 | new | test.cpp:88:14:88:33 | a | provenance |  |
+| test.cpp:127:12:127:17 | new | test.cpp:128:24:128:59 | s2 | provenance |  |
+| test.cpp:143:14:143:19 | new | test.cpp:145:28:145:68 | s1_2 | provenance |  |
+nodes
+| test.cpp:27:13:27:18 | new | semmle.label | new |
+| test.cpp:28:25:28:55 | p | semmle.label | p |
+| test.cpp:32:13:32:30 | new | semmle.label | new |
+| test.cpp:33:12:33:30 | p | semmle.label | p |
+| test.cpp:66:15:66:21 | new | semmle.label | new |
+| test.cpp:67:12:67:31 | a | semmle.label | a |
+| test.cpp:85:9:85:15 | new | semmle.label | new |
+| test.cpp:88:14:88:33 | a | semmle.label | a |
+| test.cpp:127:12:127:17 | new | semmle.label | new |
+| test.cpp:128:24:128:59 | s2 | semmle.label | s2 |
+| test.cpp:143:14:143:19 | new | semmle.label | new |
+| test.cpp:145:28:145:68 | s1_2 | semmle.label | s1_2 |
+subpaths
+#select
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-843/TypeConfusion.qlref
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-843/TypeConfusion.qlref
@@ -0,0 +1 @@
+Security/CWE/CWE-843/TypeConfusion.ql
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-843/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-843/test.cpp
@@ -0,0 +1,146 @@
+struct S1 {
+  int a;
+  void* b;
+  unsigned char c;
+};
+
+struct S1_wrapper {
+  S1 s1;
+};
+
+struct Not_S1_wrapper {
+  unsigned char x;
+  S1 s1;
+};
+
+void test1() {
+  void* p = new S1;
+  S1_wrapper* s1w = static_cast<S1_wrapper*>(p); // GOOD
+}
+
+void test2() {
+  void* p = new S1_wrapper;
+  S1* s1 = static_cast<S1*>(p); // GOOD
+}
+
+void test3() {
+  void* p = new S1;
+  Not_S1_wrapper* s1w = static_cast<Not_S1_wrapper*>(p); // BAD
+}
+
+void test4() {
+  void* p = new Not_S1_wrapper;
+  S1* s1 = static_cast<S1*>(p); // BAD
+}
+
+struct HasBitFields {
+  int x : 16;
+  int y : 16;
+  int z : 32;
+};
+
+struct BufferStruct {
+  unsigned char buffer[sizeof(HasBitFields)];
+};
+
+void test5() {
+  HasBitFields* p = new HasBitFields;
+  BufferStruct* bs = reinterpret_cast<BufferStruct*>(p); // GOOD
+}
+
+struct Animal {
+  virtual ~Animal();
+};
+
+struct Cat : public Animal {
+  Cat();
+  ~Cat();
+};
+
+struct Dog : public Animal {
+  Dog();
+  ~Dog();
+};
+
+void test6() {
+  Animal* a = new Cat;
+  Dog* d = static_cast<Dog*>(a); // BAD
+}
+
+void test7() {
+  Animal* a = new Cat;
+  Dog* d = dynamic_cast<Dog*>(a); // GOOD
+}
+
+void test8() {
+  Animal* a = new Cat;
+  Cat* d = static_cast<Cat*>(a); // GOOD
+}
+
+void test9(bool b) {
+  Animal* a;
+  if(b) {
+    a = new Cat;
+  } else {
+    a = new Dog;
+  }
+  if(b) {
+    Cat* d = static_cast<Cat*>(a); // GOOD
+  }
+}
+
+/**
+ * The layout of S2 is:
+ *  0: int
+ *  8: void*
+ *  16: unsigned char
+ *  16 + pad: unsigned char
+ *  32 + pad: int
+ *  40 + pad: void*
+ *  48 + pad: unsigned char
+*/
+struct S2 {
+  S1 s1;
+  unsigned char buffer[16];
+  S1 s1_2;
+};
+
+struct S2_prefix {
+  int a;
+  void* p;
+  unsigned char c;
+};
+
+void test10() {
+  S2* s2 = new S2;
+  S2_prefix* s2p = reinterpret_cast<S2_prefix*>(s2); // GOOD
+}
+
+struct Not_S2_prefix {
+  int a;
+  void* p;
+  void* p2;
+  unsigned char c;
+};
+
+void test11() {
+  S2* s2 = new S2;
+  Not_S2_prefix* s2p = reinterpret_cast<Not_S2_prefix*>(s2); // BAD
+}
+
+struct HasSomeBitFields {
+  int x : 16;
+  int y;
+  int z : 32;
+};
+
+void test12() {
+  // This has doesn't have any non-bitfield member, so we don't detect
+  // the problem here since the query currently ignores bitfields.
+  S1* s1 = new S1;
+  HasBitFields* hbf = reinterpret_cast<HasBitFields*>(s1); // BAD [NOT DETECTED]
+
+  S1* s1_2 = new S1;
+  // This one has a non-bitfield members. So we detect the problem
+  HasSomeBitFields* hbf2 = reinterpret_cast<HasSomeBitFields*>(s1_2); // BAD
+}