[dotnet] Missing Taint Flow from [WebMethod] Parameter Objects to Properties/Fields

[Hug0Vincent]

In ASP.NET Legacy Web Services (System.Web.Services), parameters decorated with the [WebMethod] attribute are correctly identified as RemoteFlowSource. However, the TaintTracking engine fails to propagate taint from the parameter object itself to its properties or fields during an assignment.

using System;
using System.Web.Services;
using System.Diagnostics;

namespace CodeQLRepro
{
    public class MyWrapper
    {
        public string Payload { get; set; }
    }

    public class VulnerableService : WebService
    {
        [WebMethod]
        public void ExecuteCommand(MyWrapper data)
        {
            if (data != null)
            {
                // TAINT GAP: 'data' is a RemoteFlowSource, but 'cmd' is considered clean.
                string cmd = data.Payload;

                if (!string.IsNullOrEmpty(cmd))
                {
                    // SINK: Remote Command Execution (RCE)
                    // This is NOT flagged by the standard CodeQL C# suite.
                    Process.Start("cmd.exe", "/c " + cmd);
                }
            }
        }
    }
}

When running a global taint-tracking query, the flow starts at the data parameter but terminates immediately at the property access data.Payload. The variable cmd is not marked as tainted.

Here is my 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 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(["ExecuteCommand", "UpdateConfiguration"]) and
    //  source.asParameter() = p
    //)
    source instanceof RemoteFlowSource
  }

  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"

Adding the isAdditionalFlowStep resolves the issue. Is it a normal behavior ?

For Java there is this method localAdditionalTaintExprStep, and I think it would be nice to do the same for csharp. I see references to spring in this file and I think it's a bit similar to ASP.NET in some way.

[Hug0Vincent]

Hello, I think you just quoted my answer without any comment is it normal ?

[Hug0Vincent]

I think I found the issue but I don't know how to fix it, here this class does not have associated TaintedMember class that could propagate taint on properties.

I found an example for ASP.NET Core here but it's not transitive. It's possible to have something like this:

public class AddresDto
{
      public string City {get; set;}
} 

public class UserDto
{
      public AddresDto Address {get; set;}
} 

Also the this.isAutoImplemented() check can be removed I think.

I tried to make a fix but it need some recursion to get a property of a property. So I think it would be easier to taint outputs of getters like in spring and Java. It's internal CodeQL code so I don't know how to approach this.