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.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>
-<sample src="AbstractToConcreteCollection.cs" />
+<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="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
- *              type. This makes the code brittle; it is best to program against the
- *              abstract collection interfaces only.
+ * @description A cast from an abstract collection to a concrete implementation type
+ *              makes the code brittle; it is best to program against the abstract
+ *              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 |
-           this.getSourceDeclaration().getABaseType*().getName() = name and
-           (   name.matches("ICollection<%>")
-            or name="ICollection")
+    exists(Interface i |
+      i = this.getABaseInterface*() |
+      i instanceof SystemCollectionsICollectionInterface or
+      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
-      e.getExpr().getType().(RefType).getSourceDeclaration() = i
-select e, "Questionable cast from abstract " + i.getName()
-            + " to concrete implementation " + c.getName() + "."
+where e.getType() = c
+  and e.getExpr().getType() = i
+  and c.isImplicitlyConvertibleTo(i)
+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.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>
-<sample src="ExposeRepresentation.cs" />
+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="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
- * @description Finds code that may expose an object's internal representation by
- *              incorporating reference to mutable object.
+ * @name Exposing internal representation
+ * @description An object that accidentally exposes its internal representation may allow the
+ *              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
-   representation of the object.
-   If instances are accessed by untrusted code, and unchecked changes to the mutable
-   object would compromise security or other important properties,
-   you will need to do something different. Storing a copy of the object is better
-   approach in many situations.
-
-   In this analysis an object is considered mutable if it is an array, a
-   java.util.Hashtable, or a java.util.Date.
-   The analysis detects stores to fields of these types where the value is given as a
-   parameter. */
-
-from Assignment a, Field f, VariableAccess va, RefType t
-where a.getLValue() = va and
-      va.getTarget() = f and
-      f.getType().(RefType).getSourceDeclaration() = t and
-      (   (va.(MemberAccess).hasQualifier() and
-           va.(MemberAccess).getQualifier() instanceof ThisAccess)
-       or not va.(MemberAccess).hasQualifier()) and
-      a.getRValue().(VariableAccess).getTarget() instanceof Parameter and
-      (   t instanceof ArrayType
-      //Add mutable types here as necessary. Kept the java types as a reference
-      /*or t.hasQualifiedName("java.util", "Hashtable")
-       or t.hasQualifiedName("java.util", "Date")*/)
-select a,
-       "May expose internal representation by storing an externally mutable object in "
-       + f.getName() + "."
+predicate storesCollection(Callable c, Parameter p, Field f) {
+  f.getDeclaringType() = c.getDeclaringType().getABaseType*().getSourceDeclaration() and
+  f.getType() instanceof CollectionType and
+  p = c.getAParameter() and
+  f.getAnAssignedValue() = p.getAnAccess() and
+  not c.(Modifiable).isStatic()
+}
+
+predicate returnsCollection(Callable c, Field f) {
+  f.getDeclaringType() = c.getDeclaringType().getABaseType*().getSourceDeclaration() and
+  f.getType() instanceof CollectionType and
+  c.canReturn(f.getAnAccess()) and
+  not c.(Modifiable).isStatic()
+}
+
+predicate mayWriteToCollection(Expr modified) {
+  modified instanceof CollectionModificationAccess
+  or
+  exists(Expr mid |
+    mayWriteToCollection(mid) |
+    localFlow(exprNode(modified), exprNode(mid))
+  )
+  or
+  exists(MethodCall mid, Callable c |
+    mayWriteToCollection(mid) |
+    mid.getTarget() = c and
+    c.canReturn(modified)
+  )
+}
+
+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/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;
+    }
+}