hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-05-02 04:05:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Adding a new rule for detecting usage of static objects that implement ICryptoTransform that would be thread-unsafe, and potentially result in incorrect cryptographic results.

Browse Source
This commit is contained in:
Raul Garcia
2019-02-20 17:07:04 -08:00
parent 54493eb990
commit 7d197692ac
8 changed files with 330 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

38
csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformFix.cs Normal file
View File

@@ -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);
}
}

41
csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.cs Normal file
View File

@@ -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);
}
}

34
csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp Normal file
View File

@@ -0,0 +1,34 @@
<!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>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 object in multiple threads.</p>
</overview>
<recommendation>
<p>Verify that the object is not being shared across threads.</p>
<p>If it is shared accross instances. 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 such a way that the results may be incorrect.</p>
<sample src="ThreadUnsafeICryptoTransform.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="ThreadUnSafeICryptoTransformFix.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>

93
csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.ql Normal file
View File

@@ -0,0 +1,93 @@
/**
* @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
exists( Field f |
f = c.getAMember*()
and not f.isStatic()
and ( hasICryptoTransformStaticMember( f.getType(), _ )
and msg = "Non-static field " + f + " of type " + f.getType() + " internally makes use of an static object that implements 'System.Security.Cryptography.ICryptoTransform'. This causes that usage of this class member 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

114
csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs Normal file
View File

@@ -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 IndirectStatic
{
StaticMember tc;
}
public class IndirectStatic2
{
static Nest02 _n = new Nest02();
}
/// <summary>
/// Should not be flagged (thread safe)
/// </summary>
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;
}
}

6
csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected Normal file
View File

@@ -0,0 +1,6 @@
| 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:27 | IndirectStatic | Non-static field tc of type StaticMember internally makes use of an static object that implements 'System.Security.Cryptography.ICryptoTransform'. This causes that usage of this class member is unsafe for concurrent threads. |
| ThreadUnsafeICryptoTransform.cs:74:14:74:28 | IndirectStatic2 | ClassIndirectStatic2 implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads. |

1
csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.qlref Normal file
View File

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