CodeQL queries 1.25
Skip to end of metadata
Go to start of metadata

Name: Thread-unsafe capturing of an ICryptoTransform object

Description: An instance of a class that either implements or has a field of type System.Security.Cryptography.ICryptoTransform is being captured by a lambda, and used in what seems to be a thread initialization method. Using 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.

ID: cs/thread-unsafe-icryptotransform-captured-in-lambda

Kind: problem

Severity: warning

Precision: medium

Query: ThreadUnsafeICryptoTransformLambda.ql
/**
 * @name Thread-unsafe capturing of an ICryptoTransform object
 * @description An instance of a class that either implements or has a field of type System.Security.Cryptography.ICryptoTransform is being captured by a lambda,
 *              and used in what seems to be a thread initialization method.
 *              Using 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-captured-in-lambda
 * @tags concurrency
 *       security
 *       external/cwe/cwe-362
 */

import csharp
import semmle.code.csharp.dataflow.DataFlow
import ParallelSink
import ICryptoTransform

class NotThreadSafeCryptoUsageIntoParallelInvokeConfig extends TaintTracking::Configuration {
  NotThreadSafeCryptoUsageIntoParallelInvokeConfig() {
    this = "NotThreadSafeCryptoUsageIntoParallelInvokeConfig"
  }

  override predicate isSource(DataFlow::Node source) {
    source instanceof LambdaCapturingICryptoTransformSource
  }

  override predicate isSink(DataFlow::Node sink) { sink instanceof ParallelSink }
}

from Expr e, string m, LambdaExpr l, NotThreadSafeCryptoUsageIntoParallelInvokeConfig config
where
  config.hasFlow(DataFlow::exprNode(l), DataFlow::exprNode(e)) and
  m =
    "A $@ seems to be used to start a new thread is capturing a local variable that either implements 'System.Security.Cryptography.ICryptoTransform' or has a field of this type."
select e, m, l, "lambda expression"

Classes that implement System.Security.Cryptography.ICryptoTransform are not thread safe.

This problem is caused by the way these classes are implemented using Microsoft CAPI/CNG patterns.

For example, when a hash class implements this interface, there would typically be an instance-specific hash object created (for example using BCryptCreateHash function). This object can be called multiple times to add data to the hash (for example BCryptHashData). Finally, a function is called that finishes the hash and returns the data (for example BCryptFinishHash).

Allowing the same hash object to be called with data from multiple threads before calling the finish function could potentially lead to incorrect results.

For example, if you have multiple threads hashing "abc" on a static hash object, you may occasionally obtain the results (incorrectly) for hashing "abcabc", or face other unexpected behavior.

It is very unlikely somebody outside Microsoft would write a class that implements ICryptoTransform, and even if they do, it is likely that they will follow the same common pattern as the existing classes implementing this interface.

Any object that implements System.Security.Cryptography.ICryptoTransform should not be used in concurrent threads as the instance members of such object are also not thread safe.

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.

Recommendation

Create new instances of the object that implements or has a field of type System.Security.Cryptography.ICryptoTransform to avoid sharing it accross multiple threads.

Example

This example demonstrates the dangers of using a shared System.Security.Cryptography.ICryptoTransform in a way that generates incorrect results or may raise an exception.

public static void RunThreadUnSafeICryptoTransformLambdaBad()
{
    const int threadCount = 4;
    // This local variable for a hash object is going to be shared across multiple threads
    var sha1 = SHA1.Create();
    var b = new Barrier(threadCount);
    Action start = () => {
        b.SignalAndWait();
        for (int i = 0; i < 1000; i++)
        {
            var pwd = Guid.NewGuid().ToString();
            var bytes = Encoding.UTF8.GetBytes(pwd);
            // This call may fail, or return incorrect results
            sha1.ComputeHash(bytes);
        }
    };
    var threads = Enumerable.Range(0, threadCount)
                            .Select(_ => new ThreadStart(start))
                            .Select(x => new Thread(x))
                            .ToList();
    foreach (var t in threads) t.Start();
    foreach (var t in threads) t.Join();
}

A simple fix is to change the local variable sha1 being captured by the lambda to be a local variable within the lambda.

public static void RunThreadUnSafeICryptoTransformLambdaFixed()
{
    const int threadCount = 4;
    var b = new Barrier(threadCount);
    Action start = () => {
        b.SignalAndWait();
        // The hash object is no longer shared
        for (int i = 0; i < 1000; i++)
        {
            var sha1 = SHA1.Create();
            var pwd = Guid.NewGuid().ToString();
            var bytes = Encoding.UTF8.GetBytes(pwd);
            sha1.ComputeHash(bytes);
        }
    };
    var threads = Enumerable.Range(0, threadCount)
                            .Select(_ => new ThreadStart(start))
                            .Select(x => new Thread(x))
                            .ToList();
    foreach (var t in threads) t.Start();
    foreach (var t in threads) t.Join();
}

References