[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.

[michaelnebel]

Thank you for the question.

You are right,

1. Members are not automatically tainted - it requires some extra logic.
2. Only AspNetCoreRemoteFlowSource automatically taints members.

It is worth noting that all uses of types used as eg. a parameter type for an ASP.NET action method will have their members tainted with this logic. That is, introducing taint members logic can yield results other places in the code - as it is the use of the type in the ASP.NET context that decides whether we consider the members tainted (in all use-cases in a given database).

I don't think the this.isAutoImplemented() should be removed - this might lead to an over-approximation of tainted members in case there user defined setters and getters (the check is there to be as conservative as possible).
Note, that the spring and Java tainting doesn't "transitively" taint (it only taints the result when calling getters on "unsafe" types).

I will try and draft a PR for generally tainting members on types used in ASP.NET, but I am not sure that we will be able to merge it - as it might be considered a bit to controversial to introduce in general (in case we don't merge it, you are than welcome to use the code in your own query).

[Hug0Vincent]

I think that the ASP.NET Core equivalent should also be affected by this change, what do you think ?

private class AspNetCoreRemoteFlowSourceMember extends TaintTracking::TaintedMember, Property {
  AspNetCoreRemoteFlowSourceMember() {
    this.getDeclaringType() = any(AspNetCoreRemoteFlowSource source).getType() and
    this.isPublic() and
    not this.isStatic() and
    this.isAutoImplemented() and
    this.getGetter().isPublic() and
    this.getSetter().isPublic()
  }
}

[michaelnebel]

It is worth noting that all uses of types used as eg. a parameter type for an ASP.NET action method will have their members tainted with this logic. That is, introducing taint members logic can yield results other places in the code - as it is the use of the type in the ASP.NET context that decides whether we consider the members tainted (in all use-cases in a given database).

For instance (from the test case in the PR), we have

[WebMethod]
public void MyMethod(MyData data) { ... }

This means that in all other places where use the type MyData - then the members of MyData will be considered tainted (if the object itself is tainted).
If we have something like

public void M() {
  MyData d = GetTaintedMyData();
  // All properties and fields of `d` are now recursively tainted as well, because the type `MyData` is used as a parameter type in `MyMethod`
} 

Even though M might be located in a completely different place in the code.

[michaelnebel]

Btw. would it make sense to require that a type is Serializable when tainting the members?

[michaelnebel]

I think that the ASP.NET Core equivalent should also be affected by this change, what do you think ?

private class AspNetCoreRemoteFlowSourceMember extends TaintTracking::TaintedMember, Property {
  AspNetCoreRemoteFlowSourceMember() {
    this.getDeclaringType() = any(AspNetCoreRemoteFlowSource source).getType() and
    this.isPublic() and
    not this.isStatic() and
    this.isAutoImplemented() and
    this.getGetter().isPublic() and
    this.getSetter().isPublic()
  }
}

Yes, if we end up merging it, then that is probably a good idea!

[Hug0Vincent]

Btw. would it make sense to require that a type is Serializable when tainting the members?

I think the requirement is only on Getter/Setters.

Even though M might be located in a completely different place in the code.

So why taint Properties and not output of Getters in this case ? How is it handled in other languages ?

[michaelnebel]

Thank you for the input. The PR has been merged and can be found here.

[Hug0Vincent]

Thank you so much for your time and for this PR 🚀

[michaelnebel]

I really appreciate the input and sorry for the delay (other stuff came up, which caused the delay).

[Hug0Vincent]

No worries we are all the same ☺️