github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.75k stars 1.56k forks source link

[C++] Dynamic Dispatch Through Member Not Detected #18102

Open JustusAdam opened 4 days ago

JustusAdam commented 4 days ago

Calling a virtual function on a field defeats the dispatch analysis. In the following example only the first tall to target() is detected as receiving sensitive data from source(). The second one is missed and the only difference is that in the first case the dispatched pointer is a local variable, and in the second it is a member.

int source()
{
    return 2;
}

int target(int source)
{
    return source;
}

class Base
{
public:
    virtual int f() = 0;
};

class A : public Base
{
    int f() { return source(); }
};

struct Container
{
    Base *b;
};

int main(int argv, char **argc)
{
    Base *b = new A;
    target(b->f());

    Container c;
    c.b = new A;
    target(c.b->f());
    return 0;
}

This is the query I ran

import cpp
import semmle.code.cpp.dataflow.new.TaintTracking

module SourceSinkCallConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) {
    source.asExpr().(Call).getTarget().getName() = "source"
  }

  predicate isSink(DataFlow::Node sink) {
    exists(Call call |
      call.getTarget().getName() = "target" and
      call.getArgument(0) = sink.asExpr()
    )
  }
}

module SourceSinkCallTaint = TaintTracking::Global<SourceSinkCallConfig>;

from DataFlow::Node source, DataFlow::Node sink, int source_line, int sink_line
where
  SourceSinkCallTaint::flow(source, sink) and
  source_line = source.getLocation().getStartLine() and
  sink_line = sink.getLocation().getStartLine()
select source, source_line, sink, sink_line

This is the output. I would have expected to also see a flow from line 19 to 34.

|     source     | source_line |   sink    | sink_line |
+----------------+-------------+-----------+-----------+
| call to source |          19 | call to f |        30 |

CodeQL version: 2.19.3

redsun82 commented 3 days ago

👋 @JustusAdam thanks for reaching out to us for this!

I think that may indeed be a limitation, but I will circle back to you with a confirmation tomorrow. If it is, I will make sure we have this in our backlog of things to improve!

redsun82 commented 2 days ago

I've created an internal issue to track this.