[csharp] need help with taint propagation

[Hug0Vincent]

Hi, I don't understand why my taint is not propagating. I have 3 test cases that look similar, but it only works for one. Here is my C# code:

protected FullyInstrumentedType(SerializationInfo info, StreamingContext context)
        {
            Console.WriteLine("Deserialization constructor");
            Data = info.GetString("Data");

            //1
            BinaryFormatter binaryFormatter = new BinaryFormatter();
            using (MemoryStream memoryStream = new MemoryStream(Convert.FromBase64String(this.Data)))
            {
                binaryFormatter.Deserialize(memoryStream);
            }

            //2
            byte[] a = Convert.FromBase64String(this.Data);
            MemoryStream stream = null;
            stream =  new MemoryStream(a);
            (new BinaryFormatter()).Deserialize(stream);

            //3
            byte[] b = Encoding.UTF8.GetBytes(this.Data);
            MemoryStream ms = null;
            ms = new MemoryStream(b);
            (new BinaryFormatter()).Deserialize(ms);

In the first scenario, the taint halts at the FromBase64String call, whereas in the third scenario, it persists until the Deserialize call, which aligns with my expectations. This observation was confirmed through a partial dataflow query:

/**
 * @name Forward Partial Dataflow
 * @description Forward Partial Dataflow
 * @kind path-problem
 * @precision low
 * @problem.severity error
 * @id githubsecuritylab/forward-partial-dataflow
 * @tags template
 */

import csharp
import semmle.code.csharp.dataflow.TaintTracking
import semmle.code.csharp.serialization.Serialization
import PartialFlow::PartialPathGraph
import libs.Source

private module MyConfig implements DataFlow::ConfigSig {
  
  predicate isSource(DataFlow::Node mysrc) {
      mysrc.asParameter().getCallable() instanceof SerializationConstructor and
      ( mysrc.asParameter().getCallable().hasName("ClaimsPrincipal") or 
        mysrc.asParameter().getCallable().hasName("FullyInstrumentedType")
      )
    
  }

  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
    any(SerializationInfoGetTaintStep s).step(node1, node2) 
  }

  predicate isSink(DataFlow::Node sink) { none() }
}

/**
 * We want to propagate output of SerializationInfo:
 * 
 *  Data = info.GetString("Data");
 */
class SerializationInfoGetTaintStep extends GadgetAdditionalTaintStep {
  override predicate step(DataFlow::Node fromNode, DataFlow::Node toNode) {
    exists(MethodCall mc, Method m |
      mc.getTarget() = m and
      m.getName().matches("Get%") and
      m.getDeclaringType().hasFullyQualifiedName("System.Runtime.Serialization", "SerializationInfo") and

      // Taint flows from the qualifier (info) to the call result
      fromNode.asExpr() = mc.getQualifier() and
      toNode.asExpr() = mc
    )
  }
}

private module MyFlow = TaintTracking::Global<MyConfig>; // or DataFlow::Global<..>

int explorationLimit() { result = 10 }

private module PartialFlow = MyFlow::FlowExplorationFwd<explorationLimit/0>;

from PartialFlow::PartialPathNode source, PartialFlow::PartialPathNode sink
where PartialFlow::partialFlow(source, sink, _)
select sink.getNode(), source, sink, "This node receives taint from $@.", source.getNode(),
  "this source"

Based on this it should propagate the taint. What am I missing here ?

Thank you.

[Hug0Vincent]

Using the same query on a different Codeql database yield the same problem on another method:

And again there is a summary step for this method here. So the problem might be somewhere else, I don't understand

[Hug0Vincent]

Oh no here there is a neutral step for this method, I don't understand why there is a difference between the 2 method signatures.

[michaelnebel]

There is probably not a difference - but our model generator has not been able to detect flow for the CreateBinaryReader for all overloads (these are automatically generated). Which overload are you using in the screenshot to the left?

[Hug0Vincent]

It's this one and it's in the neutralModel part, if you want the MS doc it's here.

[Hug0Vincent]

I think that in my first example the difference is in the way the return value is tainted. For GetBytes it's the ReturnValue (here) and for FromBase64String (here) it's ReturnValue.Element. What's the difference ?

[Hug0Vincent]

I have another example here where the taint stop with the same query:

I don't know which is responsible here or here. I'm probably missing something and hope someone can explain it, thank you :)

[michaelnebel]

The issue is that the models for the methods can't be "chained" correctly.
As it is now GetBytes taints the return object (the array) and MemoryStream(byte[]) propagates taint from the array to the object.
However, the summary for FromBase64String only states that the elements of the return value are tainted (so this can't be correctly chained with the summary for MemoryStream(byte[])).

[michaelnebel]

I overlooked one of your questions:
It appears that the isAdditionalFlowStep predicate in the provided example also covers the info.GetEnumerator(), which explains why taint propagates just fine to enumerator. However, this doesn’t work well with the generated models for these types. Instead of adding the query/example specific additional flow steps, we might be able to introduce some models for these types that cover this case (and which are reasonable for general use).

[Hug0Vincent]

Yes I initially added this for the GetString method because it was not propagating the taint as explained in this discussion: #18647 (it might be the same problem actually)

I didn't realize that it would also be applied to GetEnumerator ! It would be nice to have a model for this

[michaelnebel]

Thank you very much for reporting this.
I suspect at least for some of the cases there are some issues with the Models as Data modelling of the library methods.
With the current modelling we have

Convert.FromBase64String(string) : Argument[0] -> ReturnValue.Element
Encoding.GetBytes(string) : Argument[0] -> ReturnValue
MemoryStream.MemoryStream(byte[]) : Argument[0] -> Argument[this]

It looks like the model for MemoryStream.MemoryStream(byte[]) is incorrect (it should be Argument[0].Element -> Argument[this] - this is also the case for the other overloads of the constructor). Also the model for Encoding.GetBytes looks incorrect as taint is propagated from the argument to the return value (instead of the elements of the return value).

I will look into this.

[Hug0Vincent]

Thank you for your answer, let me know if I can provide additional information

[michaelnebel]

@Hug0Vincent : Will see if I can fix at least some of the modelling and get back to you.

[Hug0Vincent]

Thank you so much for looking into this :)

[michaelnebel]

Related PR #19940

[Hug0Vincent]

Thank you so much it works now for my case ! I haven't tested the new models for System.Runtime.Serialization since I want to catch all the Get* calls.

If I encounter similar issues with other functions, what is the best way to report them ?