C++: Add a new query for detecting type confusion vulnerabilities

[MathiasVP]

trafficstars

This PR adds a new medium precision query cpp/type-confusion to detect issues such as:

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
}

[github-actions[bot]]

QHelp previews:

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

Type confusion

Certain casts in C and C++ place no restrictions on the target type. For example, C style casts such as (MyClass*)p allows the programmer to cast any pointer p to an expression of type MyClass*. If the runtime type of p turns out to be a type that's incompatible with MyClass, this results in undefined behavior.

Recommendation

If possible, use dynamic_cast to safely cast between polymorphic types. If dynamic_cast is not an option, use static_cast 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.

Example

Consider the following class hierachy where we define a base class Shape and two derived classes Circle and Square that are mutually incompatible:

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();
};

The following code demonstrates a type confusion vulnerability where the programmer assumes that the runtime type of p is always a Square. However, if p is a Circle, the cast will result in undefined behavior.

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();
}

The following code fixes the vulnerability by using dynamic_cast to safely cast between polymorphic types. If the cast fails, dynamic_cast returns a null pointer, which can be checked for and handled appropriately.

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
  }
}

References

• Microsoft Learn: Type conversions and type safety.
• Common Weakness Enumeration: CWE-843.

[MathiasVP]

I'd also like to look at real-world results, perhaps after an initial round of discussion though.

Thanks! Yeah, I'm still fixing a couple of small issues that's causing some result blow-up. I'll fix that up before pulling this out of draft. So please don't look at the real-world results just yet 😂

I'm a little bit concerns about performance of the field related predicates, but a DCA run will tell us if anything's really wrong.

I've spent quite a lot of time fine-tuning this to perform well. Indeed, it's very easy to accidentially create a cartesian product doing this, but I've managed to remove them all. In fact, for all projects I've looked at this hasn't been any problem at all.

I'll start DCA as soon as I'm done fixing up real-world results.

[MathiasVP]

Hmmm... There's some bad performance on ChakraCore that seems to be more complex than a bad join 😭 I'm investigating!

[MathiasVP]

Performance on ChakraCore was awful even without any field flow. That's certainly worth digging into eventually, but as a quick fix I've removed the two state-dependent configurations from the query. The query is now less principled (which is bad!), but at least it will hopefully run a lot faster (which is good!). Let's see what DCA says 🤞