Skip to content

Commit 51e5a30

Browse files
hvitvedhvitved
authored
Merge pull request #956 from raulgarciamsft/users/raulga/ICryptoTransform
Detect usage of ICryptoTransform that would be thread-unsafe
2 parents 83e0f3b + 9eca21c commit 51e5a30

File tree

7 files changed

+322
-0
lines changed

7 files changed

+322
-0
lines changed

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
internal class TokenCacheThreadUnsafeICryptoTransformDemo
2+
{
3+
private static SHA256 _sha = SHA256.Create();
4+
5+
public string ComputeHash(string data)
6+
{
7+
byte[] passwordBytes = UTF8Encoding.UTF8.GetBytes(data);
8+
return Convert.ToBase64String(_sha.ComputeHash(passwordBytes));
9+
}
10+
}
11+
12+
class Program
13+
{
14+
static void Main(string[] args)
15+
{
16+
int max = 1000;
17+
Task[] tasks = new Task[max];
18+
19+
Action<object> action = (object obj) =>
20+
{
21+
var unsafeObj = new TokenCacheThreadUnsafeICryptoTransformDemo();
22+
if (unsafeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=")
23+
{
24+
Console.WriteLine("**** We got incorrect Results!!! ****");
25+
}
26+
};
27+
28+
for (int i = 0; i < max; i++)
29+
{
30+
// hash calculated on all threads should be the same:
31+
// ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0= (base64)
32+
//
33+
tasks[i] = Task.Factory.StartNew(action, "abc");
34+
}
35+
36+
Task.WaitAll(tasks);
37+
}
38+
}

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
internal class TokenCacheThreadUnsafeICryptoTransformDemoFixed
2+
{
3+
// We are replacing the static SHA256 field with an instance one
4+
//
5+
//private static SHA256 _sha = SHA256.Create();
6+
private SHA256 _sha = SHA256.Create();
7+
8+
public string ComputeHash(string data)
9+
{
10+
byte[] passwordBytes = UTF8Encoding.UTF8.GetBytes(data);
11+
return Convert.ToBase64String(_sha.ComputeHash(passwordBytes));
12+
}
13+
}
14+
15+
class Program
16+
{
17+
static void Main(string[] args)
18+
{
19+
int max = 1000;
20+
Task[] tasks = new Task[max];
21+
22+
Action<object> action = (object obj) =>
23+
{
24+
var safeObj = new TokenCacheThreadUnsafeICryptoTransformDemoFixed();
25+
if (safeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=")
26+
{
27+
Console.WriteLine("**** We got incorrect Results!!! ****");
28+
}
29+
};
30+
31+
for (int i = 0; i < max; i++)
32+
{
33+
// hash calculated on all threads should be the same:
34+
// ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0= (base64)
35+
//
36+
tasks[i] = Task.Factory.StartNew(action, "abc");
37+
}
38+
39+
Task.WaitAll(tasks);
40+
}
41+
}

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Classes that implement <code>System.Security.Cryptography.ICryptoTransform</code> are not thread safe.</p>
7+
<p>This problem is caused by the way these classes are implemented using Microsoft CAPI/CNG patterns.</p>
8+
<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>
9+
<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>
10+
<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>
11+
<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>
12+
<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>
13+
<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>
14+
15+
</overview>
16+
<recommendation>
17+
<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>
18+
<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>
19+
20+
</recommendation>
21+
<example>
22+
<p>This example demonstrates the dangers of using a static <code>System.Security.Cryptography.ICryptoTransform</code> in a way that generates incorrect results.</p>
23+
<sample src="ThreadUnSafeICryptoTransformBad.cs" />
24+
25+
<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>
26+
<sample src="ThreadUnSafeICryptoTransformGood.cs" />
27+
</example>
28+
29+
<references>
30+
<li>
31+
Microsoft documentation, <a href="https://docs.microsoft.com/en-us/dotnet/api/system.threadstaticattribute?view=netframework-4.7.2">ThreadStaticAttribute Class</a>.
32+
</li>
33+
<li>
34+
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>.
35+
</li>
36+
</references>
37+
38+
</qhelp>

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

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
/**
2+
* @name Class defines a field that uses an ICryptoTransform class in a way that would be unsafe for concurrent threads.
3+
* @description The class has a field that directly or indirectly make use of a static System.Security.Cryptography.ICryptoTransform object.
4+
* Using this an instance of this class in concurrent threads is dangerous as it may not only result in an error,
5+
* but under some circumstances may also result in incorrect results.
6+
* @kind problem
7+
* @problem.severity warning
8+
* @precision medium
9+
* @id cs/thread-unsafe-icryptotransform-field-in-class
10+
* @tags concurrency
11+
* security
12+
* external/cwe/cwe-362
13+
*/
14+
15+
import csharp
16+
17+
class ICryptoTransform extends Class {
18+
ICryptoTransform() {
19+
this.getABaseType*().hasQualifiedName("System.Security.Cryptography", "ICryptoTransform")
20+
}
21+
}
22+
23+
predicate usesICryptoTransformType( Type t ) {
24+
exists( ICryptoTransform ict |
25+
ict = t
26+
or usesICryptoTransformType( t.getAChild() )
27+
)
28+
}
29+
30+
predicate hasICryptoTransformMember( Class c) {
31+
exists( Field f |
32+
f = c.getAMember()
33+
and (
34+
exists( ICryptoTransform ict | ict = f.getType() )
35+
or hasICryptoTransformMember(f.getType())
36+
or usesICryptoTransformType(f.getType())
37+
)
38+
)
39+
}
40+
41+
predicate hasICryptoTransformStaticMemberNested( Class c ) {
42+
exists( Field f |
43+
f = c.getAMember() |
44+
hasICryptoTransformStaticMemberNested( f.getType() )
45+
or (
46+
f.isStatic() and hasICryptoTransformMember(f.getType())
47+
and not exists( Attribute a
48+
| a = f.getAnAttribute() |
49+
a.getType().getQualifiedName() = "System.ThreadStaticAttribute"
50+
)
51+
)
52+
)
53+
}
54+
55+
predicate hasICryptoTransformStaticMember( Class c, string msg) {
56+
exists( Field f |
57+
f = c.getAMember*()
58+
and f.isStatic()
59+
and not exists( Attribute a
60+
| a = f.getAnAttribute()
61+
and a.getType().getQualifiedName() = "System.ThreadStaticAttribute"
62+
)
63+
and (
64+
exists( ICryptoTransform ict |
65+
ict = f.getType()
66+
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."
67+
)
68+
or
69+
(
70+
not exists( ICryptoTransform ict | ict = f.getType() ) // Avoid dup messages
71+
and exists( Type t | t = f.getType() |
72+
usesICryptoTransformType(t)
73+
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."
74+
)
75+
)
76+
)
77+
)
78+
or ( hasICryptoTransformStaticMemberNested(c)
79+
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."
80+
)
81+
}
82+
83+
from Class c , string s
84+
where hasICryptoTransformStaticMember(c, s)
85+
select c, s

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

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
// semmle-extractor-options: /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll /r:System.Security.Cryptography.Primitives.dll
2+
using System;
3+
using System.Collections.Generic;
4+
using System.Security.Cryptography;
5+
6+
public class Nest01
7+
{
8+
private readonly SHA256 _sha;
9+
10+
public Nest01()
11+
{
12+
_sha = SHA256.Create();
13+
}
14+
}
15+
16+
public class Nest02
17+
{
18+
private readonly Nest01 _n;
19+
20+
public Nest02()
21+
{
22+
_n = new Nest01();
23+
}
24+
}
25+
26+
public class ListNonStatic
27+
{
28+
private List<SHA512> _shaList;
29+
30+
public ListNonStatic()
31+
{
32+
_shaList = new List<SHA512>();
33+
}
34+
}
35+
36+
/// <summary>
37+
/// Positive results (classes are not thread safe)
38+
/// </summary>
39+
public class Nest03
40+
{
41+
private static readonly Nest01 _n = new Nest01();
42+
}
43+
44+
public class Nest04
45+
{
46+
static ListNonStatic _list = new ListNonStatic();
47+
}
48+
49+
public static class StaticMemberChildUsage
50+
{
51+
public enum DigestAlgorithm
52+
{
53+
SHA1,
54+
SHA256,
55+
}
56+
57+
private static readonly Dictionary<DigestAlgorithm, HashAlgorithm> HashMap = new Dictionary<DigestAlgorithm, HashAlgorithm>
58+
{
59+
{ DigestAlgorithm.SHA1, SHA1.Create() },
60+
{ DigestAlgorithm.SHA256, SHA256.Create() },
61+
};
62+
}
63+
64+
public class StaticMember
65+
{
66+
private static SHA1 _sha1 = SHA1.Create();
67+
}
68+
69+
public class IndirectStatic2
70+
{
71+
static Nest02 _n = new Nest02();
72+
}
73+
74+
/// <summary>
75+
/// Should not be flagged (thread safe)
76+
/// </summary>
77+
78+
public class IndirectStatic
79+
{
80+
StaticMember tc;
81+
}
82+
83+
public class TokenCacheFP
84+
{
85+
/// <summary>
86+
/// Should be OK. Not shared between threads
87+
/// </summary>
88+
[ThreadStatic]
89+
private static SHA1 _sha1 = SHA1.Create();
90+
91+
private string ComputeHash(string password)
92+
{
93+
return password;
94+
}
95+
}
96+
97+
public class TokenCacheNonStat
98+
{
99+
/// <summary>
100+
/// Should be OK. Not shared between threads
101+
/// </summary>
102+
private SHA1 _sha1;
103+
104+
public TokenCacheNonStat()
105+
{
106+
_sha1 = SHA1.Create();
107+
}
108+
109+
private string ComputeHash(string password)
110+
{
111+
return password;
112+
}
113+
}
114+

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
| 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. |
2+
| 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. |
3+
| 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. |
4+
| 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. |
5+
| 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‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Likely Bugs/ThreadUnsafeICryptoTransform.ql

0 commit comments

Comments
 (0)