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

protectedFullyInstrumentedType(SerializationInfo info,StreamingContext context){Console.WriteLine("Deserialization constructor");Data=info.GetString("Data");//1BinaryFormatterbinaryFormatter=newBinaryFormatter();using(MemoryStreammemoryStream=newMemoryStream(Convert.FromBase64String(this.Data))){binaryFormatter.Deserialize(memoryStream);}//2byte[]a=Convert.FromBase64String(this.Data);MemoryStreamstream=null;stream=newMemoryStream(a);(newBinaryFormatter()).Deserialize(stream);//3byte[]b=Encoding.UTF8.GetBytes(this.Data);MemoryStreamms=null;ms=newMemoryStream(b);(newBinaryFormatter()).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
privatemodule MyConfig implements DataFlow::ConfigSig{predicateisSource(DataFlow::Nodemysrc){mysrc.asParameter().getCallable()instanceofSerializationConstructorand(mysrc.asParameter().getCallable().hasName("ClaimsPrincipal")ormysrc.asParameter().getCallable().hasName("FullyInstrumentedType"))}predicateisAdditionalFlowStep(DataFlow::Nodenode1, DataFlow::Nodenode2){any(SerializationInfoGetTaintSteps).step(node1,node2)}predicateisSink(DataFlow::Nodesink){none()}}/** * We want to propagate output of SerializationInfo: *  * Data = info.GetString("Data"); */classSerializationInfoGetTaintStepextendsGadgetAdditionalTaintStep{overridepredicatestep(DataFlow::NodefromNode, DataFlow::NodetoNode){exists(MethodCallmc,Methodm|mc.getTarget()=mandm.getName().matches("Get%")andm.getDeclaringType().hasFullyQualifiedName("System.Runtime.Serialization","SerializationInfo")and// Taint flows from the qualifier (info) to the call resultfromNode.asExpr()=mc.getQualifier()andtoNode.asExpr()=mc)}}privatemodule MyFlow = TaintTracking::Global<MyConfig>; // or DataFlow::Global<..>intexplorationLimit(){result=10}privatemodule PartialFlow = MyFlow::FlowExplorationFwd<explorationLimit/0>;
from PartialFlow::PartialPathNodesource, PartialFlow::PartialPathNodesinkwhere PartialFlow::partialFlow(source,sink, _)selectsink.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 ?

[owen-mc]

Opening an issue here is the best way of reporting the issue. Or, if you feel able to fix it yourself, you are always welcome to open a pull request.

[michaelnebel]

The related PR #19940 has been merged.

[Hug0Vincent]

Hi @michaelnebel I've reopened the issue because I found a new variant of this bug this time with the StringReader class here there are summaryModel for some constructors, but others are defined in neutralModel (here). A false positive of one of my query lead me find this one. In my case I have this pattern and I would assume that the result would be tainted, but that's not the case:

newXmlTextReader(newStringReader(taintedString))

As a workaround I've added this line in the yaml file and it works:

- ["System.Xml", "XmlTextReader", False, "XmlTextReader", "(System.IO.TextReader)", "", "Argument[0]", "Argument[this]", "taint", "dfc-generated"]

I'm unsure if certain constructors should be filtered out.

How are those generated model packs created?

[michaelnebel]

Opening a PR for the modelling here: #20290

I suppose the ReadXmlSchema is declared in the codebase you are analyzing? Otherwise, the partial flow might stop here as well.
You are more than welcome to contribute to the modelling of APIs when you discover that models are missing/not working - we would be very happy to get such contributions! 😄

[Hug0Vincent]

Yes exactly the function is from my codebase.

Perfect that's a nice example, I have 2 questions:

1. what's the difference between the tags: manual/df-manual/dfc-manual ?
2. How to build the expected test cases ?

Did you manually review all of the functions and do you use the vscode extension for modeling ?

[michaelnebel]

• The model provenance information manual, df-manual, dfc-manual, df-generated and dfc-generated: manual means a model that we came up with, df-manual/dfc-manual is a model that was generated by our model generator (df means by our heuristic model generation and dfc means by our content based model generation) but that we have manually "inspected" (reviewed) and thus "promoted" to a manual model (that is, models that already existed but had provenance df-generated or dfc-generated). This means that whatever models you provide will be "manual". Also, it is important to note that there is a difference in semantics for manual and generated models: generated models are ONLY applied in case there is no source code available for the callable (the logic is slightly more complicated - but this is the gist of it), whereas manual models are always applied - even if the source code is available.
• FlowSummaries*.expected files can be updated by running codeql test run csharp/ql/test/library-tests/dataflow/library/ --learn in the CodeQL clone.
• I manually reviewed all the functions and didn't use the VS Code extension. I think that the extension is most suitable when you are trying to model the codebase in your current workspace.

[Hug0Vincent]

Ok thank you so much, now I don't have any excuse, next time I'll open a PR. thank you again for all the help it's always nice to ask a question here :)

For the model generation you then fill all the parameter and types by hand ?

[michaelnebel]

For these cases there were neutral models for most of the methods (they have the right signatures) - I just copied those and provided the correct input/output columns (and the other minor remaining). It is a bit cumbersome, but at least it is easier compared to when we had to write QL for such things. 😄

[Hug0Vincent]

I have one new here also, fixed with this:

- ["System.Xml", "XmlDictionaryReader", True, "ReadContentAsBase64", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]

Some other Read* might need to be added as well

[michaelnebel]

This should also be covered by #20290

[michaelnebel]

@Hug0Vincent : If you experience more gaps in the modelling - feel free to make a PR or open another issue with a request. The turnaround time is typically faster, if you open a PR 😄

[Hug0Vincent]

Hey @michaelnebel I'm trying to play with the sinkModel but there is something that I don't understand, I have this model:

extensions:
- addsTo:
pack: codeql/csharp-allextensible: sinkModeldata:
- ["System", "Type", True, "GetMethod", "(System.String)", "", "Argument[0]", "gadget-sink", "manual"]
- ["System.Reflection", "MethodBase", True, "Invoke", "(System.Object,System.Object[])", "", "Argument[0]", "gadget-sink", "manual"]

But it can't find the call to the Invoke method:

What I'm I missing ? Since MethodInfo extends MethodBase should my model also catch the call MethodInfo.Invoke ?

According to the documentation the third parameter is subtypes but I'm not sure if it's for overriden methods or all subtypes.

Here is my code and my query:

/** * @id synacktiv/csharp/test * @description test * @name test * @kind table * @tags audit */import csharp
privateimport semmle.code.csharp.security.dataflow.flowsinks.FlowSinks
privateimport semmle.code.csharp.dataflow.internal.ExternalFlow
abstractclassSinkextendsApiSinkExprNode{}privateclassExternalGadgetSinkextendsSink{ExternalGadgetSink(){sinkNode(this,"gadget-sink")}}fromExternalGadgetSinksselects

usingSystem;usingSystem.Collections.Generic;usingSystem.Linq;usingSystem.Reflection;usingSystem.Text;usingSystem.Threading.Tasks;namespaceConsoleApp1{internalclassProgram{staticvoidMain(string[]args){BasePoc.BasePocPoc();}publicstaticvoidTest(){// 1. Create an instance of the classMyClassmyObject=newMyClass();// 2. Get the Type objectTypetype=typeof(MyClass);// 3. Get the MethodInfo object for the method you want to callMethodInfomethodInfo=type.GetMethod("SayHello");MethodBasemethodInfo2=type.GetMethod("SayHello");if(methodInfo!=null){// 4. Call the method using MethodInfo.InvokemethodInfo.Invoke(myObject,newobject[]{"Alice"});methodInfo2.Invoke(myObject,newobject[]{"Alice"});}else{Console.WriteLine("Method not found.");}}}publicclassMyClass{publicvoidSayHello(stringname){Console.WriteLine($"Hello, {name}!");}}}

Let me know if I'm asking too many questions

[Hug0Vincent]

I'm confused by this behavior. The function only intercepts the call to Invoke when there's a concat in the GetMethod invocation. I don’t understand why this happens:

privatestaticvoidTest2(objectobj,stringname){MethodInfomethod0;method0=obj.GetType().GetMethod("test");method0.Invoke(obj,newobject[0]);MethodInfomethod1;method1=obj.GetType().GetMethod("test"+name);method1.Invoke(obj,newobject[0]);// catchedMethodInfomethod2;method2=obj.GetType().GetMethod("test",BindingFlags.Public);method2.Invoke(obj,newobject[0]);MethodInfomethod3;method3=obj.GetType().GetMethod("test"+name,BindingFlags.Public);method3.Invoke(obj,newobject[0]);// catched}

[michaelnebel]

Sorry for the late reply (had to do some debugging to figure out the cause). You are asking excellent questions, which have already revealed some areas where we could improve our code.

We have all kinds of special handling for reflection methods when it comes to dataflow and dispatch call. As a consequence, it is currently not possible to add models for the reflection (Invoke, InvokeMember) methods (there are only a couple of these). This is an exceptional case - and I don't think we plan to make support for it. Very impressive that you ran into it 😄

[Hug0Vincent]

Alright, thank you so much for this answer i was going crazy 😅 Are there other exceptions or just this one with reflection? I can still do the same without a model, with plain codeql.

[michaelnebel]

The part of the code only excluded the reflection methods 😄 If you find other examples, we would like to hear about it as well!