[CSharp] - Set a constructor as a source for a DataFlow analysis

[314erre]

Hi there ! First of all, thanks for this awesome project !

I've been working on some basic C# queries in order to find issues in my codebase, which are working pretty well !
However, I'm struggling to create a "basic" DataFlow analysis, and coudn't find any documentation applying to C#.

I'm trying to get catch these kinds of patterns :

namespace CodeQLTest
{
    [Serializable]
    internal class Parent
    {
        public string parentprop { get; set; }
        public void step2 ()
        {
            // Some sink that is hit by a tainted value
            Sink(this.parentprop);
        }
    }

    [Serializable]
    internal class Child : Parent , ISerializable
    {
        public string bla { get; set; }

         // This is the source, weither it is called in the codebase or not
        public Child(SerializationInfo info, StreamingContext context)
        {
            info.AddValue("bla", this.bla);
            this.step1()
        }

        public void step1()
        {
            this.step2();  
        }
        public void GetObjectData(SerializationInfo info, StreamingContext context)
        {
            throw new NotImplementedException();
        }
    }
}

I don't really understand how am i supposed to create a proper isSource predicate of my DataFlow ?
Moreover, as during a deserialization process, all properties (except some special cases) can be controlled my goal would be to set all the object's properties as tainted. Do you happen to know if that is possible ?
For the isSink I would only need to create a predicate matching some Sink Method, which can be represented as so :

class ProcessStart extends Method {
    ProcessStart() {
        hasFullyQualifiedName("System.Diagnostics.Process", "Start")
    }
}

class DangerousMethod extends Callable {
    DangerousMethod() {
        this instanceof ProcessStart
    }
}
/* ... Within the DataFlow */ 
    predicate isSink(DataFlow::Node sink) {
    exists(MethodCall mc | 
        mc.getTarget() instanceof DangerousMethod and
        mc.getArgument(0) = sink.asExpr()
    )
  }

Does this make any sense ?

Thanks ! :)

[redsun82]

👋 @314erre

Could you elaborate on what you mean with

         // This is the source, weither it is called in the codebase or not

Do you mean that any Child deserialization should be considered a source?

Some documentation about C# data flow analysis can be found here, but I'm guessing you already read through that, and it doesn't help your specific problem.

[314erre]

Thanks ! So I've added a isAdditionalFlowStep predicate to make it work, which is great !

However, what seams a bit odd is that the System.Runtime.Serialization.SerializationInfo.GetString should definitely be adding a taint flow by default ? In the C:\Users\Administrator\.codeql\packages\codeql\csharp-all\4.0.2\System.Runtime.Serialization.model.yaml, the following summaryModel can be found (line 52) :

      - ["System.Runtime.Serialization", "SerializationInfo", False, "GetString", "(System.String)", "", "Argument[this].SyntheticField[System.Runtime.Serialization.SerializationInfo._values].Element", "ReturnValue", "value", "dfc-generated"]

I guess this should allow taint flow through the GetString of the System.Runtime.Serialization.SerializationInfo instance.

So I don't really understand why it isn't working on my side !

Here's my setup using CodeQL CLI v2.20.3 :

• qlpack.yml :

---
library: false
warnOnImplicitThis: false
name: foo/test
version: 0.0.1
dependencies:
  codeql/csharp-all: '*'

• codeql-pack-lock.yml :

---
lockVersion: 1.0.0
dependencies:
  codeql/controlflow:
    version: 1.0.15
  codeql/csharp-all:
    version: 4.0.2
  codeql/dataflow:
    version: 1.1.9
  codeql/mad:
    version: 1.0.15
  codeql/ssa:
    version: 1.0.15
  codeql/threat-models:
    version: 1.0.15
  codeql/tutorial:
    version: 1.0.15
  codeql/typetracking:
    version: 1.0.15
  codeql/util:
    version: 2.0.2
  codeql/xml:
    version: 1.0.15
compiled: false

I've also attached the queries logs and the query itself :)
query.ql.txt
codeql_logs.txt

[redsun82]

Hmm, I have a hunch, that the model for GetString in SerializationInfo will make taint flow from the internal data carried by the SerializationInfo object to the return value, not from the SerializationInfo object itself. If that's the case, tainting info won't help here. Let me ask around.

[hvitved]

@redsun82 is absolutely right, the crucial part is .SyntheticField[System.Runtime.Serialization.SerializationInfo._values].Element. If that is removed, it should work.

[hugo-syn]

Hi @hvitved could you explain this : .SyntheticField[System.Runtime.Serialization.SerializationInfo._values].Element or link some documentation on this please :)

[owen-mc]

Suppose that a type T has field f which is a collection of some kind, and a function foo returns an element of that collection. To refer to that in a models-as-data model we would use argument[this].Field[T.f].Element. This allows us to track taint more precisely than making the whole class tainted when some element of some field of it is all that is tainted. Now, sometimes when we are writing models we want to do something like this, but there isn't a public field to refer to. Normally there is a private field. In this case we use SyntheticField instead of Field. Values stored in a synthetic field can only be read out by another model which refers to the synthetic field. Does that make sense?