mirror of
https://github.com/github/codeql.git
synced 2026-05-04 21:25:44 +02:00
2n part of ICryptoTransform.
Detecting potential unsafe usage (object shared across multiple threads) on variables captured by Lambda
This commit is contained in:
Raul Garcia
@@ -0,0 +1,23 @@
|
||||
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();
|
||||
}
|
||||
@@ -0,0 +1,22 @@
|
||||
public static void RunThreadUnSafeICryptoTransformLambdaFixed()
|
||||
{
|
||||
const int threadCount = 4;
|
||||
var b = new Barrier(threadCount);
|
||||
Action start = () => {
|
||||
b.SignalAndWait();
|
||||
for (int i = 0; i < 1000; i++)
|
||||
{
|
||||
// The hash object is no longer shared
|
||||
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();
|
||||
}
|
||||
@@ -0,0 +1,30 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<include src="ThreadUnsafeICryptoTransform.qhelp" />
|
||||
|
||||
</overview>
|
||||
<recommendation>
|
||||
<p>Create new instances of the object that implements or has a field of type <code>System.Security.Cryptography.ICryptoTransform</code> to avoid sharing it accross multiple threads.</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 or may raise an exception.</p>
|
||||
<sample src="ThreadUnSafeICryptoTransformLambdaBad.cs" />
|
||||
|
||||
<p>A simple fix is to change the local variable <code>sha1</code> being captured by the lambda to be a local variable within the lambda.</p>
|
||||
<sample src="ThreadUnSafeICryptoTransformLambdaGood.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>
|
||||
@@ -0,0 +1,72 @@
|
||||
/**
|
||||
* @name Potential usage of a n object implementing ICryptoTransform class in a way that would be unsafe for concurrent threads.
|
||||
* @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 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-captured-in-lambda
|
||||
* @tags concurrency
|
||||
* security
|
||||
* external/cwe/cwe-362
|
||||
*/
|
||||
|
||||
import csharp
|
||||
import semmle.code.csharp.dataflow.DataFlow
|
||||
|
||||
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() )
|
||||
or usesICryptoTransformType( t.(Class).getAMember() )
|
||||
)
|
||||
}
|
||||
|
||||
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())
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
class UsesICryptoTransform extends Class {
|
||||
UsesICryptoTransform() {
|
||||
usesICryptoTransformType(this) or hasICryptoTransformMember(this)
|
||||
}
|
||||
}
|
||||
|
||||
class NotThreadSafeCryptoUsageIntoStartingCallingConfig extends TaintTracking::Configuration {
|
||||
NotThreadSafeCryptoUsageIntoStartingCallingConfig() { this = "NotThreadSafeCryptoUsageIntoStartingCallingConfig" }
|
||||
|
||||
override predicate isSource(DataFlow::Node source) {
|
||||
exists( LambdaExpr l, LocalScopeVariable lsvar, UsesICryptoTransform ict |
|
||||
l = source.asExpr() |
|
||||
ict = lsvar.getType()
|
||||
and lsvar.getACapturingCallable() = l
|
||||
)
|
||||
}
|
||||
|
||||
override predicate isSink(DataFlow::Node sink) {
|
||||
exists( DelegateCreation dc, Expr e |
|
||||
e = sink.asExpr() |
|
||||
dc.getArgument() = e
|
||||
and dc.getType().getName().matches("%Start")
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
from NotThreadSafeCryptoUsageIntoStartingCallingConfig config, LambdaExpr l, Expr e
|
||||
where config.hasFlow(DataFlow::exprNode(l), DataFlow::exprNode(e))
|
||||
select e, "A Lambda expression at " + l.getLocation() + " 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."
|
||||
@@ -0,0 +1,133 @@
|
||||
// semmle-extractor-options: /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll /r:System.Security.Cryptography.Primitives.dll /r:System.Threading.Tasks.dll /r:System.Threading.Thread.dll /r:System.Linq.dll /r:System.Collections.dll
|
||||
using System;
|
||||
using System.Linq;
|
||||
using System.Collections.Generic;
|
||||
using System.Threading;
|
||||
using System.Threading.Tasks;
|
||||
using System.Security.Cryptography;
|
||||
|
||||
class DirectUsagePositiveCase
|
||||
{
|
||||
public static void Run(int max)
|
||||
{
|
||||
const int threadCount = 4;
|
||||
|
||||
// This variable is used in multiple threads
|
||||
var sha1 = SHA1.Create();
|
||||
Action start = () => {
|
||||
for (int i = 0; i < max; i++)
|
||||
{
|
||||
var bytes = new byte[4];
|
||||
sha1.ComputeHash(bytes);
|
||||
}
|
||||
};
|
||||
|
||||
// BUG expected
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
||||
class DirectUsageNegativeCase
|
||||
{
|
||||
public static void Run(int max)
|
||||
{
|
||||
const int threadCount = 4;
|
||||
Action start = () => {
|
||||
for (int i = 0; i < max; i++)
|
||||
{
|
||||
var sha1 = SHA1.Create();
|
||||
var bytes = new byte[4];
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
||||
public class Nest01
|
||||
{
|
||||
private readonly SHA256 _sha;
|
||||
|
||||
public Nest01()
|
||||
{
|
||||
_sha = SHA256.Create();
|
||||
}
|
||||
|
||||
public byte[] ComputeHash(byte[] bytes)
|
||||
{
|
||||
return _sha.ComputeHash(bytes);
|
||||
}
|
||||
}
|
||||
|
||||
class IndirectUsagePositiveCase
|
||||
{
|
||||
public static void Run(int max)
|
||||
{
|
||||
const int threadCount = 4;
|
||||
// This variable is used in multiple threads
|
||||
var sha1 = new Nest01();
|
||||
|
||||
// BUG expected
|
||||
Action start = () => {
|
||||
for (int i = 0; i < max; i++)
|
||||
{
|
||||
var bytes = new byte[4];
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
||||
class indirectUsageNegativeCase
|
||||
{
|
||||
public static void Run(int max)
|
||||
{
|
||||
const int threadCount = 4;
|
||||
Action start = () => {
|
||||
for (int i = 0; i < max; i++)
|
||||
{
|
||||
var sha1 = new Nest01();
|
||||
var bytes = new byte[4];
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
||||
class LambdaNotStart
|
||||
{
|
||||
public static void Run()
|
||||
{
|
||||
var sha1 = SHA1.Create();
|
||||
|
||||
Func<string> myFunc = () =>
|
||||
{
|
||||
var bytes = new byte[4];
|
||||
return Convert.ToBase64String(sha1.ComputeHash(bytes));
|
||||
};
|
||||
|
||||
var d = myFunc.DynamicInvoke();
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,2 @@
|
||||
| ThreadUnsafeICryptoTransformLambda.cs:27:62:27:66 | access to local variable start | A Lambda expression at ThreadUnsafeICryptoTransformLambda.cs:17:24:23:9 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. |
|
||||
| ThreadUnsafeICryptoTransformLambda.cs:89:62:89:66 | access to local variable start | A Lambda expression at ThreadUnsafeICryptoTransformLambda.cs:81:24:87:9 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. |
|
||||
@@ -0,0 +1 @@
|
||||
Likely Bugs/ThreadUnsafeICryptoTransformLambda.ql
|
||||
Reference in New Issue
Block a user