Merge pull request #1200 from calumgrant/cs/icryptotransform

C#: Tidy up cs/thread-unsafe-icryptotransform-field-in-class

Author: hvitved

change-notes/1.21/analysis-csharp.md

@@ -4,7 +4,7 @@
 
 | **Query**                    | **Expected impact**    | **Change**                        |
 |------------------------------|------------------------|-----------------------------------|
-
+| Class defines a field that uses an ICryptoTransform class in a way that would be unsafe for concurrent threads (`cs/thread-unsafe-icryptotransform-field-in-class`) | Fewer false positive results | The criteria for a result has changed to include nested properties, nested fields and collections. The format of the alert message has changed to highlight the static field. |
 
 ## Changes to code extraction
 

csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.ql

@@ -1,5 +1,5 @@
 /**
- * @name Class defines a field that uses an ICryptoTransform class in a way that would be unsafe for concurrent threads.
+ * @name Class defines a field that uses an ICryptoTransform class in a way that would be unsafe for concurrent threads
  * @description The class has a field that directly or indirectly make use of a static System.Security.Cryptography.ICryptoTransform object.
  *              Using this an instance of this class in concurrent threads is dangerous as it may not only result in an error,
  *              but under some circumstances may also result in incorrect results.
@@ -13,72 +13,44 @@
  */
 
 import csharp
+import semmle.code.csharp.frameworks.system.collections.Generic
 
-class ICryptoTransform extends Class {
-  ICryptoTransform() {
-    this.getABaseType*().hasQualifiedName("System.Security.Cryptography", "ICryptoTransform")
+class UnsafeField extends Field {
+  UnsafeField() {
+    this.isStatic() and
+    not this.getAnAttribute().getType().getQualifiedName() = "System.ThreadStaticAttribute" and
+    this.getType() instanceof UsesICryptoTransform
   }
 }
 
-predicate usesICryptoTransformType(Type t) {
-  exists(ICryptoTransform ict |
-    ict = t or
-    usesICryptoTransformType(t.getAChild())
+ValueOrRefType getAnEnumeratedType(ValueOrRefType type) {
+  exists(ConstructedInterface interface |
+    interface = type.getABaseInterface*() and
+    interface.getUnboundGeneric() instanceof SystemCollectionsGenericIEnumerableTInterface
+  |
+    result = interface.getATypeArgument()
   )
 }
 
-predicate hasICryptoTransformMember(Class c) {
-  exists(Field f |
-    f = c.getAMember() and
-    (
-      exists(ICryptoTransform ict | ict = f.getType()) or
-      hasICryptoTransformMember(f.getType()) or
-      usesICryptoTransformType(f.getType())
-    )
-  )
-}
-
-predicate hasICryptoTransformStaticMemberNested(Class c) {
-  exists(Field f | f = c.getAMember() |
-    hasICryptoTransformStaticMemberNested(f.getType())
+class UsesICryptoTransform extends ValueOrRefType {
+  UsesICryptoTransform() {
+    this instanceof ICryptoTransform
     or
-    f.isStatic() and
-    hasICryptoTransformMember(f.getType()) and
-    not exists(Attribute a | a = f.getAnAttribute() |
-      a.getType().getQualifiedName() = "System.ThreadStaticAttribute"
-    )
-  )
+    this.getAField().getType() instanceof UsesICryptoTransform
+    or
+    this.getAProperty().getType() instanceof UsesICryptoTransform
+    or
+    getAnEnumeratedType(this) instanceof UsesICryptoTransform
+  }
 }
 
-predicate hasICryptoTransformStaticMember(Class c, string msg) {
-  exists(Field f |
-    f = c.getAMember*() and
-    f.isStatic() and
-    not exists(Attribute a |
-      a = f.getAnAttribute() and
-      a.getType().getQualifiedName() = "System.ThreadStaticAttribute"
-    ) and
-    (
-      exists(ICryptoTransform ict |
-        ict = f.getType() and
-        msg = "Static field " + f + " of type " + f.getType() +
-            ", implements 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads."
-      )
-      or
-      not exists(ICryptoTransform ict | ict = f.getType()) and // Avoid dup messages
-      exists(Type t | t = f.getType() |
-        usesICryptoTransformType(t) and
-        msg = "Static field " + f + " of type " + f.getType() +
-            " makes usage of 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads."
-      )
-    )
-  )
-  or
-  hasICryptoTransformStaticMemberNested(c) and
-  msg = "Class" + c +
-      " implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads."
+class ICryptoTransform extends ValueOrRefType {
+  ICryptoTransform() {
+    this.getABaseType*().hasQualifiedName("System.Security.Cryptography", "ICryptoTransform")
+  }
 }
 
-from Class c, string s
-where hasICryptoTransformStaticMember(c, s)
-select c, s
+from UnsafeField field
+select field,
+  "Static field '" + field.getName() +
+    "' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way."

csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs

@@ -54,7 +54,7 @@ public enum DigestAlgorithm
         SHA256,
     }
 
-    private static readonly Dictionary<DigestAlgorithm, HashAlgorithm> HashMap = new Dictionary<DigestAlgorithm, HashAlgorithm>
+    private static readonly IDictionary<DigestAlgorithm, HashAlgorithm> HashMap = new Dictionary<DigestAlgorithm, HashAlgorithm>
         {
             { DigestAlgorithm.SHA1, SHA1.Create() },
             { DigestAlgorithm.SHA256, SHA256.Create() },
@@ -112,3 +112,10 @@ private string ComputeHash(string password)
     }
 }
 
+public class FuncTest
+{
+    /// <summary>
+    /// Should be OK. Does not store the field.
+    /// </summary>
+    public static Func<SHA1> function;
+}

csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected

@@ -1,5 +1,5 @@
-| ThreadUnsafeICryptoTransform.cs:39:14:39:19 | Nest03 | ClassNest03 implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads. |
-| ThreadUnsafeICryptoTransform.cs:44:14:44:19 | Nest04 | ClassNest04 implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads. |
-| ThreadUnsafeICryptoTransform.cs:49:21:49:42 | StaticMemberChildUsage | Static field HashMap of type Dictionary<DigestAlgorithm,HashAlgorithm> makes usage of 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads. |
-| ThreadUnsafeICryptoTransform.cs:64:14:64:25 | StaticMember | Static field _sha1 of type SHA1, implements 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads. |
-| ThreadUnsafeICryptoTransform.cs:69:14:69:28 | IndirectStatic2 | ClassIndirectStatic2 implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads. |
+| ThreadUnsafeICryptoTransform.cs:41:36:41:37 | _n | Static field '_n' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way. |
+| ThreadUnsafeICryptoTransform.cs:46:26:46:30 | _list | Static field '_list' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way. |
+| ThreadUnsafeICryptoTransform.cs:57:73:57:79 | HashMap | Static field 'HashMap' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way. |
+| ThreadUnsafeICryptoTransform.cs:66:25:66:29 | _sha1 | Static field '_sha1' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way. |
+| ThreadUnsafeICryptoTransform.cs:71:19:71:20 | _n | Static field '_n' contains a 'System.Security.Cryptography.ICryptoTransform' that could be used in an unsafe way. |