Skip to content

Commit 5be19d7

Browse files
raulgarciamsftraulgarciamsft
committed
Separating some reusable code into QLL libraries.
Fixing bugs from code review.
1 parent 4ad9163 commit 5be19d7

File tree

4 files changed

+73
-56
lines changed

4 files changed

+73
-56
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: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
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+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,9 @@ public static void RunThreadUnSafeICryptoTransformLambdaFixed()
44
var b = new Barrier(threadCount);
55
Action start = () => {
66
b.SignalAndWait();
7+
// The hash object is no longer shared
78
for (int i = 0; i < 1000; i++)
89
{
9-
// The hash object is no longer shared
1010
var sha1 = SHA1.Create();
1111
var pwd = Guid.NewGuid().ToString();
1212
var bytes = Encoding.UTF8.GetBytes(pwd);

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

Lines changed: 18 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
/**
2-
* @name Potential usage of a n object implementing ICryptoTransform class in a way that would be unsafe for concurrent threads.
2+
* @name Potential usage of an object implementing ICryptoTransform class in a way that would be unsafe for concurrent threads.
33
* @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,
44
* and used in what seems to be a thread initialization method.
5-
* Using this an instance of this class in concurrent threads is dangerous as it may not only result in an error,
5+
* Using an instance of this class in concurrent threads is dangerous as it may not only result in an error,
66
* but under some circumstances may also result in incorrect results.
77
* @kind problem
88
* @problem.severity warning
@@ -15,47 +15,21 @@
1515

1616
import csharp
1717
import semmle.code.csharp.dataflow.DataFlow
18-
19-
class ICryptoTransform extends Class {
20-
ICryptoTransform() {
21-
this.getABaseType*().hasQualifiedName("System.Security.Cryptography", "ICryptoTransform")
22-
}
23-
}
24-
25-
predicate usesICryptoTransformType( Type t ) {
26-
exists( ICryptoTransform ict |
27-
ict = t
28-
or usesICryptoTransformType( t.getAChild() )
29-
or usesICryptoTransformType( t.(Class).getAMember() )
30-
)
31-
}
32-
33-
predicate hasICryptoTransformMember( Class c) {
34-
c.getAField().getType() instanceof UsesICryptoTransform
35-
}
36-
37-
class UsesICryptoTransform extends Class {
38-
UsesICryptoTransform() {
39-
usesICryptoTransformType(this) or hasICryptoTransformMember(this)
40-
}
41-
}
18+
import ParallelSink
19+
import ICryptoTransform
4220

4321
class NotThreadSafeCryptoUsageIntoStartingCallingConfig extends TaintTracking::Configuration {
4422
NotThreadSafeCryptoUsageIntoStartingCallingConfig() { this = "NotThreadSafeCryptoUsageIntoStartingCallingConfig" }
4523

4624
override predicate isSource(DataFlow::Node source) {
47-
exists( LambdaExpr l, LocalScopeVariable lsvar, UsesICryptoTransform ict |
48-
l = source.asExpr() |
49-
ict = lsvar.getType()
50-
and lsvar.getACapturingCallable() = l
51-
)
25+
source instanceof LambdaCapturingICryptoTransformSource
5226
}
5327

5428
override predicate isSink(DataFlow::Node sink) {
5529
exists( DelegateCreation dc, Expr e |
56-
e = sink.asExpr() |
57-
dc.getArgument() = e
58-
and dc.getType().getName().matches("%Start")
30+
e = sink.asExpr() |
31+
dc.getArgument() = e
32+
and dc.getType().getName().matches("%Start")
5933
)
6034
}
6135
}
@@ -64,33 +38,22 @@ class NotThreadSafeCryptoUsageIntoParallelInvokeConfig extends TaintTracking::Co
6438
NotThreadSafeCryptoUsageIntoParallelInvokeConfig() { this = "NotThreadSafeCryptoUsageIntoParallelInvokeConfig" }
6539

6640
override predicate isSource(DataFlow::Node source) {
67-
exists( LambdaExpr l, LocalScopeVariable lsvar, UsesICryptoTransform ict |
68-
l = source.asExpr() |
69-
ict = lsvar.getType()
70-
and lsvar.getACapturingCallable() = l
71-
)
41+
source instanceof LambdaCapturingICryptoTransformSource
7242
}
7343

7444
override predicate isSink(DataFlow::Node sink) {
75-
exists( Class c, Method m, MethodCall mc, Expr e |
76-
e = sink.asExpr() |
77-
c.getABaseType*().hasQualifiedName("System.Threading.Tasks", "Parallel")
78-
and c.getAMethod() = m
79-
and m.getName() = "Invoke"
80-
and m.getACall() = mc
81-
and mc.getAnArgument() = e
82-
)
45+
sink instanceof ParallelSink
8346
}
8447
}
8548

86-
from Expr e, string m
49+
from Expr e, string m, LambdaExpr l
8750
where
88-
exists( NotThreadSafeCryptoUsageIntoParallelInvokeConfig config, LambdaExpr l |
89-
config.hasFlow(DataFlow::exprNode(l), DataFlow::exprNode(e))
90-
and m = "A Lambda expression at " + l.getLocation() + " seems to be used to start a new thread using System.Threading.Tasks.Parallel.Invoke, and is capturing a local variable that either implements 'System.Security.Cryptography.ICryptoTransform' or has a field of this type."
51+
exists( NotThreadSafeCryptoUsageIntoParallelInvokeConfig config |
52+
config.hasFlow(DataFlow::exprNode(l), DataFlow::exprNode(e))
53+
and m = "A $@ seems to be used to start a new thread using System.Threading.Tasks.Parallel.Invoke, and is capturing a local variable that either implements 'System.Security.Cryptography.ICryptoTransform' or has a field of this type."
9154
)
92-
or exists ( NotThreadSafeCryptoUsageIntoStartingCallingConfig config, LambdaExpr l |
93-
config.hasFlow(DataFlow::exprNode(l), DataFlow::exprNode(e))
94-
and m = "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."
55+
or exists ( NotThreadSafeCryptoUsageIntoStartingCallingConfig config |
56+
config.hasFlow(DataFlow::exprNode(l), DataFlow::exprNode(e))
57+
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."
9558
)
96-
select e, m
59+
select e, m, l, "lambda expression"

0 commit comments

Comments
 (0)