Merge pull request #5 from hvitved/csharp/bad-practices-implementation-hiding

C#: Update queries in `Bad Practices/Implementation Hiding`

change-notes/1.18/analysis-csharp.md

@@ -15,6 +15,7 @@
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
+| [Exposing internal representation (cs/expose-implementation)] | Different results | The query has been rewritten, based on the equivalent Java query. |
 | Local scope variable shadows member (cs/local-shadows-member) | maintainability, readability | Replaces the existing queries [Local variable shadows class member (cs/local-shadows-class-member)](https://help.semmle.com/wiki/display/CSHARP/Local+variable+shadows+class+member), [Local variable shadows struct member (cs/local-shadows-struct-member)](https://help.semmle.com/wiki/display/CSHARP/Local+variable+shadows+struct+member), [Parameter shadows class member (cs/parameter-shadows-class-member)](https://help.semmle.com/wiki/display/CSHARP/Parameter+shadows+class+member), and [Parameter shadows struct member (cs/parameter-shadows-struct-member)](https://help.semmle.com/wiki/display/CSHARP/Parameter+shadows+struct+member). |
 
 ## Changes to existing queries

csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.cs

@@ -1,10 +0,0 @@
-class AbstractToConcreteCollection
-{
-    public static void Main(string[] args)
-    {
-        ICollection<String> foo = new List<String>();
-        foo.Add("hello");
-        foo.Add("world");
-        List<String> bar = (List<String>)foo; // BAD
-    }
-}

csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.qhelp

@@ -12,8 +12,8 @@ more difficult to change which implementation you are using at a later date.</p>
 
 </recommendation>
 <example>
-<p>The example shows casting from an ICollection to a List. This should be avoided where possible.</p>
+<p>The example shows casting from an <code>IEnumerable&lt;string&gt;</code> to a <code>List&lt;string&gt;</code>. This should be avoided where possible.</p>
-<sample src="AbstractToConcreteCollection.cs" />
+<sample src="AbstractToConcreteCollectionBad.cs" />
 
 </example>
 <references>

csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.ql

@@ -1,8 +1,8 @@
 /**
  * @name Cast from abstract to concrete collection
- * @description Finds casts from an abstract collection to a concrete implementation
+ * @description A cast from an abstract collection to a concrete implementation type
- *              type. This makes the code brittle; it is best to program against the
+ *              makes the code brittle; it is best to program against the abstract
- *              abstract collection interfaces only.
+ *              collection interface only.
  * @kind problem
  * @problem.severity warning
  * @precision medium
@@ -13,20 +13,25 @@
  *       external/cwe/cwe-485
  */
 import csharp
+import semmle.code.csharp.frameworks.system.Collections
+import semmle.code.csharp.frameworks.system.collections.Generic
 
-/** A sub-interface of Collection */
+/** A collection interface. */
 class CollectionInterface extends Interface {
   CollectionInterface() {
-    exists(string name |
+    exists(Interface i |
-           this.getSourceDeclaration().getABaseType*().getName() = name and
+      i = this.getABaseInterface*() |
-           (   name.matches("ICollection<%>")
+      i instanceof SystemCollectionsICollectionInterface or
-            or name="ICollection")
+      i.getSourceDeclaration() instanceof SystemCollectionsGenericICollectionInterface or
+      i instanceof SystemCollectionsIEnumerableInterface or
+      i.getSourceDeclaration() instanceof SystemCollectionsGenericIEnumerableTInterface
     )
   }
 }
 
 from CastExpr e, Class c, CollectionInterface i
-where e.getType() = c and
+where e.getType() = c
-      e.getExpr().getType().(RefType).getSourceDeclaration() = i
+  and e.getExpr().getType() = i
-select e, "Questionable cast from abstract " + i.getName()
+  and c.isImplicitlyConvertibleTo(i)
-            + " to concrete implementation " + c.getName() + "."
+select e, "Questionable cast from abstract '" + i.getName()
+            + "' to concrete implementation '" + c.getName() + "'."

csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollectionBad.cs

@@ -0,0 +1,21 @@
+using System.Collections.Generic;
+
+class Bad
+{
+    public static void Main(string[] args)
+    {
+        var names = GetNames();
+        var list = (List<string>) names;
+        list.Add("Eve");
+    }
+
+    static IEnumerable<string> GetNames()
+    {
+        var ret = new List<string>()
+        {
+            "Alice",
+            "Bob"
+        };
+        return ret;
+    }
+}

csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.cs

@@ -1,27 +0,0 @@
-class Range
-{
-    private int[] rarray = new int[2];
-    public Range(int min, int max)
-    {
-        if (min <= max)
-        {
-            rarray[0] = min;
-            rarray[1] = max;
-        }
-    }
-    public int[] get()
-    {
-        return rarray;
-    }
-}
-class ExposeRepresentation
-{
-    public static void Main(string[] args)
-    {
-        Range a = new Range(1, 10);
-        int[] a_range = a.get();
-        a_range[0] = 500;
-        Console.WriteLine("Min: " + a.get()[0] + " Max: " + a.get()[1]);
-        // prints "Min: 500 Max: 10"
-    }
-}

csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.qhelp

@@ -19,25 +19,25 @@ fields will not be affected.</p>
 </recommendation>
 <example>
 <p>This example clearly demonstrates the problem with passing references to mutable objects outside a class. In this case 
-it was possible to modify the values in the array despite the Range class not offering any method to do so.</p>
+it was possible to modify the values in the array despite the <code>Range</code> class not offering any method to do so.</p>
-<sample src="ExposeRepresentation.cs" />
+<sample src="ExposeRepresentationBad.cs" />
 
 </example>
-<section title="Fixing With an Immutable Object">
+<section title="Fixing with an immutable object">
 <p>Here the example has been modified to prevent changes to the private field by using a <code>ReadOnlyCollection</code> 
 object.</p>
-<sample src="ExposeRepresentationFix1.cs" />
+<sample src="ExposeRepresentationGood1.cs" />
 
 </section>
-<section title="Fixing With Defensive Copying">
+<section title="Fixing with defensive copying">
 <p>This is an example of the same class but this time it returns a defensive copy of the private field. There is also 
 a short program showing what happens when an attempt is made to modify the data held by the field.</p>
-<sample src="ExposeRepresentationFix2.cs" />
+<sample src="ExposeRepresentationGood2.cs" />
 
 </section>
 <references>
 
-  <li>MSDN, C# Programming Guide, <a href="http://msdn.microsoft.com/en-us/library/vstudio/2z4khca9.aspx">Arrays as Objects</a>.</li>
+  <li>MSDN, C# Programming Guide, <a href="https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/arrays/arrays-as-objects">Arrays as Objects</a>.</li>
   <li>MSDN, <a href="http://msdn.microsoft.com/en-us/library/ms132474.aspx">ReadOnlyCollection&lt;T&gt;</a>.</li>
 
 </references>

csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.ql

@@ -1,40 +1,82 @@
 /**
- * @name Exposes internal representation
+ * @name Exposing internal representation
- * @description Finds code that may expose an object's internal representation by
+ * @description An object that accidentally exposes its internal representation may allow the
- *              incorporating reference to mutable object.
+ *              object's fields to be modified in ways that the object is not prepared to handle.
  * @kind problem
  * @problem.severity recommendation
- * @precision medium
+ * @precision high
  * @id cs/expose-implementation
  * @tags reliability
  *       external/cwe/cwe-485
  */
 import csharp
+import semmle.code.csharp.commons.Collections
+import DataFlow
 
-/* This code stores a reference to an externally mutable object into the internal
+predicate storesCollection(Callable c, Parameter p, Field f) {
-   representation of the object.
+  f.getDeclaringType() = c.getDeclaringType().getABaseType*().getSourceDeclaration() and
-   If instances are accessed by untrusted code, and unchecked changes to the mutable
+  f.getType() instanceof CollectionType and
-   object would compromise security or other important properties,
+  p = c.getAParameter() and
-   you will need to do something different. Storing a copy of the object is better
+  f.getAnAssignedValue() = p.getAnAccess() and
-   approach in many situations.
+  not c.(Modifiable).isStatic()
+}
 
-   In this analysis an object is considered mutable if it is an array, a
+predicate returnsCollection(Callable c, Field f) {
-   java.util.Hashtable, or a java.util.Date.
+  f.getDeclaringType() = c.getDeclaringType().getABaseType*().getSourceDeclaration() and
-   The analysis detects stores to fields of these types where the value is given as a
+  f.getType() instanceof CollectionType and
-   parameter. */
+  c.canReturn(f.getAnAccess()) and
+  not c.(Modifiable).isStatic()
+}
 
-from Assignment a, Field f, VariableAccess va, RefType t
+predicate mayWriteToCollection(Expr modified) {
-where a.getLValue() = va and
+  modified instanceof CollectionModificationAccess
-      va.getTarget() = f and
+  or
-      f.getType().(RefType).getSourceDeclaration() = t and
+  exists(Expr mid |
-      (   (va.(MemberAccess).hasQualifier() and
+    mayWriteToCollection(mid) |
-           va.(MemberAccess).getQualifier() instanceof ThisAccess)
+    localFlow(exprNode(modified), exprNode(mid))
-       or not va.(MemberAccess).hasQualifier()) and
+  )
-      a.getRValue().(VariableAccess).getTarget() instanceof Parameter and
+  or
-      (   t instanceof ArrayType
+  exists(MethodCall mid, Callable c |
-      //Add mutable types here as necessary. Kept the java types as a reference
+    mayWriteToCollection(mid) |
-      /*or t.hasQualifiedName("java.util", "Hashtable")
+    mid.getTarget() = c and
-       or t.hasQualifiedName("java.util", "Date")*/)
+    c.canReturn(modified)
-select a,
+  )
-       "May expose internal representation by storing an externally mutable object in "
+}
-       + f.getName() + "."
+
+predicate modificationAfter(Expr before, Expr after) {
+  mayWriteToCollection(after) and
+  localFlowStep+(exprNode(before), exprNode(after))
+}
+
+VariableAccess varPassedInto(Callable c, Parameter p) {
+  exists(Call call |
+    call.getTarget() = c |
+    call.getArgumentForParameter(p) = result
+  )
+}
+
+predicate exposesByReturn(Callable c, Field f, Expr why, string whyText) {
+  returnsCollection(c, f) and
+  exists(MethodCall ma |
+    ma.getTarget() = c |
+    mayWriteToCollection(ma) and
+    why = ma and
+    whyText = "after this call to " + c.getName()
+  )
+}
+
+predicate exposesByStore(Callable c, Field f, Expr why, string whyText) {
+  exists(VariableAccess v, Parameter p |
+    storesCollection(c, p, f) and
+    v = varPassedInto(c, p) and
+    modificationAfter(v, why) and
+    whyText = "through the variable " + v.getTarget().getName()
+  )
+}
+
+from Callable c, Field f, Expr why, string whyText
+where exposesByReturn(c, f, why, whyText) or
+      exposesByStore(c, f, why, whyText)
+select c, "'" + c.getName() + "' exposes the internal representation stored in field '" + f.getName() +
+          "'. The value may be modified $@.",
+          why.getLocation(), whyText

csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationBad.cs

@@ -0,0 +1,29 @@
+using System;
+
+class Bad
+{
+    class Range
+    {
+        private int[] rarray = new int[2];
+
+        public Range(int min, int max)
+        {
+            if (min <= max)
+            {
+                rarray[0] = min;
+                rarray[1] = max;
+            }
+        }
+
+        public int[] Get() => rarray;
+    }
+
+    public static void Main(string[] args)
+    {
+        var r = new Range(1, 10);
+        var r_range = r.Get();
+        r_range[0] = 500;
+        Console.WriteLine("Min: " + r.Get()[0] + " Max: " + r.Get()[1]);
+        // prints "Min: 500 Max: 10"
+    }
+}

csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationFix1.cs

@@ -1,20 +0,0 @@
-using System.Collections.ObjectModel;
-
-class Range
-{
-    private ReadOnlyCollection<int> rarray = new ReadOnlyCollection<int>(new int[2]);
-    public Range(int min, int max)
-    {
-        if (min <= max)
-        {
-            int[] rarray = new int[2];
-            rarray[0] = min;
-            rarray[1] = max;
-            this.rarray = new ReadOnlyCollection<int>(rarray);
-        }
-    }
-    public ReadOnlyCollection<int> get()
-    {
-        return rarray;
-    }
-}

csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationFix2.cs

@@ -1,27 +0,0 @@
-class Range
-{
-    private int[] rarray = new int[2];
-    public Range(int min, int max)
-    {
-        if (min <= max)
-        {
-            rarray[0] = min;
-            rarray[1] = max;
-        }
-    }
-    public int[] get()
-    {
-        return (int[])rarray.Clone();
-    }
-}
-class Program
-{
-    public static void Main(string[] args)
-    {
-        Range a = new Range(1, 10);
-        int[] a_range = a.get();
-        a_range[0] = 500;
-        Console.WriteLine("Min: " + a.get()[0] + " Max: " + a.get()[1]);
-        // prints "Min: 1 Max: 10"
-    }
-}

csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationGood1.cs

@@ -0,0 +1,22 @@
+using System.Collections.ObjectModel;
+
+class Good1
+{
+    class Range
+    {
+        private ReadOnlyCollection<int> rarray = new ReadOnlyCollection<int>(new int[2]);
+
+        public Range(int min, int max)
+        {
+            if (min <= max)
+            {
+                int[] rarray = new int[2];
+                rarray[0] = min;
+                rarray[1] = max;
+                this.rarray = new ReadOnlyCollection<int>(rarray);
+            }
+        }
+
+        public ReadOnlyCollection<int> Get() => rarray;
+    }
+}

csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationGood2.cs

@@ -0,0 +1,29 @@
+using System;
+
+class Good2
+{
+    class Range
+    {
+        private int[] rarray = new int[2];
+
+        public Range(int min, int max)
+        {
+            if (min <= max)
+            {
+                rarray[0] = min;
+                rarray[1] = max;
+            }
+        }
+
+        public int[] Get() => (int[])rarray.Clone();
+    }
+
+    public static void Main(string[] args)
+    {
+        Range a = new Range(1, 10);
+        int[] a_range = a.Get();
+        a_range[0] = 500;
+        Console.WriteLine("Min: " + a.Get()[0] + " Max: " + a.Get()[1]);
+        // prints "Min: 1 Max: 10"
+    }
+}

csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.cs

@@ -1,8 +0,0 @@
-class StaticArray
-{
-    public static readonly string[] Foo = { "hello", "world" }; // BAD
-    public static void Main(string[] args)
-    {
-        Foo[0] = "goodbye";
-    }
-}

csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.qhelp

@@ -3,28 +3,28 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>Arrays might be made static readonly in order to prevent their contents being changed.
+<p>Arrays might be made <code>static readonly</code> to prevent their contents from being changed.
-This is not the case because arrays are mutable. The readonly option prevents the array from being replaced 
+This doesn't have the desired effect because arrays are mutable. The <code>readonly</code> modifier prevents the array from being replaced 
-by a new array but it does not prevent the internal contents of the array from being changed.</p>
+by a new array but it does not prevent the contents of the array from being changed.</p>
 
 </overview>
 <recommendation>
 <p>Consider whether the array could be split up into separate constants. If the array cannot be split then you may 
-wish to use a ReadOnlyCollection instead of an array.</p>
+wish to use a <code>ReadOnlyCollection</code> instead of an array.</p>
 
 </recommendation>
 <example>
-<p>In this example the "Foo" array is readonly but it is still modified by the Main method.</p>
+<p>In this example the <code>Foo</code> array is <code>readonly</code> but it is still modified by the <code>Main</code> method.</p>
-<sample src="StaticArray.cs" />
+<sample src="StaticArrayBad.cs" />
 
 <p>This example uses a <code>ReadOnlyCollection</code>. Any attempt to modify <code>Foo</code> will 
 cause the program not to compile.</p>
-<sample src="StaticArrayFix.cs" />
+<sample src="StaticArrayGood.cs" />
 
 </example>
 <references>
 
-  <li>MSDN, C# Programming Guide, <a href="http://msdn.microsoft.com/en-us/library/vstudio/2z4khca9.aspx">Arrays as Objects</a>.</li>
+  <li>MSDN, C# Programming Guide, <a href="https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/arrays/arrays-as-objects">Arrays as Objects</a>.</li>
   <li>MSDN, <a href="http://msdn.microsoft.com/en-us/library/ms132474.aspx">ReadOnlyCollection&lt;T&gt;</a>.</li>
 
 </references>

csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.ql

@@ -1,10 +1,9 @@
 /**
- * @name Static array and non-empty array literal
+ * @name Array constant vulnerable to change
- * @description Finds public constants that are assigned an array.
+ * @description Array constants are mutable and can be changed by malicious code or by accident.
- *              Arrays are mutable and can be changed by malicious code or by accident.
  * @kind problem
  * @problem.severity recommendation
- * @precision high
+ * @precision medium
  * @id cs/static-array
  * @tags reliability
  *       maintainability
@@ -14,25 +13,28 @@
 import csharp
 
 predicate nonEmptyArrayLiteralOrNull(Expr e) {
-  exists(ArrayCreation arr | arr = e |
+  e = any(ArrayCreation arr |
     exists(arr.getInitializer().getAnElement())
     or
     not arr.getALengthArgument().getValue() = "0"
   )
-  or e instanceof NullLiteral
   or
-  exists(ConditionalExpr cond | cond = e |
+  e instanceof NullLiteral
+  or
+  e = any(ConditionalExpr cond |
     nonEmptyArrayLiteralOrNull(cond.getThen()) and
     nonEmptyArrayLiteralOrNull(cond.getElse())
   )
 }
 
 from Field f
-where f.isPublic() and
+where f.isPublic()
-      f.isStatic() and
+  and f.isStatic()
-      f.isReadOnly() and
+  and f.isReadOnly()
-      f.getType() instanceof ArrayType and
+  and f.getType() instanceof ArrayType
-      f.fromSource() and
+  and f.fromSource()
-      forall(AssignExpr a | a.getLValue() = f.getAnAccess() | nonEmptyArrayLiteralOrNull(a.getRValue())) and
+  and forall(AssignableDefinition def |
-      forall(Expr e | e = f.getInitializer() | nonEmptyArrayLiteralOrNull(e))
+    def.getTarget() = f |
-select f, f.getName() + " is a static array vulnerable to mutation."
+    nonEmptyArrayLiteralOrNull(def.getSource())
+  )
+select f, "The array constant '" + f.getName() + "' is vulnerable to mutation."

csharp/ql/src/Bad Practices/Implementation Hiding/StaticArrayBad.cs

@@ -0,0 +1,8 @@
+class Bad
+{
+    public static readonly string[] Foo = { "hello", "world" };
+    public static void Main(string[] args)
+    {
+        Foo[0] = "goodbye";
+    }
+}

csharp/ql/src/Bad Practices/Implementation Hiding/StaticArrayGood.cs

@@ -1,4 +1,6 @@
-class StaticArrayFix
+using System.Collections.ObjectModel;
+
+class Good
 {
     public static readonly ReadOnlyCollection<string> Foo
         = new ReadOnlyCollection<string>(new string[] { "hello", "world" });

csharp/ql/src/Likely Bugs/Collections/Collection.qll

@@ -1,159 +0,0 @@
-import csharp
-
-string modifyMethodName()
-{
-  result="Add"  or
-  result="AddFirst" or
-  result="AddLast" or
-  result="Clear" or
-  result="Enqueue" or
-  result="ExceptWith" or
-  result="Insert" or
-  result="IntersectWith" or
-  result="Push" or
-  result="Remove" or
-  result="RemoveAt" or
-  result="RemoveFirst" or
-  result="RemoveLast" or
-  result="Set" or
-  result="SetAll" or
-  result="SymmetricExceptWith" or
-  result="UnionWith"
-}
-
-class ModifierMethodCall extends MethodCall
-{
-  ModifierMethodCall()
-  {
-    getParent() instanceof ExprStmt and
-    this.getTarget().hasName(modifyMethodName())
-  }
-}
-
-string collectionTypeName()
-{
-  result="ArrayList" or
-  result="BitArray" or
-  result="Hashtable" or
-  result="ICollection" or
-  result="IDictionary" or
-  result="IList" or
-  result="Queue" or
-  result="ReadOnlyCollectionBase" or
-  result="SortedList" or
-  result="Stack"
-}
-
-string collectionNamespaceName()
-{
-  result="Mono.Collections" or
-  result="System.Collections"
-}
-
-string genericCollectionNamespaceName()
-{
-  result="Mono.Collections.Generic" or
-  result="System.Collections.Generic"
-}
-
-string genericCollectionTypeName()
-{
-  result="Dictionary<,>" or
-  result="HashSet<>" or
-  result="ICollection<>" or
-  result="IDictionary<,>" or
-  result="IList<>" or
-  result="ISet<>" or
-  result="LinkedList<>" or
-  result="List<>" or
-  result="Queue<>" or
-  result="SortedDictionary<,>" or
-  result="SortedList<,>" or
-  result="SortedSet<>" or
-  result="Stack<>" or
-  result="SynchronizedCollection<>" or
-  result="SynchronizedKeyedCollection<>" or
-  result="SynchronizedReadOnlyCollection<>"
-}
-
-predicate isCollectionType(RefType r)
-{
-  r.hasQualifiedName( collectionNamespaceName(), collectionTypeName() ) or
-  r.(ConstructedType).getUnboundGeneric().hasQualifiedName( genericCollectionNamespaceName(), genericCollectionTypeName() )
-}
-
-class EmptyCollectionCreation extends ObjectCreation
-{
-  EmptyCollectionCreation()
-  {
-    isCollectionType(getType())
-    and not hasInitializer()
-    and getNumberOfArguments()=0
-  }
-}
-
-// Methods that do not mutate a collection.
-// These methods are useless on empty collections.
-string readonlyMethodName()
-{
-  result="BinarySearch" or
-  result="Clone" or
-  result="Contains" or
-  result="ContainsKey" or
-  result="ContainsValue" or
-  result="CopyTo" or
-  result="Equals" or
-  result="FixedArray" or
-  result="FixedSize" or
-  result="Get" or
-  result="GetEnumerator" or
-  result="GetHashCode" or
-  result="GetRange" or
-  result="IndexOf" or
-  result="IsProperSubsetOf" or
-  result="IsProperSupersetOf" or
-  result="IsSubsetOf" or
-  result="IsSupersetOf" or
-  result="LastIndexOf" or
-  result="MemberwiseClone" or
-  result="Peek" or
-  result="ToArray" or
-  result="ToString" or
-  result="TryGetValue"
-}
-
-// Methods that do not modify an empty collection.
-// These methods are useless on empty collections.
-string noAddMethodName()
-{
-  result = readonlyMethodName() or
-  result="Dequeue" or
-  result="Pop"
-}
-
-// An access that does not modify the collection
-predicate readonlyAccess(Access a)
-{
-  // A read-only method call
-  exists( MethodCall mc | mc.getQualifier()=a | mc.getTarget().hasName(readonlyMethodName()) )
-
-  // Any property access
-  or exists( PropertyAccess pa | pa.getQualifier()=a )
-
-  // An indexed read
-  or exists( IndexerAccess ia | ia.getQualifier()=a | not ia.getParent().(Assignment).getLValue()=ia )
-}
-
-// An access that does not add items to a collection
-predicate noAddAccess(Access a)
-{
-  readonlyAccess(a)
-  or exists( MethodCall mc | mc.getQualifier()=a | mc.getTarget().hasName(noAddMethodName()) )
-}
-
-// An empty collection is assigned
-predicate emptyCollectionAssignment(Access a)
-{
-  exists( Assignment ass | ass.getLValue()=a | ass.getRValue() instanceof EmptyCollectionCreation )
-  or a.(LocalVariableDeclAndInitExpr).getRValue() instanceof EmptyCollectionCreation
-}

csharp/ql/src/Likely Bugs/Collections/ReadOnlyContainer.ql

@@ -12,12 +12,12 @@
  */
 
 import csharp
-import Collection
+import semmle.code.csharp.commons.Collections
 
 from Variable v
 where
   v.fromSource()
-  and isCollectionType(v.getType())
+  and v.getType() instanceof CollectionType
 
   // Publics might get assigned elsewhere
   and (v instanceof LocalVariable or v.(Field).isPrivate())
@@ -26,12 +26,12 @@ where
   and forall( AssignableDefinition d | v = d.getTarget() | d.getSource() instanceof EmptyCollectionCreation )
 
   // All accesses do not add data.
-  and forex( Access a | v.getAnAccess()=a | noAddAccess(a) or emptyCollectionAssignment(a) )
+  and forex( Access a | v.getAnAccess()=a | a instanceof NoAddAccess or a instanceof EmptyInitializationAccess)
 
   // Attributes indicate some kind of reflection
   and (not exists( Attribute a | v=a.getTarget() ))
 
   // There is at least one non-assignment access
-  and noAddAccess(v.getAnAccess())
+  and v.getAnAccess() instanceof NoAddAccess
 select v, "The contents of this container are never initialized."
 

csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql

@@ -11,10 +11,10 @@
  */
 
 import csharp
-import Collection
+import semmle.code.csharp.commons.Collections
 
 from Variable v
-where isCollectionType(v.getType())
+where v.getType() instanceof CollectionType
   and (v instanceof LocalVariable or v = any(Field f | f.isEffectivelyPrivate() or f.isEffectivelyInternal()))
   and forex(Access a |
     a = v.getAnAccess() |

csharp/ql/src/semmle/code/csharp/commons/Assertions.qll

@@ -1,4 +1,4 @@
-/** Provides classed for assertions. */
+/** Provides classes for assertions. */
 private import semmle.code.csharp.frameworks.system.Diagnostics
 private import semmle.code.csharp.frameworks.test.VisualStudio
 

csharp/ql/src/semmle/code/csharp/commons/Collections.qll

@@ -0,0 +1,174 @@
+/** Provides classes for collections. */
+import csharp
+
+private string modifyMethodName() {
+  result = "Add" or
+  result = "AddFirst" or
+  result = "AddLast" or
+  result = "Clear" or
+  result = "Enqueue" or
+  result = "ExceptWith" or
+  result = "Insert" or
+  result = "IntersectWith" or
+  result = "Push" or
+  result = "Remove" or
+  result = "RemoveAt" or
+  result = "RemoveFirst" or
+  result = "RemoveLast" or
+  result = "Set" or
+  result = "SetAll" or
+  result = "SymmetricExceptWith" or
+  result = "UnionWith"
+}
+
+/** A method call that modifies a collection. */
+class ModifierMethodCall extends MethodCall {
+  ModifierMethodCall() {
+    this.getParent() instanceof ExprStmt and
+    this.getTarget().hasName(modifyMethodName())
+  }
+}
+
+/** An access that modifies a collection. */
+class CollectionModificationAccess extends Access {
+  CollectionModificationAccess() {
+    this = any(ModifierMethodCall mc).getQualifier() or
+    this = any(ElementWrite ew).getQualifier()
+  }
+}
+
+private string collectionTypeName() {
+  result = "ArrayList" or
+  result = "BitArray" or
+  result = "Hashtable" or
+  result = "ICollection" or
+  result = "IDictionary" or
+  result = "IList" or
+  result = "Queue" or
+  result = "ReadOnlyCollectionBase" or
+  result = "SortedList" or
+  result = "Stack"
+}
+
+private string collectionNamespaceName() {
+  result = "Mono.Collections" or
+  result = "System.Collections"
+}
+
+private string genericCollectionNamespaceName() {
+  result = "Mono.Collections.Generic" or
+  result = "System.Collections.Generic"
+}
+
+private string genericCollectionTypeName() {
+  result = "Dictionary<,>" or
+  result = "HashSet<>" or
+  result = "ICollection<>" or
+  result = "IDictionary<,>" or
+  result = "IList<>" or
+  result = "ISet<>" or
+  result = "LinkedList<>" or
+  result = "List<>" or
+  result = "Queue<>" or
+  result = "SortedDictionary<,>" or
+  result = "SortedList<,>" or
+  result = "SortedSet<>" or
+  result = "Stack<>" or
+  result = "SynchronizedCollection<>" or
+  result = "SynchronizedKeyedCollection<>" or
+  result = "SynchronizedReadOnlyCollection<>"
+}
+
+/** A collection type. */
+class CollectionType extends RefType {
+  CollectionType() {
+    exists(RefType base |
+      base = this.getABaseType*() |
+      base.hasQualifiedName(collectionNamespaceName(), collectionTypeName())
+      or
+      base.(ConstructedType).getUnboundGeneric().hasQualifiedName(genericCollectionNamespaceName(), genericCollectionTypeName())
+    )
+    or
+    this instanceof ArrayType
+  }
+}
+
+/** An object creation that creates an empty collection. */
+class EmptyCollectionCreation extends ObjectCreation {
+  EmptyCollectionCreation() {
+    this.getType() instanceof CollectionType and
+    not this.hasInitializer() and
+    this.getNumberOfArguments() = 0
+  }
+}
+
+private string readonlyMethodName() {
+  result = "BinarySearch" or
+  result = "Clone" or
+  result = "Contains" or
+  result = "ContainsKey" or
+  result = "ContainsValue" or
+  result = "CopyTo" or
+  result = "Equals" or
+  result = "FixedArray" or
+  result = "FixedSize" or
+  result = "Get" or
+  result = "GetEnumerator" or
+  result = "GetHashCode" or
+  result = "GetRange" or
+  result = "IndexOf" or
+  result = "IsProperSubsetOf" or
+  result = "IsProperSupersetOf" or
+  result = "IsSubsetOf" or
+  result = "IsSupersetOf" or
+  result = "LastIndexOf" or
+  result = "MemberwiseClone" or
+  result = "Peek" or
+  result = "ToArray" or
+  result = "ToString" or
+  result = "TryGetValue"
+}
+
+private string noAddMethodName() {
+  result = readonlyMethodName() or
+  result = "Dequeue" or
+  result = "Pop"
+}
+
+/** Holds if `a` is an access that does not modify a collection. */
+private predicate readonlyAccess(Access a) {
+  // A read-only method call
+  exists(MethodCall mc |
+    mc.getQualifier() = a |
+    mc.getTarget().hasName(readonlyMethodName())
+  )
+  or
+  // Any property access
+  a = any(PropertyAccess pa).getQualifier()
+  or
+  // An element read
+  a = any(ElementRead er).getQualifier()
+}
+
+/** An access that does not add items to a collection. */
+class NoAddAccess extends Access {
+  NoAddAccess() {
+    readonlyAccess(this)
+    or
+    exists(MethodCall mc |
+      mc.getQualifier() = this |
+      mc.getTarget().hasName(noAddMethodName())
+    )
+  }
+}
+
+
+/** An access that initializes an empty collection. */
+class EmptyInitializationAccess extends Access {
+  EmptyInitializationAccess() {
+    exists(AssignableDefinition def |
+      def.getTargetAccess() = this |
+      def.getSource() instanceof EmptyCollectionCreation
+    )
+  }
+}

csharp/ql/src/semmle/code/csharp/frameworks/system/Collections.qll

@@ -77,3 +77,10 @@ class SystemCollectionsIEnumeratorInterface extends SystemCollectionsInterface {
 /** DEPRECATED. Gets the `System.Collections.IEnumerator` interface. */
 deprecated
 SystemCollectionsIEnumeratorInterface getSystemCollectionsIEnumeratorInterface() { any() }
+
+/** The `System.Collections.ICollection` interface. */
+class SystemCollectionsICollectionInterface extends SystemCollectionsInterface {
+  SystemCollectionsICollectionInterface() {
+    this.hasName("ICollection")
+  }
+}

csharp/ql/src/semmle/code/csharp/frameworks/system/collections/Generic.qll

@@ -158,3 +158,10 @@ class SystemCollectionsGenericKeyValuePairStruct extends SystemCollectionsGeneri
 /** DEPRECATED. Gets the `System.Collections.Generic.KeyValuePair<TKey, TValue>` structure. */
 deprecated
 SystemCollectionsGenericKeyValuePairStruct getSystemCollectionsGenericKeyValuePairStruct() { any() }
+
+/** The `System.Collections.Generic.ICollection<>` interface. */
+class SystemCollectionsGenericICollectionInterface extends SystemCollectionsGenericUnboundGenericInterface {
+  SystemCollectionsGenericICollectionInterface() {
+    this.hasName("ICollection<>")
+  }
+}

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.cs

@@ -0,0 +1,10 @@
+using System.Collections.Generic;
+
+class AbstractToConcreteCollection
+{
+    void M(IEnumerable<string> strings)
+    {
+        var list = (List<string>) strings; // BAD
+        var o = (object) strings; // GOOD
+    }
+}

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.expected

@@ -0,0 +1,2 @@
+| AbstractToConcreteCollection.cs:7:20:7:41 | (...) ... | Questionable cast from abstract 'IEnumerable<String>' to concrete implementation 'List<String>'. |
+| AbstractToConcreteCollectionBad.cs:8:20:8:39 | (...) ... | Questionable cast from abstract 'IEnumerable<String>' to concrete implementation 'List<String>'. |

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.qlref

@@ -0,0 +1 @@
+Bad Practices/Implementation Hiding/AbstractToConcreteCollection.ql

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollectionBad.cs

@@ -0,0 +1,21 @@
+using System.Collections.Generic;
+
+class Bad
+{
+    public static void Main(string[] args)
+    {
+        var names = GetNames();
+        var list = (List<string>) names;
+        list.Add("Eve");
+    }
+
+    static IEnumerable<string> GetNames()
+    {
+        var ret = new List<string>()
+        {
+            "Alice",
+            "Bob"
+        };
+        return ret;
+    }
+}

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.cs

@@ -0,0 +1,18 @@
+using System;
+
+class ExposeRepresentation
+{
+    class Range
+    {
+        private int[] rarray = new int[2];
+        public void Set(int[] a) { rarray = a; }
+    }
+
+    public static void Main(string[] args)
+    {
+        var a = new int[] { 0, 1 };
+        var r = new Range();
+        r.Set(a); // BAD
+        a[0] = 500;
+    }
+}

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.expected

@@ -0,0 +1,2 @@
+| ExposeRepresentation.cs:8:21:8:23 | Set | 'Set' exposes the internal representation stored in field 'rarray'. The value may be modified $@. | ExposeRepresentation.cs:16:9:16:9 | ExposeRepresentation.cs:16:9:16:9 | through the variable a |
+| ExposeRepresentationBad.cs:18:22:18:24 | Get | 'Get' exposes the internal representation stored in field 'rarray'. The value may be modified $@. | ExposeRepresentationBad.cs:24:23:24:29 | ExposeRepresentationBad.cs:24:23:24:29 | after this call to Get |

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.qlref

@@ -0,0 +1 @@
+Bad Practices/Implementation Hiding/ExposeRepresentation.ql

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationBad.cs

@@ -0,0 +1,29 @@
+using System;
+
+class Bad
+{
+    class Range
+    {
+        private int[] rarray = new int[2];
+
+        public Range(int min, int max)
+        {
+            if (min <= max)
+            {
+                rarray[0] = min;
+                rarray[1] = max;
+            }
+        }
+
+        public int[] Get() => rarray;
+    }
+
+    public static void Main(string[] args)
+    {
+        var r = new Range(1, 10);
+        var r_range = r.Get();
+        r_range[0] = 500;
+        Console.WriteLine("Min: " + r.Get()[0] + " Max: " + r.Get()[1]);
+        // prints "Min: 500 Max: 10"
+    }
+}

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationGood1.cs

@@ -0,0 +1,22 @@
+using System.Collections.ObjectModel;
+
+class Good1
+{
+    class Range
+    {
+        private ReadOnlyCollection<int> rarray = new ReadOnlyCollection<int>(new int[2]);
+
+        public Range(int min, int max)
+        {
+            if (min <= max)
+            {
+                int[] rarray = new int[2];
+                rarray[0] = min;
+                rarray[1] = max;
+                this.rarray = new ReadOnlyCollection<int>(rarray);
+            }
+        }
+
+        public ReadOnlyCollection<int> Get() => rarray;
+    }
+}

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationGood2.cs

@@ -0,0 +1,29 @@
+using System;
+
+class Good2
+{
+    class Range
+    {
+        private int[] rarray = new int[2];
+
+        public Range(int min, int max)
+        {
+            if (min <= max)
+            {
+                rarray[0] = min;
+                rarray[1] = max;
+            }
+        }
+
+        public int[] Get() => (int[])rarray.Clone();
+    }
+
+    public static void Main(string[] args)
+    {
+        Range a = new Range(1, 10);
+        int[] a_range = a.Get();
+        a_range[0] = 500;
+        Console.WriteLine("Min: " + a.Get()[0] + " Max: " + a.Get()[1]);
+        // prints "Min: 1 Max: 10"
+    }
+}

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray.expected

@@ -1,3 +0,0 @@
-| StaticArray.cs:13:34:13:47 | NonEmptyArray1 | NonEmptyArray1 is a static array vulnerable to mutation. |
-| StaticArray.cs:17:34:17:47 | NonEmptyArray3 | NonEmptyArray3 is a static array vulnerable to mutation. |
-| StaticArray.cs:19:34:19:38 | Array | Array is a static array vulnerable to mutation. |

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArray.expected

@@ -0,0 +1,4 @@
+| StaticArray.cs:13:34:13:47 | NonEmptyArray1 | The array constant 'NonEmptyArray1' is vulnerable to mutation. |
+| StaticArray.cs:17:34:17:47 | NonEmptyArray3 | The array constant 'NonEmptyArray3' is vulnerable to mutation. |
+| StaticArray.cs:19:34:19:38 | Array | The array constant 'Array' is vulnerable to mutation. |
+| StaticArrayBad.cs:3:37:3:39 | Foo | The array constant 'Foo' is vulnerable to mutation. |

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArrayBad.cs

@@ -0,0 +1,8 @@
+class Bad
+{
+    public static readonly string[] Foo = { "hello", "world" };
+    public static void Main(string[] args)
+    {
+        Foo[0] = "goodbye";
+    }
+}

csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArrayGood.cs

@@ -0,0 +1,11 @@
+using System.Collections.ObjectModel;
+
+class Good
+{
+    public static readonly ReadOnlyCollection<string> Foo
+        = new ReadOnlyCollection<string>(new string[] { "hello", "world" });
+    public static void Main(string[] args)
+    {
+
+    }
+}