Skip to content

Commit 5a3cf2c

Browse files
calumgrantcalumgrant
authored
Merge pull request #1054 from raulgarciamsft/users/raulga/ICryptoTransformLambda
2n part of ICryptoTransform.
2 parents 5441352 + 110c750 commit 5a3cf2c

9 files changed

+343
-0
lines changed

‎csharp/ql/src/Likely Bugs/ICryptoTransform.qll‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import csharp
2+
import semmle.code.csharp.dataflow.DataFlow
3+
4+
import csharp
5+
class ImplementsICryptoTransform extends Class {
6+
ImplementsICryptoTransform() {
7+
this.getABaseType*().hasQualifiedName("System.Security.Cryptography", "ICryptoTransform")
8+
}
9+
}
10+
11+
predicate usesICryptoTransformType( ValueOrRefType t ) {
12+
exists( ImplementsICryptoTransform ict |
13+
ict = t
14+
or usesICryptoTransformType( t.getAChild() )
15+
)
16+
}
17+
18+
predicate hasICryptoTransformMember( Class c) {
19+
c.getAField().getType() instanceof UsesICryptoTransform
20+
}
21+
22+
class UsesICryptoTransform extends Class {
23+
UsesICryptoTransform() {
24+
usesICryptoTransformType(this) or hasICryptoTransformMember(this)
25+
}
26+
}
27+
28+
class LambdaCapturingICryptoTransformSource extends DataFlow::Node {
29+
LambdaCapturingICryptoTransformSource() {
30+
exists( LambdaExpr l, LocalScopeVariable lsvar, UsesICryptoTransform ict |
31+
l = this.asExpr() |
32+
ict = lsvar.getType()
33+
and lsvar.getACapturingCallable() = l
34+
)
35+
}
36+
}

‎csharp/ql/src/Likely Bugs/ParallelSink.qll‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import csharp
2+
import semmle.code.csharp.dataflow.DataFlow
3+
4+
abstract class ParallelSink extends DataFlow::Node {
5+
}
6+
7+
class LambdaParallelSink extends ParallelSink {
8+
LambdaParallelSink() {
9+
exists( Class c, Method m, MethodCall mc, Expr e |
10+
e = this.asExpr() |
11+
c.getABaseType*().hasQualifiedName("System.Threading.Tasks", "Parallel")
12+
and c.getAMethod() = m
13+
and m.getName() = "Invoke"
14+
and m.getACall() = mc
15+
and mc.getAnArgument() = e
16+
)
17+
}
18+
}
19+
20+
class ThreadStartParallelSink extends ParallelSink {
21+
ThreadStartParallelSink() {
22+
exists( DelegateCreation dc, Expr e |
23+
e = this.asExpr() |
24+
dc.getArgument() = e
25+
and dc.getType().getName().matches("%Start")
26+
)
27+
}
28+
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
public static void RunThreadUnSafeICryptoTransformLambdaBad()
2+
{
3+
const int threadCount = 4;
4+
// This local variable for a hash object is going to be shared across multiple threads
5+
var sha1 = SHA1.Create();
6+
var b = new Barrier(threadCount);
7+
Action start = () => {
8+
b.SignalAndWait();
9+
for (int i = 0; i < 1000; i++)
10+
{
11+
var pwd = Guid.NewGuid().ToString();
12+
var bytes = Encoding.UTF8.GetBytes(pwd);
13+
// This call may fail, or return incorrect results
14+
sha1.ComputeHash(bytes);
15+
}
16+
};
17+
var threads = Enumerable.Range(0, threadCount)
18+
.Select(_ => new ThreadStart(start))
19+
.Select(x => new Thread(x))
20+
.ToList();
21+
foreach (var t in threads) t.Start();
22+
foreach (var t in threads) t.Join();
23+
}

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
public static void RunThreadUnSafeICryptoTransformLambdaFixed()
2+
{
3+
const int threadCount = 4;
4+
var b = new Barrier(threadCount);
5+
Action start = () => {
6+
b.SignalAndWait();
7+
// The hash object is no longer shared
8+
for (int i = 0; i < 1000; i++)
9+
{
10+
var sha1 = SHA1.Create();
11+
var pwd = Guid.NewGuid().ToString();
12+
var bytes = Encoding.UTF8.GetBytes(pwd);
13+
sha1.ComputeHash(bytes);
14+
}
15+
};
16+
var threads = Enumerable.Range(0, threadCount)
17+
.Select(_ => new ThreadStart(start))
18+
.Select(x => new Thread(x))
19+
.ToList();
20+
foreach (var t in threads) t.Start();
21+
foreach (var t in threads) t.Join();
22+
}

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<include src="ThreadUnsafeICryptoTransform.qhelp" />
7+
8+
</overview>
9+
<recommendation>
10+
<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>
11+
12+
</recommendation>
13+
<example>
14+
<p>This example demonstrates the dangers of using a shared <code>System.Security.Cryptography.ICryptoTransform</code> in a way that generates incorrect results or may raise an exception.</p>
15+
<sample src="ThreadUnSafeICryptoTransformLambdaBad.cs" />
16+
17+
<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>
18+
<sample src="ThreadUnSafeICryptoTransformLambdaGood.cs" />
19+
</example>
20+
21+
<references>
22+
<li>
23+
Microsoft documentation, <a href="https://docs.microsoft.com/en-us/dotnet/api/system.threadstaticattribute?view=netframework-4.7.2">ThreadStaticAttribute Class</a>.
24+
</li>
25+
<li>
26+
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>.
27+
</li>
28+
</references>
29+
30+
</qhelp>

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/**
2+
* @name Potential usage of an object implementing ICryptoTransform class in a way that would be unsafe for concurrent threads.
3+
* @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,
4+
* and used in what seems to be a thread initialization method.
5+
* Using an instance of this class in concurrent threads is dangerous as it may not only result in an error,
6+
* but under some circumstances may also result in incorrect results.
7+
* @kind problem
8+
* @problem.severity warning
9+
* @precision medium
10+
* @id cs/thread-unsafe-icryptotransform-captured-in-lambda
11+
* @tags concurrency
12+
* security
13+
* external/cwe/cwe-362
14+
*/
15+
16+
import csharp
17+
import semmle.code.csharp.dataflow.DataFlow
18+
import ParallelSink
19+
import ICryptoTransform
20+
21+
class NotThreadSafeCryptoUsageIntoParallelInvokeConfig extends TaintTracking::Configuration {
22+
NotThreadSafeCryptoUsageIntoParallelInvokeConfig() { this = "NotThreadSafeCryptoUsageIntoParallelInvokeConfig" }
23+
24+
override predicate isSource(DataFlow::Node source) {
25+
source instanceof LambdaCapturingICryptoTransformSource
26+
}
27+
28+
override predicate isSink(DataFlow::Node sink) {
29+
sink instanceof ParallelSink
30+
}
31+
}
32+
33+
from Expr e, string m, LambdaExpr l, NotThreadSafeCryptoUsageIntoParallelInvokeConfig config
34+
where
35+
config.hasFlow(DataFlow::exprNode(l), DataFlow::exprNode(e))
36+
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."
37+
select e, m, l, "lambda expression"

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

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
// 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 /r:System.Threading.Tasks.Parallel.dll
2+
using System;
3+
using System.Linq;
4+
using System.Collections.Generic;
5+
using System.Threading;
6+
using System.Threading.Tasks;
7+
using System.Security.Cryptography;
8+
9+
class DirectUsagePositiveCase
10+
{
11+
public static void Run(int max)
12+
{
13+
const int threadCount = 4;
14+
15+
// This variable is used in multiple threads
16+
var sha1 = SHA1.Create();
17+
Action start = () => {
18+
for (int i = 0; i < max; i++)
19+
{
20+
var bytes = new byte[4];
21+
sha1.ComputeHash(bytes);
22+
}
23+
};
24+
25+
// BUG expected
26+
var threads = Enumerable.Range(0, threadCount)
27+
.Select(_ => new ThreadStart(start))
28+
.Select(x => new Thread(x))
29+
.ToList();
30+
foreach (var t in threads) t.Start();
31+
foreach (var t in threads) t.Join();
32+
}
33+
}
34+
35+
class DirectUsageNegativeCase
36+
{
37+
public static void Run(int max)
38+
{
39+
const int threadCount = 4;
40+
Action start = () => {
41+
for (int i = 0; i < max; i++)
42+
{
43+
var sha1 = SHA1.Create();
44+
var bytes = new byte[4];
45+
sha1.ComputeHash(bytes);
46+
}
47+
};
48+
var threads = Enumerable.Range(0, threadCount)
49+
.Select(_ => new ThreadStart(start))
50+
.Select(x => new Thread(x))
51+
.ToList();
52+
foreach (var t in threads) t.Start();
53+
foreach (var t in threads) t.Join();
54+
}
55+
}
56+
57+
public class Nest01
58+
{
59+
private readonly SHA256 _sha;
60+
61+
public Nest01()
62+
{
63+
_sha = SHA256.Create();
64+
}
65+
66+
public byte[] ComputeHash(byte[] bytes)
67+
{
68+
return _sha.ComputeHash(bytes);
69+
}
70+
}
71+
72+
class IndirectUsagePositiveCase
73+
{
74+
public static void Run(int max)
75+
{
76+
const int threadCount = 4;
77+
// This variable is used in multiple threads
78+
var sha1 = new Nest01();
79+
80+
// BUG expected
81+
Action start = () => {
82+
for (int i = 0; i < max; i++)
83+
{
84+
var bytes = new byte[4];
85+
sha1.ComputeHash(bytes);
86+
}
87+
};
88+
var threads = Enumerable.Range(0, threadCount)
89+
.Select(_ => new ThreadStart(start))
90+
.Select(x => new Thread(x))
91+
.ToList();
92+
foreach (var t in threads) t.Start();
93+
foreach (var t in threads) t.Join();
94+
}
95+
}
96+
97+
class IndirectUsageNegativeCase
98+
{
99+
public static void Run(int max)
100+
{
101+
const int threadCount = 4;
102+
Action start = () => {
103+
for (int i = 0; i < max; i++)
104+
{
105+
var sha1 = new Nest01();
106+
var bytes = new byte[4];
107+
sha1.ComputeHash(bytes);
108+
}
109+
};
110+
var threads = Enumerable.Range(0, threadCount)
111+
.Select(_ => new ThreadStart(start))
112+
.Select(x => new Thread(x))
113+
.ToList();
114+
foreach (var t in threads) t.Start();
115+
foreach (var t in threads) t.Join();
116+
}
117+
}
118+
119+
class LambdaNotStart
120+
{
121+
public static void Run()
122+
{
123+
var sha1 = SHA1.Create();
124+
125+
Func<string> myFunc = () =>
126+
{
127+
var bytes = new byte[4];
128+
return Convert.ToBase64String(sha1.ComputeHash(bytes));
129+
};
130+
131+
var d = myFunc.DynamicInvoke();
132+
}
133+
}
134+
135+
class ParallelInvoke
136+
{
137+
public static void Run()
138+
{
139+
var sha1 = SHA1.Create();
140+
141+
try
142+
{
143+
Parallel.Invoke(() =>
144+
{
145+
var bytes = new byte[4];
146+
Convert.ToBase64String(sha1.ComputeHash(bytes));
147+
},
148+
() =>
149+
{
150+
var bytes = new byte[4];
151+
Convert.ToBase64String(sha1.ComputeHash(bytes));
152+
}
153+
);
154+
155+
}
156+
catch (AggregateException e)
157+
{
158+
Console.WriteLine("An action has thrown an exception. THIS WAS UNEXPECTED.\n{0}", e.InnerException.ToString());
159+
}
160+
}
161+
162+
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
| ThreadUnsafeICryptoTransformLambda.cs:27:62:27:66 | access to local variable start | 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. | ThreadUnsafeICryptoTransformLambda.cs:17:24:23:9 | (...) => ... | lambda expression |
2+
| ThreadUnsafeICryptoTransformLambda.cs:89:62:89:66 | access to local variable start | 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. | ThreadUnsafeICryptoTransformLambda.cs:81:24:87:9 | (...) => ... | lambda expression |
3+
| ThreadUnsafeICryptoTransformLambda.cs:143:29:147:17 | (...) => ... | 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. | ThreadUnsafeICryptoTransformLambda.cs:143:29:147:17 | (...) => ... | lambda expression |
4+
| ThreadUnsafeICryptoTransformLambda.cs:148:17:152:17 | (...) => ... | 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. | ThreadUnsafeICryptoTransformLambda.cs:148:17:152:17 | (...) => ... | lambda expression |

‎csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransformLambda/ThreadUnsafeICryptoTransformLambda.qlref‎

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

0 commit comments

Comments
 (0)