Detect usage of ICryptoTransform that would be thread-unsafe

csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformBad.cs

@@ -0,0 +1,38 @@
+internal class TokenCacheThreadUnsafeICryptoTransformDemo
+{
+    private static SHA256 _sha = SHA256.Create();
+
+    public string ComputeHash(string data)
+    {
+        byte[] passwordBytes = UTF8Encoding.UTF8.GetBytes(data);
+        return Convert.ToBase64String(_sha.ComputeHash(passwordBytes));
+    }
+}
+
+class Program
+{
+    static void Main(string[] args)
+    {
+        int max = 1000;
+        Task[] tasks = new Task[max];
+
+        Action<object> action = (object obj) =>
+        {
+            var unsafeObj = new TokenCacheThreadUnsafeICryptoTransformDemo();
+            if (unsafeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=")
+            {
+                Console.WriteLine("**** We got incorrect Results!!! ****");
+            }
+        };
+
+        for (int i = 0; i < max; i++)
+        {
+            // hash calculated on all threads should be the same:
+            // ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0= (base64)
+            // 
+            tasks[i] = Task.Factory.StartNew(action, "abc");
+        }
+
+        Task.WaitAll(tasks);
+    }
+}

csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformGood.cs

@@ -0,0 +1,41 @@
+internal class TokenCacheThreadUnsafeICryptoTransformDemoFixed
+{
+    // We are replacing the static SHA256 field with an instance one
+    //
+    //private static SHA256 _sha = SHA256.Create();
+    private SHA256 _sha = SHA256.Create();
+
+    public string ComputeHash(string data)
+    {
+        byte[] passwordBytes = UTF8Encoding.UTF8.GetBytes(data);
+        return Convert.ToBase64String(_sha.ComputeHash(passwordBytes));
+    }
+}
+
+class Program
+{
+    static void Main(string[] args)
+    {
+        int max = 1000;
+        Task[] tasks = new Task[max];
+
+        Action<object> action = (object obj) =>
+        {
+            var safeObj = new TokenCacheThreadUnsafeICryptoTransformDemoFixed();
+            if (safeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=")
+            {
+                Console.WriteLine("**** We got incorrect Results!!! ****");
+            }
+        };
+
+        for (int i = 0; i < max; i++)
+        {
+            // hash calculated on all threads should be the same:
+            // ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0= (base64)
+            // 
+            tasks[i] = Task.Factory.StartNew(action, "abc");
+        }
+
+        Task.WaitAll(tasks);
+    }
+}

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

@@ -0,0 +1,38 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Classes that implement <code>System.Security.Cryptography.ICryptoTransform</code> are not thread safe.</p>
+<p>This problem is caused by the way these classes are implemented using Microsoft CAPI/CNG patterns.</p>
+<p>For example, when a hash class implements this interface, there would typically be an instance-specific hash object created (for example using <code>BCryptCreateHash</code> function). This object can be called multiple times to add data to the hash (for example <code>BCryptHashData</code>). Finally, a function is called that finishes the hash and returns the data (for example <code>BCryptFinishHash</code>).</p>
+<p>Allowing the same hash object to be called with data from multiple threads before calling the finish function could potentially lead to incorrect results.</p>
+<p>For example, if you have multiple threads hashing <code>"abc"</code> on a static hash object, you may occasionally obtain the results (incorrectly) for hashing <code>"abcabc"</code>, or face other unexpected behavior.</p>
+<p>It is very unlikely somebody outside Microsoft would write a class that implements <code>ICryptoTransform</code>, and even if they do, it is likely that they will follow the same common pattern as the existing classes implementing this interface.</p>
+<p>Any object that implements <code>System.Security.Cryptography.ICryptoTransform</code> should not be used in concurrent threads as the instance members of such object are also not thread safe.</p>
+<p>Potential problems may not be evident at first, but can range from explicit errors such as exceptions, to incorrect results when sharing an instance of such an object in multiple threads.</p>
+
+</overview>
+<recommendation>
+<p>If the object is shared across instances, you should consider changing the code to use a non-static object of type <code>System.Security.Cryptography.ICryptoTransform</code> instead.</p>
+<p>As an alternative, you could also look into using <code>ThreadStatic</code> attribute, but make sure you read the initialization remarks on the documentation.</p>
+
+</recommendation>
+<example>
+<p>This example demonstrates the dangers of using a static <code>System.Security.Cryptography.ICryptoTransform</code> in a way that generates incorrect results.</p>
+<sample src="ThreadUnSafeICryptoTransformBad.cs" />
+
+<p>A simple fix is to change the <code>_sha</code> field from being a static member to an instance one by removing the <code>static</code> keyword.</p>
+<sample src="ThreadUnSafeICryptoTransformGood.cs" />
+</example>
+
+<references>
+  <li>
+    Microsoft documentation, <a href="https://docs.microsoft.com/en-us/dotnet/api/system.threadstaticattribute?view=netframework-4.7.2">ThreadStaticAttribute Class</a>.
+  </li>
+  <li>
+    Stack Overflow, <a href="https://stackoverflow.com/questions/26592596/why-does-sha1-computehash-fail-under-high-load-with-many-threads">Why does SHA1.ComputeHash fail under high load with many threads?</a>.
+  </li>
+</references>
+
+</qhelp>

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

@@ -0,0 +1,85 @@
+/**
+ * @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.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id cs/thread-unsafe-icryptotransform-field-in-class
+ * @tags concurrency
+ *       security
+ *       external/cwe/cwe-362
+ */
+
+import csharp
+
+class ICryptoTransform extends Class {
+  ICryptoTransform() {
+    this.getABaseType*().hasQualifiedName("System.Security.Cryptography", "ICryptoTransform")
+  }
+}
+
+predicate usesICryptoTransformType( Type t ) {
+  exists(  ICryptoTransform ict |
+    ict = t
+    or usesICryptoTransformType( t.getAChild() )
+  )
+}
+
+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() )
+    or (
+      f.isStatic() and hasICryptoTransformMember(f.getType())
+      and not exists( Attribute a
+        | a = f.getAnAttribute() | 
+        a.getType().getQualifiedName() = "System.ThreadStaticAttribute"
+      )
+    )
+  )
+}
+
+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() ) // Avoid dup messages
+        and 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."
+  )
+}
+
+from Class c , string s
+  where hasICryptoTransformStaticMember(c, s)
+select c, s

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

@@ -0,0 +1,114 @@
+// semmle-extractor-options: /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll /r:System.Security.Cryptography.Primitives.dll
+using System;
+using System.Collections.Generic;
+using System.Security.Cryptography;
+
+public class Nest01
+{
+    private readonly SHA256 _sha;
+
+    public Nest01()
+    {
+        _sha = SHA256.Create();
+    }
+}
+
+public class Nest02
+{
+    private readonly Nest01 _n;
+
+    public Nest02()
+    {
+        _n = new Nest01();
+    }
+}
+
+public class ListNonStatic
+{
+    private List<SHA512> _shaList;
+
+    public ListNonStatic()
+    {
+        _shaList = new List<SHA512>();
+    }
+}
+
+/// <summary>
+/// Positive results (classes are not thread safe)
+/// </summary>
+public class Nest03
+{
+    private static readonly Nest01 _n = new Nest01();
+}
+
+public class Nest04
+{
+    static ListNonStatic _list = new ListNonStatic();
+}
+
+public static class StaticMemberChildUsage
+{
+    public enum DigestAlgorithm
+    {
+        SHA1,
+        SHA256,
+    }
+
+    private static readonly Dictionary<DigestAlgorithm, HashAlgorithm> HashMap = new Dictionary<DigestAlgorithm, HashAlgorithm>
+        {
+            { DigestAlgorithm.SHA1, SHA1.Create() },
+            { DigestAlgorithm.SHA256, SHA256.Create() },
+        };
+}
+
+public class StaticMember
+{
+    private static SHA1 _sha1 = SHA1.Create();
+}
+
+public class IndirectStatic2
+{
+    static Nest02 _n = new Nest02();
+}
+
+/// <summary>
+/// Should not be flagged (thread safe)
+/// </summary>
+
+public class IndirectStatic
+{
+    StaticMember tc;
+}
+
+public class TokenCacheFP
+{
+    /// <summary>
+    ///  Should be OK. Not shared between threads
+    /// </summary>
+    [ThreadStatic]
+    private static SHA1 _sha1 = SHA1.Create();
+
+    private string ComputeHash(string password)
+    {
+        return password;
+    }
+}
+
+public class TokenCacheNonStat
+{
+    /// <summary>
+    ///  Should be OK. Not shared between threads
+    /// </summary>
+    private SHA1 _sha1;
+
+    public TokenCacheNonStat()
+    {
+        _sha1 = SHA1.Create();
+    }
+
+    private string ComputeHash(string password)
+    {
+        return password;
+    }
+}
+

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

@@ -0,0 +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. |

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

@@ -0,0 +1 @@
+Likely Bugs/ThreadUnsafeICryptoTransform.ql