From d05109df76361153e2b6d6cff02d74abc3a7f305 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Thu, 2 Aug 2018 16:26:29 +0200 Subject: [PATCH] C#: Update queries in `Bad Practices/Implementation Hiding` --- change-notes/1.18/analysis-csharp.md | 1 + .../AbstractToConcreteCollection.cs | 10 - .../AbstractToConcreteCollection.qhelp | 4 +- .../AbstractToConcreteCollection.ql | 29 +-- .../AbstractToConcreteCollectionBad.cs | 21 +++ .../ExposeRepresentation.cs | 27 --- .../ExposeRepresentation.qhelp | 14 +- .../ExposeRepresentation.ql | 100 +++++++--- .../ExposeRepresentationBad.cs | 29 +++ .../ExposeRepresentationFix1.cs | 20 -- .../ExposeRepresentationFix2.cs | 27 --- .../ExposeRepresentationGood1.cs | 22 +++ .../ExposeRepresentationGood2.cs | 29 +++ .../Implementation Hiding/StaticArray.cs | 8 - .../Implementation Hiding/StaticArray.qhelp | 16 +- .../Implementation Hiding/StaticArray.ql | 32 ++-- .../Implementation Hiding/StaticArrayBad.cs | 8 + .../{StaticArrayFix.cs => StaticArrayGood.cs} | 4 +- .../Likely Bugs/Collections/Collection.qll | 159 ---------------- .../Collections/ReadOnlyContainer.ql | 8 +- .../Collections/WriteOnlyContainer.ql | 4 +- .../semmle/code/csharp/commons/Assertions.qll | 2 +- .../code/csharp/commons/Collections.qll | 174 ++++++++++++++++++ .../csharp/frameworks/system/Collections.qll | 7 + .../frameworks/system/collections/Generic.qll | 7 + .../AbstractToConcreteCollection.cs | 10 + .../AbstractToConcreteCollection.expected | 2 + .../AbstractToConcreteCollection.qlref | 1 + .../AbstractToConcreteCollectionBad.cs | 21 +++ .../ExposeRepresentation.cs | 18 ++ .../ExposeRepresentation.expected | 2 + .../ExposeRepresentation.qlref | 1 + .../ExposeRepresentationBad.cs | 29 +++ .../ExposeRepresentationGood1.cs | 22 +++ .../ExposeRepresentationGood2.cs | 29 +++ .../StaticArray.expected | 3 - .../{ => StaticArray}/StaticArray.cs | 0 .../StaticArray/StaticArray.expected | 4 + .../{ => StaticArray}/StaticArray.qlref | 0 .../StaticArray/StaticArrayBad.cs | 8 + .../StaticArray/StaticArrayGood.cs | 11 ++ 41 files changed, 588 insertions(+), 335 deletions(-) delete mode 100644 csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.cs create mode 100644 csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollectionBad.cs delete mode 100644 csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.cs create mode 100644 csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationBad.cs delete mode 100644 csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationFix1.cs delete mode 100644 csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationFix2.cs create mode 100644 csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationGood1.cs create mode 100644 csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationGood2.cs delete mode 100644 csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.cs create mode 100644 csharp/ql/src/Bad Practices/Implementation Hiding/StaticArrayBad.cs rename csharp/ql/src/Bad Practices/Implementation Hiding/{StaticArrayFix.cs => StaticArrayGood.cs} (79%) delete mode 100644 csharp/ql/src/Likely Bugs/Collections/Collection.qll create mode 100644 csharp/ql/src/semmle/code/csharp/commons/Collections.qll create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.cs create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.expected create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.qlref create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollectionBad.cs create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.cs create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.expected create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.qlref create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationBad.cs create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationGood1.cs create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationGood2.cs delete mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray.expected rename csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/{ => StaticArray}/StaticArray.cs (100%) create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArray.expected rename csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/{ => StaticArray}/StaticArray.qlref (100%) create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArrayBad.cs create mode 100644 csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArrayGood.cs diff --git a/change-notes/1.18/analysis-csharp.md b/change-notes/1.18/analysis-csharp.md index 8ae26bd493e..c6cb7daeace 100644 --- a/change-notes/1.18/analysis-csharp.md +++ b/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 diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.cs b/csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.cs deleted file mode 100644 index fee350bd719..00000000000 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.cs +++ /dev/null @@ -1,10 +0,0 @@ -class AbstractToConcreteCollection -{ - public static void Main(string[] args) - { - ICollection foo = new List(); - foo.Add("hello"); - foo.Add("world"); - List bar = (List)foo; // BAD - } -} diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.qhelp b/csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.qhelp index e1206fed3b7..bac228017c8 100644 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.qhelp +++ b/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.

-

The example shows casting from an ICollection to a List. This should be avoided where possible.

- +

The example shows casting from an IEnumerable<string> to a List<string>. This should be avoided where possible.

+ diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.ql b/csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.ql index 192f67f8a3a..7639863fdd7 100644 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollection.ql +++ b/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() + "'." diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollectionBad.cs b/csharp/ql/src/Bad Practices/Implementation Hiding/AbstractToConcreteCollectionBad.cs new file mode 100644 index 00000000000..9538fb9d7a0 --- /dev/null +++ b/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) names; + list.Add("Eve"); + } + + static IEnumerable GetNames() + { + var ret = new List() + { + "Alice", + "Bob" + }; + return ret; + } +} diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.cs b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.cs deleted file mode 100644 index 31063beacc7..00000000000 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.cs +++ /dev/null @@ -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" - } -} diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.qhelp b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.qhelp index 7fa45098469..ef835994f50 100644 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.qhelp +++ b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.qhelp @@ -19,25 +19,25 @@ fields will not be affected.

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.

- +it was possible to modify the values in the array despite the Range class not offering any method to do so.

+ -
+

Here the example has been modified to prevent changes to the private field by using a ReadOnlyCollection object.

- +
-
+

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.

- +
-
  • MSDN, C# Programming Guide, Arrays as Objects.
    • +
  • MSDN, C# Programming Guide, Arrays as Objects.
  • MSDN, ReadOnlyCollection<T>.
    •  diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.ql b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.ql index e6e1c9e1c1e..b46189aa587 100644 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.ql +++ b/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. +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() +} - 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. */ +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() +} -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 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 diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationBad.cs b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationBad.cs new file mode 100644 index 00000000000..221f906aa97 --- /dev/null +++ b/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" + } +} diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationFix1.cs b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationFix1.cs deleted file mode 100644 index 886bf5828a9..00000000000 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationFix1.cs +++ /dev/null @@ -1,20 +0,0 @@ -using System.Collections.ObjectModel; - -class Range -{ - private ReadOnlyCollection rarray = new ReadOnlyCollection(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(rarray); - } - } - public ReadOnlyCollection get() - { - return rarray; - } -} diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationFix2.cs b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationFix2.cs deleted file mode 100644 index 6d59f5312e0..00000000000 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationFix2.cs +++ /dev/null @@ -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" - } -} diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationGood1.cs b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationGood1.cs new file mode 100644 index 00000000000..01302a24ce4 --- /dev/null +++ b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationGood1.cs @@ -0,0 +1,22 @@ +using System.Collections.ObjectModel; + +class Good1 +{ + class Range + { + private ReadOnlyCollection rarray = new ReadOnlyCollection(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(rarray); + } + } + + public ReadOnlyCollection Get() => rarray; + } +} diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationGood2.cs b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentationGood2.cs new file mode 100644 index 00000000000..d703675c56d --- /dev/null +++ b/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" + } +} diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.cs b/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.cs deleted file mode 100644 index b47ae61a37d..00000000000 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.cs +++ /dev/null @@ -1,8 +0,0 @@ -class StaticArray -{ - public static readonly string[] Foo = { "hello", "world" }; // BAD - public static void Main(string[] args) - { - Foo[0] = "goodbye"; - } -} diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.qhelp b/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.qhelp index efef6e888ab..52360eb8ed9 100644 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.qhelp +++ b/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.qhelp @@ -3,28 +3,28 @@ "qhelp.dtd"> -

    Arrays might be made static readonly in order to prevent their contents being changed. -This is not the case because arrays are mutable. The readonly option prevents the array from being replaced -by a new array but it does not prevent the internal contents of the array from being changed.

    +

    Arrays might be made static readonly to prevent their contents from being changed. +This doesn't have the desired effect because arrays are mutable. The readonly modifier prevents the array from being replaced +by a new array but it does not prevent the contents of the array from being changed.

    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.

    +wish to use a ReadOnlyCollection instead of an array.

     -

    In this example the "Foo" array is readonly but it is still modified by the Main method.

    - +

    In this example the Foo array is readonly but it is still modified by the Main method.

    +

    This example uses a ReadOnlyCollection. Any attempt to modify Foo will cause the program not to compile.

    - +     -
  • MSDN, C# Programming Guide, Arrays as Objects.
    • +
  • MSDN, C# Programming Guide, Arrays as Objects.
  • MSDN, ReadOnlyCollection<T>.
    •  diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.ql b/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.ql index 9d1d887b272..29a60defcdc 100644 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.ql +++ b/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArray.ql @@ -1,10 +1,9 @@ /** - * @name Static array and non-empty array literal - * @description Finds public constants that are assigned an array. - * Arrays are mutable and can be changed by malicious code or by accident. + * @name Array constant vulnerable to change + * @description Array constants 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 - f.isStatic() and - f.isReadOnly() and - f.getType() instanceof ArrayType and - f.fromSource() and - forall(AssignExpr a | a.getLValue() = f.getAnAccess() | nonEmptyArrayLiteralOrNull(a.getRValue())) and - forall(Expr e | e = f.getInitializer() | nonEmptyArrayLiteralOrNull(e)) -select f, f.getName() + " is a static array vulnerable to mutation." +where f.isPublic() + and f.isStatic() + and f.isReadOnly() + and f.getType() instanceof ArrayType + and f.fromSource() + and forall(AssignableDefinition def | + def.getTarget() = f | + nonEmptyArrayLiteralOrNull(def.getSource()) + ) +select f, "The array constant '" + f.getName() + "' is vulnerable to mutation." diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArrayBad.cs b/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArrayBad.cs new file mode 100644 index 00000000000..fe035d4e4f1 --- /dev/null +++ b/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"; + } +} diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArrayFix.cs b/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArrayGood.cs similarity index 79% rename from csharp/ql/src/Bad Practices/Implementation Hiding/StaticArrayFix.cs rename to csharp/ql/src/Bad Practices/Implementation Hiding/StaticArrayGood.cs index 7c75841615e..cfc72a883bb 100644 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/StaticArrayFix.cs +++ b/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 Foo = new ReadOnlyCollection(new string[] { "hello", "world" }); diff --git a/csharp/ql/src/Likely Bugs/Collections/Collection.qll b/csharp/ql/src/Likely Bugs/Collections/Collection.qll deleted file mode 100644 index 62af956f83c..00000000000 --- a/csharp/ql/src/Likely Bugs/Collections/Collection.qll +++ /dev/null @@ -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 -} diff --git a/csharp/ql/src/Likely Bugs/Collections/ReadOnlyContainer.ql b/csharp/ql/src/Likely Bugs/Collections/ReadOnlyContainer.ql index 358c5102ecf..84f452def43 100644 --- a/csharp/ql/src/Likely Bugs/Collections/ReadOnlyContainer.ql +++ b/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." diff --git a/csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql b/csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql index 0bb7b33aa31..5fab438fbb4 100644 --- a/csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql +++ b/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() | diff --git a/csharp/ql/src/semmle/code/csharp/commons/Assertions.qll b/csharp/ql/src/semmle/code/csharp/commons/Assertions.qll index a995e7ccd9d..4e85a7f1356 100644 --- a/csharp/ql/src/semmle/code/csharp/commons/Assertions.qll +++ b/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 diff --git a/csharp/ql/src/semmle/code/csharp/commons/Collections.qll b/csharp/ql/src/semmle/code/csharp/commons/Collections.qll new file mode 100644 index 00000000000..ceabeb4f360 --- /dev/null +++ b/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 + ) + } +} diff --git a/csharp/ql/src/semmle/code/csharp/frameworks/system/Collections.qll b/csharp/ql/src/semmle/code/csharp/frameworks/system/Collections.qll index a797284cf45..76e797bbeeb 100644 --- a/csharp/ql/src/semmle/code/csharp/frameworks/system/Collections.qll +++ b/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") + } +} diff --git a/csharp/ql/src/semmle/code/csharp/frameworks/system/collections/Generic.qll b/csharp/ql/src/semmle/code/csharp/frameworks/system/collections/Generic.qll index b6147791c97..9179a2b58fe 100644 --- a/csharp/ql/src/semmle/code/csharp/frameworks/system/collections/Generic.qll +++ b/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` structure. */ deprecated SystemCollectionsGenericKeyValuePairStruct getSystemCollectionsGenericKeyValuePairStruct() { any() } + +/** The `System.Collections.Generic.ICollection<>` interface. */ +class SystemCollectionsGenericICollectionInterface extends SystemCollectionsGenericUnboundGenericInterface { + SystemCollectionsGenericICollectionInterface() { + this.hasName("ICollection<>") + } +} diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.cs b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.cs new file mode 100644 index 00000000000..795952a8205 --- /dev/null +++ b/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 strings) + { + var list = (List) strings; // BAD + var o = (object) strings; // GOOD + } +} diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.expected b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.expected new file mode 100644 index 00000000000..a82dcd8187d --- /dev/null +++ b/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' to concrete implementation 'List'. | +| AbstractToConcreteCollectionBad.cs:8:20:8:39 | (...) ... | Questionable cast from abstract 'IEnumerable' to concrete implementation 'List'. | diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.qlref b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.qlref new file mode 100644 index 00000000000..307c259dbbb --- /dev/null +++ b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollection.qlref @@ -0,0 +1 @@ +Bad Practices/Implementation Hiding/AbstractToConcreteCollection.ql \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollectionBad.cs b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/AbstractToConcreteCollection/AbstractToConcreteCollectionBad.cs new file mode 100644 index 00000000000..9538fb9d7a0 --- /dev/null +++ b/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) names; + list.Add("Eve"); + } + + static IEnumerable GetNames() + { + var ret = new List() + { + "Alice", + "Bob" + }; + return ret; + } +} diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.cs b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.cs new file mode 100644 index 00000000000..01b05a00027 --- /dev/null +++ b/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; + } +} diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.expected b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.expected new file mode 100644 index 00000000000..92f7365adeb --- /dev/null +++ b/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 | diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.qlref b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.qlref new file mode 100644 index 00000000000..e8bd17759d4 --- /dev/null +++ b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentation.qlref @@ -0,0 +1 @@ +Bad Practices/Implementation Hiding/ExposeRepresentation.ql \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationBad.cs b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationBad.cs new file mode 100644 index 00000000000..221f906aa97 --- /dev/null +++ b/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" + } +} diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationGood1.cs b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationGood1.cs new file mode 100644 index 00000000000..01302a24ce4 --- /dev/null +++ b/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 rarray = new ReadOnlyCollection(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(rarray); + } + } + + public ReadOnlyCollection Get() => rarray; + } +} diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationGood2.cs b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/ExposeRepresentation/ExposeRepresentationGood2.cs new file mode 100644 index 00000000000..d703675c56d --- /dev/null +++ b/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" + } +} diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray.expected b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray.expected deleted file mode 100644 index d51b44757b9..00000000000 --- a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray.expected +++ /dev/null @@ -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. | diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray.cs b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArray.cs similarity index 100% rename from csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray.cs rename to csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArray.cs diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArray.expected b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArray.expected new file mode 100644 index 00000000000..be3e97fc61d --- /dev/null +++ b/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. | diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray.qlref b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArray.qlref similarity index 100% rename from csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray.qlref rename to csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArray.qlref diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArrayBad.cs b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArrayBad.cs new file mode 100644 index 00000000000..fe035d4e4f1 --- /dev/null +++ b/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"; + } +} diff --git a/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArrayGood.cs b/csharp/ql/test/query-tests/Bad Practices/Implementation Hiding/StaticArray/StaticArrayGood.cs new file mode 100644 index 00000000000..cfc72a883bb --- /dev/null +++ b/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 Foo + = new ReadOnlyCollection(new string[] { "hello", "world" }); + public static void Main(string[] args) + { + + } +}