[C#] Taint analysis does not have flow path for non constant field of a class as its source

[kanan1832]

Hello, I'm fairly new to codeql so if I'm using codeql terms or statements incorrectly, apologies for that.

I have this simple Test class, where I'm trying to mimic a path problem.
with Message1 as source and Console.WriteLine(Message1) as sink.

 class Test
    {
        private const string Message1 = "Test Message 1";
        private readonly string Message2 = "Test Message 2";

        public void Run()
        {
            Console.WriteLine(Message1);
            Console.WriteLine(Message2);
        }
    }

import csharp
import DataFlow::PathGraph

class Source extends DataFlow::Node {
  Source() { this.asExpr() instanceof StringLiteral }
}

class WriteLineMethod extends Method {
  WriteLineMethod() { this.hasQualifiedName("System.Console.WriteLine") }
}

class Sink extends DataFlow::Node {
  Sink() {
    exists(MethodCall m |
      m.getTarget() instanceof WriteLineMethod and
      this.asExpr() = m.getArgument(0)
    )
  }
}

class SimpleConfiguration extends TaintTracking::Configuration {
  SimpleConfiguration() { this = "Simple configuration" }

  override predicate isSource(DataFlow::Node source) { source instanceof Source }

  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
  
}

from DataFlow::PathNode source, DataFlow::PathNode sink, SimpleConfiguration cfg
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "$@ is used in WriteLine method.", source.getNode(),
  "String"

Here I did simple taint tracking analysis on above Test class using above query, for const field private const string Message1 = "Test Message 1"; as source and
Console.WriteLine(Message1); as sink i'm getting correct result, But for non const field private readonly string Message2 = "Test Message 2"; it doesn't seem to work.

Did i miss something here? why does taint tracking works for const field or static field in static class but not for instance field?

I have asked this same question at few different places, below is the link for that.

stackoverflow question
github/codeql/issues/9569
github/securitylab/discussions/692

[tamasvajk]

This is a great question, I had to dig into CFG and DF implementation details for the answer, and I'm not sure I have the full picture yet, but hopefully the below answers your question.

The CodeQL C# library handles instance fields differently than static or const fields. Initializers of instance fields are moved into instance constructors when the CFG is constructed. So the following sample

class C 
{
  int x = 42;

  C() { }
}

is modelled as the below in the CFG

class C
{
  int x;

  C() 
  {
    this.x = 42;
  }
}

I'm not sure about the reasons why it was implemented like this, but I assume this lets us do precise dataflow analysis in case of inheritance and virtual dispatch. Also, it reduces noise: in your sample, there's actually no code path that would print Test Message 2, because there's no instance of Test on which Run() is invoked.

Based on the above CFG model, in order to find the missing flow, we need to add some code that includes the initialization and a call to Run(). So for example adding the following constructor

Test()
{
  Run();
}

works, because it is transformed to

Test()
{
  this.Message2 = "Test Message 2";
  this.Run();
}

and there's field based dataflow from the string literal to WriteLine on the this instance.

Another way to introduce the missing flow would be adding the following:

static void M()
{
  var t = new Test();
  t.Run();
}

public Test() { }

In this case CodeQL finds the flow for instance t. (Note that adding the empty constructor should not be necessary, but due to a bug in the CFG construction, the flow is not found without it. I submitted an internal issue to fix this.)

[hvitved]

Thanks for digging into this @tamasvajk . I can confirm that your explanations are absolutely correct. @aschackmull, didn't you run into a similar issue in the Java analysis? I recall a discussion about adding synthesized method/constructor calls in order to be able to capture flow like this, but I forgot the details.

[aschackmull]

Yes. Besides the toy examples like this where we can easily explain the lack of flow like Tamas just did, then there are a few real cases where this can currently be a problem, namely those where the flow from constructor to method invocation is hidden, e.g. if the object construction happens in an opaque way, say by reflection inside a factory method in a library. But this has not been a big enough problem to prioritise yet.