[dotnet] taint propagation errors / regressions

[Hug0Vincent]

Hello, I'm playing with deserilization vulnerabilities and I found an error where a test case already exist, however I think it's not properly handling the case. I just added a variable assignment and the taint stop in the constructor:

[WebMethod]
public void UpdateConfiguration2(string data)
{
    var ds = new BinaryFormatter();
    // BAD
    MemoryStream ms = new MemoryStream(Convert.FromBase64String(data))
    ds.Deserialize(ms); // $ Alert[cs/unsafe-deserialization-untrusted-input]
}

Here is the original test case:

class BadBinaryFormatter2
{
    public static object Deserialize(TextBox type, TextBox data)
    {
        var ds = new BinaryFormatter();
        // BAD
        return ds.Deserialize(new MemoryStream(Convert.FromBase64String(data.Text))); // $ Alert[cs/unsafe-deserialization-untrusted-input]
    }
}

Some changes were made in some summary models for example in the method FromBase64String here

 - ["System", "Convert", False, "FromBase64String", "(System.String)", "", "Argument[0]", "ReturnValue", "taint", "manual"]

Before it was this (ReturnValue.Element)

  - ["System", "Convert", False, "FromBase64String", "(System.String)", "", "Argument[0]", "ReturnValue.Element", "taint", "manual"]

Here is the summaryModel for MemoryStream, I tried to remove the .Element part but I get the same results:

  - ["System.IO", "MemoryStream", False, "MemoryStream", "(System.Byte[])", "", "Argument[0].Element", "Argument[this]", "taint", "manual"]

It reminds me another problem that we already discuss so I don't know if it's linked: #19911

This .Element is confusing since some methods are using it and other not. Also I don't think there is an implicit read by default but I might be wrong. Do you have any idea ?

Here is my 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 PartialFlow::PartialPathGraph
import semmle.code.csharp.dataflow.flowsources.Remote

private module MyConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) {
    exists(Parameter p |
      p.hasName("data") and 
      p.getCallable().hasName(["UpdateConfiguration2", "Deserialize"]) and
      source.asParameter() = p
    )
  }



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

  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
    exists(MemberAccess ma |
      // node2 is the reading of the field
      ma = node2.asExpr() and
      // node1 is the object being accessed
      ma.getQualifier() = node1.asExpr()
    )
  }
}

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"

Thank you :)

[Hug0Vincent]

My imports are missing and CodeQL didn't find MemoryStream it's my fault sorry for the spam :)

[redsun82]

no worries! Thanks for coming back to this, I was about to launch an investigation 🙂 glad you managed to sort it out!

[Hug0Vincent]

If you have time to look at this one #21567 I would appreciate :)