Merge pull request #8675 from michaelnebel/csharp/capturemodelimprovement

C#: CaptureModel improvements

csharp/ql/src/utils/model-generator/internal/CaptureModelsSpecific.qll

@@ -7,6 +7,7 @@ private import semmle.code.csharp.commons.Util as Util
 private import semmle.code.csharp.commons.Collections as Collections
 private import semmle.code.csharp.dataflow.internal.DataFlowDispatch
 private import semmle.code.csharp.frameworks.System as System
+private import semmle.code.csharp.frameworks.system.linq.Expressions
 import semmle.code.csharp.dataflow.ExternalFlow as ExternalFlow
 import semmle.code.csharp.dataflow.internal.DataFlowImplCommon as DataFlowImplCommon
 import semmle.code.csharp.dataflow.internal.DataFlowPrivate as DataFlowPrivate
@@ -18,21 +19,11 @@ module TaintTracking = CS::TaintTracking;
 class Type = CS::Type;
 
 /**
- * Holds if `api` is an override or an interface implementation that
- * is irrelevant to the data flow analysis.
+ * Holds if any of the parameters of `api` are `System.Func<>`.
  */
-private predicate isIrrelevantOverrideOrImplementation(CS::Callable api) {
-  exists(CS::Callable exclude, CS::Method m |
-    (
-      api = m.getAnOverrider*().getUnboundDeclaration()
-      or
-      api = m.getAnUltimateImplementor().getUnboundDeclaration()
-    ) and
-    exclude = m.getUnboundDeclaration()
-  |
-    exists(System::SystemObjectClass c | exclude = [c.getGetHashCodeMethod(), c.getEqualsMethod()])
-    or
-    exists(System::SystemIEquatableTInterface i | exclude = i.getEqualsMethod())
+private predicate isHigherOrder(CS::Callable api) {
+  exists(Type t | t = api.getAParameter().getType().getUnboundDeclaration() |
+    t instanceof SystemLinqExpressions::DelegateExtType
   )
 }
 
@@ -44,7 +35,7 @@ private predicate isRelevantForModels(CS::Callable api) {
   api.getDeclaringType().getNamespace().getQualifiedName() != "" and
   not api instanceof CS::ConversionOperator and
   not api instanceof Util::MainMethod and
-  not isIrrelevantOverrideOrImplementation(api)
+  not isHigherOrder(api)
 }
 
 /**
@@ -65,8 +56,13 @@ predicate asPartialModel = DataFlowPrivate::Csv::asPartialModel/1;
 /**
  * Holds for type `t` for fields that are relevant as an intermediate
  * read or write step in the data flow analysis.
+ * That is, flow through any data-flow node that does not have a relevant type
+ * will be excluded.
  */
-predicate isRelevantType(CS::Type t) { not t instanceof CS::Enum }
+predicate isRelevantType(CS::Type t) {
+  not t instanceof CS::SimpleType and
+  not t instanceof CS::Enum
+}
 
 /**
  * Gets the CSV string representation of the qualifier.

csharp/ql/test/utils/model-generator/CaptureSummaryModels.expected

@@ -1,5 +1,5 @@
-| NoSummaries;PublicClassFlow;false;PublicReturn;(System.Int32);;Argument[0];ReturnValue;generated:taint |
-| Summaries;BaseClassFlow;true;ReturnParam;(System.Int32);;Argument[0];ReturnValue;generated:taint |
+| NoSummaries;PublicClassFlow;false;PublicReturn;(System.Object);;Argument[0];ReturnValue;generated:taint |
+| Summaries;BaseClassFlow;true;ReturnParam;(System.Object);;Argument[0];ReturnValue;generated:taint |
 | Summaries;BasicFlow;false;ReturnField;();;Argument[Qualifier];ReturnValue;generated:taint |
 | Summaries;BasicFlow;false;ReturnParam0;(System.String,System.Object);;Argument[0];ReturnValue;generated:taint |
 | Summaries;BasicFlow;false;ReturnParam1;(System.String,System.Object);;Argument[1];ReturnValue;generated:taint |
@@ -11,14 +11,14 @@
 | Summaries;CollectionFlow;false;AddFieldToList;(System.Collections.Generic.List<System.String>);;Argument[Qualifier];Argument[0].Element;generated:taint |
 | Summaries;CollectionFlow;false;AddToList;(System.Collections.Generic.List<System.Object>,System.Object);;Argument[1];Argument[0].Element;generated:taint |
 | Summaries;CollectionFlow;false;AssignFieldToArray;(System.Object[]);;Argument[Qualifier];Argument[0].Element;generated:taint |
-| Summaries;CollectionFlow;false;AssignToArray;(System.Int32,System.Int32[]);;Argument[0];Argument[1].Element;generated:taint |
-| Summaries;CollectionFlow;false;ReturnArrayElement;(System.Int32[]);;Argument[0].Element;ReturnValue;generated:taint |
+| Summaries;CollectionFlow;false;AssignToArray;(System.Object,System.Object[]);;Argument[0];Argument[1].Element;generated:taint |
+| Summaries;CollectionFlow;false;ReturnArrayElement;(System.Object[]);;Argument[0].Element;ReturnValue;generated:taint |
 | Summaries;CollectionFlow;false;ReturnFieldInAList;();;Argument[Qualifier];ReturnValue;generated:taint |
 | Summaries;CollectionFlow;false;ReturnListElement;(System.Collections.Generic.List<System.Object>);;Argument[0].Element;ReturnValue;generated:taint |
-| Summaries;DerivedClass1Flow;false;ReturnParam1;(System.Int32,System.Int32);;Argument[1];ReturnValue;generated:taint |
-| Summaries;DerivedClass2Flow;false;ReturnParam0;(System.Int32,System.Int32);;Argument[0];ReturnValue;generated:taint |
-| Summaries;DerivedClass2Flow;false;ReturnParam;(System.Int32);;Argument[0];ReturnValue;generated:taint |
-| Summaries;EqualsGetHashCodeNoFlow;false;Equals;(System.Int32);;Argument[0];ReturnValue;generated:taint |
+| Summaries;DerivedClass1Flow;false;ReturnParam1;(System.String,System.String);;Argument[1];ReturnValue;generated:taint |
+| Summaries;DerivedClass2Flow;false;ReturnParam0;(System.String,System.Int32);;Argument[0];ReturnValue;generated:taint |
+| Summaries;DerivedClass2Flow;false;ReturnParam;(System.Object);;Argument[0];ReturnValue;generated:taint |
+| Summaries;EqualsGetHashCodeNoFlow;false;Equals;(System.String);;Argument[0];ReturnValue;generated:taint |
 | Summaries;GenericFlow<>;false;AddFieldToGenericList;(System.Collections.Generic.List<T>);;Argument[Qualifier];Argument[0].Element;generated:taint |
 | Summaries;GenericFlow<>;false;AddToGenericList<>;(System.Collections.Generic.List<S>,S);;Argument[1];Argument[0].Element;generated:taint |
 | Summaries;GenericFlow<>;false;ReturnFieldInGenericList;();;Argument[Qualifier];ReturnValue;generated:taint |

csharp/ql/test/utils/model-generator/NoSummaries.cs

@@ -6,7 +6,7 @@ namespace NoSummaries;
 // Just to prove that, if a method like this is correctly exposed, a flow summary will be captured.
 public class PublicClassFlow
 {
-    public int PublicReturn(int input)
+    public object PublicReturn(object input)
     {
         return input;
     }
@@ -14,19 +14,19 @@ public class PublicClassFlow
 
 public sealed class PublicClassNoFlow
 {
-    private int PrivateReturn(int input)
+    private object PrivateReturn(object input)
     {
         return input;
     }
 
-    internal int InternalReturn(int input)
+    internal object InternalReturn(object input)
     {
         return input;
     }
 
     private class PrivateClassNoFlow
     {
-        public int ReturnParam(int input)
+        public object ReturnParam(object input)
         {
             return input;
         }
@@ -36,7 +36,7 @@ public sealed class PublicClassNoFlow
     {
         public class NestedPublicClassFlow
         {
-            public int ReturnParam(int input)
+            public object ReturnParam(object input)
             {
                 return input;
             }
@@ -44,10 +44,10 @@ public sealed class PublicClassNoFlow
     }
 }
 
-public class EquatableBound : IEquatable<int>
+public class EquatableBound : IEquatable<object>
 {
     public readonly bool tainted;
-    public bool Equals(int other)
+    public bool Equals(object other)
     {
         return tainted;
     }
@@ -60,4 +60,42 @@ public class EquatableUnBound<T> : IEquatable<T>
     {
         return tainted;
     }
+}
+
+// No methods in this class will have generated flow summaries as
+// simple types are used.
+public class SimpleTypes
+{
+    public bool M1(bool b)
+    {
+        return b;
+    }
+
+    public Boolean M2(Boolean b)
+    {
+        return b;
+    }
+
+    public int M3(int i)
+    {
+        return i;
+    }
+
+    public Int32 M4(Int32 i)
+    {
+        return i;
+    }
+}
+
+public class HigherOrderParameters
+{
+    public string M1(string s, Func<string, string> map)
+    {
+        return s;
+    }
+
+    public object M2(Func<object, object> map, object o)
+    {
+        return map(o);
+    }
 }

csharp/ql/test/utils/model-generator/Summaries.cs

@@ -48,12 +48,12 @@ public class CollectionFlow
 {
     private string tainted;
 
-    public int ReturnArrayElement(int[] input)
+    public object ReturnArrayElement(object[] input)
     {
         return input[0];
     }
 
-    public void AssignToArray(int data, int[] target)
+    public void AssignToArray(object data, object[] target)
     {
         target[0] = data;
     }
@@ -146,7 +146,7 @@ public class GenericFlow<T>
 
 public abstract class BaseClassFlow
 {
-    public virtual int ReturnParam(int input)
+    public virtual object ReturnParam(object input)
     {
         return input;
     }
@@ -154,7 +154,7 @@ public abstract class BaseClassFlow
 
 public class DerivedClass1Flow : BaseClassFlow
 {
-    public int ReturnParam1(int input0, int input1)
+    public string ReturnParam1(string input0, string input1)
     {
         return input1;
     }
@@ -162,12 +162,12 @@ public class DerivedClass1Flow : BaseClassFlow
 
 public class DerivedClass2Flow : BaseClassFlow
 {
-    public override int ReturnParam(int input)
+    public override object ReturnParam(object input)
     {
         return input;
     }
 
-    public int ReturnParam0(int input0, int input1)
+    public string ReturnParam0(string input0, int input1)
     {
         return input0;
     }
@@ -195,13 +195,13 @@ public class OperatorFlow
     }
 
     // No flow summary as this is an implicit conversion operator.
-    public static implicit operator OperatorFlow(int i)
+    public static implicit operator OperatorFlow(string s)
     {
-        return new OperatorFlow(i);
+        return new OperatorFlow(s);
     }
 
     // No flow summary as this is an explicit conversion operator.
-    public static explicit operator OperatorFlow(byte b)
+    public static explicit operator OperatorFlow(string[] b)
     {
         return new OperatorFlow(b);
     }
@@ -220,9 +220,9 @@ public class EqualsGetHashCodeNoFlow
     }
 
     // Flow summary as this is not an override of the object Equals method.
-    public int Equals(int i)
+    public string Equals(string s)
     {
-        return i;
+        return s;
     }
 
     // No flow summary as this is an override of the GetHashCode method.