Merge pull request #2442 from hvitved/csharp/dataflow/conversion-operator

Approved by calumgrant

Author: semmle-qlci

change-notes/1.24/analysis-csharp.md

@@ -0,0 +1,19 @@
+# Improvements to C# analysis
+
+The following changes in version 1.24 affect C# analysis in all applications.
+
+## General improvements
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+
+## Changes to existing queries
+
+| **Query**                  | **Expected impact**    | **Change**                                                       |
+|----------------------------|------------------------|------------------------------------------------------------------|
+
+## Changes to libraries
+
+* The taint tracking library now tracks flow through (implicit or explicit) conversion operator calls.

csharp/ql/src/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll

@@ -113,6 +113,12 @@ private class LocalTaintExprStepConfiguration extends ControlFlowReachabilityCon
           scope = e2 and
           isSuccessor = true
         )
+      or
+      e2 = any(OperatorCall oc |
+          oc.getTarget().(ConversionOperator).fromLibrary() and
+          e1 = oc.getAnArgument() and
+          isSuccessor = true
+        )
     )
   }
 

csharp/ql/test/library-tests/dataflow/local/DataFlowStep.expected

@@ -586,6 +586,9 @@
 | LocalDataFlow.cs:480:67:480:68 | os | LocalDataFlow.cs:486:32:486:33 | access to parameter os |
 | LocalDataFlow.cs:483:21:483:21 | access to parameter x | LocalDataFlow.cs:483:16:483:21 | ... = ... |
 | LocalDataFlow.cs:486:32:486:33 | access to parameter os | LocalDataFlow.cs:486:26:486:33 | ... = ... |
+| LocalDataFlow.cs:491:41:491:44 | args | LocalDataFlow.cs:493:29:493:32 | access to parameter args |
+| LocalDataFlow.cs:493:29:493:32 | [post] access to parameter args | LocalDataFlow.cs:494:27:494:30 | access to parameter args |
+| LocalDataFlow.cs:493:29:493:32 | access to parameter args | LocalDataFlow.cs:494:27:494:30 | access to parameter args |
 | SSA.cs:5:17:5:17 | SSA entry def(this.S) | SSA.cs:67:9:67:14 | access to field S |
 | SSA.cs:5:17:5:17 | this | SSA.cs:67:9:67:12 | this access |
 | SSA.cs:5:26:5:32 | tainted | SSA.cs:8:24:8:30 | access to parameter tainted |

csharp/ql/test/library-tests/dataflow/local/LocalDataFlow.cs

@@ -485,4 +485,12 @@ public void AssignmentFlow(IDisposable x, IEnumerable<object> os)
         IEnumerable<object> os2;
         foreach(var o in os2 = os) { }
     }
+
+    public static implicit operator LocalDataFlow(string[] args) => null;
+
+    public void ConversionFlow(string[] args)
+    {
+        Span<object> span = args; // flow (library operator)
+        LocalDataFlow x = args; // no flow (source code operator)
+    }
 }

csharp/ql/test/library-tests/dataflow/local/TaintTrackingStep.expected

@@ -736,6 +736,11 @@
 | LocalDataFlow.cs:480:67:480:68 | os | LocalDataFlow.cs:486:32:486:33 | access to parameter os |
 | LocalDataFlow.cs:483:21:483:21 | access to parameter x | LocalDataFlow.cs:483:16:483:21 | ... = ... |
 | LocalDataFlow.cs:486:32:486:33 | access to parameter os | LocalDataFlow.cs:486:26:486:33 | ... = ... |
+| LocalDataFlow.cs:491:41:491:44 | args | LocalDataFlow.cs:491:41:491:44 | args |
+| LocalDataFlow.cs:491:41:491:44 | args | LocalDataFlow.cs:493:29:493:32 | access to parameter args |
+| LocalDataFlow.cs:493:29:493:32 | [post] access to parameter args | LocalDataFlow.cs:494:27:494:30 | access to parameter args |
+| LocalDataFlow.cs:493:29:493:32 | access to parameter args | LocalDataFlow.cs:493:29:493:32 | call to operator implicit conversion |
+| LocalDataFlow.cs:493:29:493:32 | access to parameter args | LocalDataFlow.cs:494:27:494:30 | access to parameter args |
 | SSA.cs:5:17:5:17 | SSA entry def(this.S) | SSA.cs:67:9:67:14 | access to field S |
 | SSA.cs:5:17:5:17 | this | SSA.cs:67:9:67:12 | this access |
 | SSA.cs:5:26:5:32 | tainted | SSA.cs:5:26:5:32 | tainted |

csharp/ql/test/library-tests/frameworks/EntityFramework/EntityFrameworkCore.cs

@@ -50,7 +50,7 @@ void TestDataFlow()
             Sink(taintSource);  // Tainted
             Sink(new RawSqlString(taintSource));  // Tainted
             Sink((RawSqlString)taintSource);  // Tainted
-            Sink((RawSqlString)(FormattableString)$"{taintSource}");  // Not tainted
+            Sink((RawSqlString)(FormattableString)$"{taintSource}");  // Tainted, but not reported because conversion operator is in a stub .cs file
 
             // Tainted via database, even though technically there were no reads or writes to the database in this particular case.
             var p1 = new Person { Name = taintSource };