From 7d197692acfd4271a03ce4953bf570d9efaa32b1 Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Wed, 20 Feb 2019 17:07:04 -0800 Subject: [PATCH 1/8] 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. --- .gitignore | 3 + .../ThreadUnSafeICryptoTransformFix.cs | 38 ++++++ .../ThreadUnsafeICryptoTransform.cs | 41 +++++++ .../ThreadUnsafeICryptoTransform.qhelp | 34 ++++++ .../ThreadUnsafeICryptoTransform.ql | 93 ++++++++++++++ .../ThreadUnsafeICryptoTransform.cs | 114 ++++++++++++++++++ .../ThreadUnsafeICryptoTransform.expected | 6 + .../ThreadUnsafeICryptoTransform.qlref | 1 + 8 files changed, 330 insertions(+) create mode 100644 csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformFix.cs create mode 100644 csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.cs create mode 100644 csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp create mode 100644 csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.ql create mode 100644 csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs create mode 100644 csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected create mode 100644 csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.qlref diff --git a/.gitignore b/.gitignore index 7e82b2f488ca..c614f8f91592 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,6 @@ /.vs/ql/v15/Browse.VC.db /.vs/ProjectSettings.json +/.vs/ql_ICryptoTransform/v15/Browse.VC.opendb +/.vs/ql_ICryptoTransform/v15/Browse.VC.db +/.vs/ql_ICryptoTransform/v15/.suo diff --git a/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformFix.cs b/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformFix.cs new file mode 100644 index 000000000000..b698abdb600d --- /dev/null +++ b/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformFix.cs @@ -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 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); + } +} diff --git a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.cs b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.cs new file mode 100644 index 000000000000..dbbc3586f981 --- /dev/null +++ b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.cs @@ -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 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); + } +} diff --git a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp new file mode 100644 index 000000000000..0f1ae02868d0 --- /dev/null +++ b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp @@ -0,0 +1,34 @@ + + + +

Classes that implement System.Security.Cryptography.ICryptoTransform are not thread safe.

+

Any object that implements System.Security.Cryptography.ICryptoTransform should not be used in concurrent threads as the instance members of such object are also not thread safe.

+

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.

+ + + +

Verify that the object is not being shared across threads.

+

If it is shared accross instances. Consider changing the code to use a non-static object of type System.Security.Cryptography.ICryptoTransform instead.

+

As an alternative, you could also look into using ThreadStatic attribute, but make sure you read the initialization remarks on the documentation.

+ + + +

This example demonstrates the dangers of using a static System.Security.Cryptography.ICryptoTransform in such a way that the results may be incorrect.

+ + +

A simple fix is to change the _sha field from being a static member to an instance one by removing the static keyword.

+ + + + +
  • + Microsoft documentation, ThreadStaticAttribute Class. +
    • +
  • + Stack Overflow, Why does SHA1.ComputeHash fail under high load with many threads?. +
    • +     + +     diff --git a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.ql b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.ql new file mode 100644 index 000000000000..6db9fad3b45b --- /dev/null +++ b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.ql @@ -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 diff --git a/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs b/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs new file mode 100644 index 000000000000..90214fc988a2 --- /dev/null +++ b/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs @@ -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 _shaList; + + public ListNonStatic() + { + _shaList = new List(); + } +} + +/// +/// Positive results (classes are not thread safe) +/// +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 HashMap = new Dictionary + { + { 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(); +} + +/// +/// Should not be flagged (thread safe) +/// + +public class TokenCacheFP +{ + /// + /// Should be OK. Not shared between threads + /// + [ThreadStatic] + private static SHA1 _sha1 = SHA1.Create(); + + private string ComputeHash(string password) + { + return password; + } +} + +public class TokenCacheNonStat +{ + /// + /// Should be OK. Not shared between threads + /// + private SHA1 _sha1; + + public TokenCacheNonStat() + { + _sha1 = SHA1.Create(); + } + + private string ComputeHash(string password) + { + return password; + } +} + diff --git a/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected b/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected new file mode 100644 index 000000000000..e9ca1952f635 --- /dev/null +++ b/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected @@ -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 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. | diff --git a/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.qlref b/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.qlref new file mode 100644 index 000000000000..e247961a538d --- /dev/null +++ b/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.qlref @@ -0,0 +1 @@ +Likely Bugs/ThreadUnsafeICryptoTransform.ql \ No newline at end of file From fa73b8488aee98f925b8267baf5531b416d63ede Mon Sep 17 00:00:00 2001 From: Raul Garcia <42392023+raulgarciamsft@users.noreply.github.com> Date: Wed, 20 Feb 2019 17:10:19 -0800 Subject: [PATCH 2/8] Update .gitignore --- .gitignore | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.gitignore b/.gitignore index c614f8f91592..4b055e55a091 100644 --- a/.gitignore +++ b/.gitignore @@ -12,7 +12,3 @@ /.vs/ql/v15/Browse.VC.opendb /.vs/ql/v15/Browse.VC.db /.vs/ProjectSettings.json - -/.vs/ql_ICryptoTransform/v15/Browse.VC.opendb -/.vs/ql_ICryptoTransform/v15/Browse.VC.db -/.vs/ql_ICryptoTransform/v15/.suo From 143b1e576ef40b3918e9227fa23058cdd2bc9d14 Mon Sep 17 00:00:00 2001 From: Raul Garcia <42392023+raulgarciamsft@users.noreply.github.com> Date: Wed, 20 Feb 2019 17:10:32 -0800 Subject: [PATCH 3/8] Update .gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 4b055e55a091..7e82b2f488ca 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,4 @@ /.vs/ql/v15/Browse.VC.opendb /.vs/ql/v15/Browse.VC.db /.vs/ProjectSettings.json + From 9bb7816a3ce375daa955f77da1d24c5e16c01903 Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Fri, 22 Feb 2019 10:10:20 -0800 Subject: [PATCH 4/8] Making changes based on feedback. --- ...ansform.cs => ThreadUnSafeICryptoTransformBad.cs} | 0 ...ormFix.cs => ThreadUnSafeICryptoTransformGood.cs} | 0 .../Likely Bugs/ThreadUnsafeICryptoTransform.qhelp | 4 ++-- .../src/Likely Bugs/ThreadUnsafeICryptoTransform.ql | 12 ++---------- .../ThreadUnsafeICryptoTransform.cs | 10 +++++----- .../ThreadUnsafeICryptoTransform.expected | 3 +-- 6 files changed, 10 insertions(+), 19 deletions(-) rename csharp/ql/src/Likely Bugs/{ThreadUnsafeICryptoTransform.cs => ThreadUnSafeICryptoTransformBad.cs} (100%) rename csharp/ql/src/Likely Bugs/{ThreadUnSafeICryptoTransformFix.cs => ThreadUnSafeICryptoTransformGood.cs} (100%) diff --git a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.cs b/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformBad.cs similarity index 100% rename from csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.cs rename to csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformBad.cs diff --git a/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformFix.cs b/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformGood.cs similarity index 100% rename from csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformFix.cs rename to csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformGood.cs diff --git a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp index 0f1ae02868d0..1e71a53840ee 100644 --- a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp +++ b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp @@ -16,10 +16,10 @@

    This example demonstrates the dangers of using a static System.Security.Cryptography.ICryptoTransform in such a way that the results may be incorrect.

    - +

    A simple fix is to change the _sha field from being a static member to an instance one by removing the static keyword.

    - +     diff --git a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.ql b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.ql index 6db9fad3b45b..e63cfa66c1e1 100644 --- a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.ql +++ b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.ql @@ -23,13 +23,13 @@ class ICryptoTransform extends Class { predicate usesICryptoTransformType( Type t ) { exists( ICryptoTransform ict | ict = t - or usesICryptoTransformType( t.getAChild*() ) + or usesICryptoTransformType( t.getAChild() ) ) } predicate hasICryptoTransformMember( Class c) { exists( Field f | - f = c.getAMember*() + f = c.getAMember() and ( exists( ICryptoTransform ict | ict = f.getType() ) or hasICryptoTransformMember(f.getType()) @@ -75,14 +75,6 @@ predicate hasICryptoTransformStaticMember( Class c, string msg) { ) ) ) - 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." ) diff --git a/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs b/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs index 90214fc988a2..0983c9c768db 100644 --- a/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs +++ b/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs @@ -66,11 +66,6 @@ public class StaticMember private static SHA1 _sha1 = SHA1.Create(); } -public class IndirectStatic -{ - StaticMember tc; -} - public class IndirectStatic2 { static Nest02 _n = new Nest02(); @@ -80,6 +75,11 @@ public class IndirectStatic2 /// Should not be flagged (thread safe) /// +public class IndirectStatic +{ + StaticMember tc; +} + public class TokenCacheFP { /// diff --git a/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected b/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected index e9ca1952f635..defba765fa98 100644 --- a/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected +++ b/csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected @@ -2,5 +2,4 @@ | 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 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. | +| 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. | From f8ae56a27cfd7e8896b5dd135bbb3021a518471d Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Tue, 26 Feb 2019 16:22:39 -0800 Subject: [PATCH 5/8] Improving documentation --- csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp index 1e71a53840ee..b72fa19140e3 100644 --- a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp +++ b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp @@ -4,6 +4,11 @@

    Classes that implement System.Security.Cryptography.ICryptoTransform are not thread safe.

    +

    The root cause of the problem is because of the way these class are implemented using Microsoft CAPI/CNG patterns.

    +

    For example, a hash class implementing this interface, typically there would be an instance-specific hash object (i.e. using BCryptCreateHash function), which can be called multiple times to add data to the hash (i.e. BCryptHashData), and finally calling the function that would finish the hash & get back the data (i.e. BCryptFinishHash).

    +

    The implementation would potentially allow the same hash object to be called with data from multiple threads before calling the finish function, thus leading to potentially incorrect results.

    +

    Because of this pattern, you can expect that, if you have multiple threads hashing "abc" on a static hash object, you may occasionally obtain the results (incorrectly) for hashing "abcabc", or face other unexpected behavior.

    +

    It is very unlikely somebody outside Microsoft would write a class that implements ICryptoTransform, and even if they do, it is likely that they will follow the same common pattern than the existing classes implementing this interface.

    Any object that implements System.Security.Cryptography.ICryptoTransform should not be used in concurrent threads as the instance members of such object are also not thread safe.

    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.

    From 1ae18974d89f95408c76b75efc7c9ffaeed781fb Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Wed, 27 Feb 2019 18:41:23 -0800 Subject: [PATCH 6/8] Fixing bugs found during Code Review. --- .gitignore | 3 +++ .../ThreadUnSafeICryptoTransformBad.cs | 11 ++++------- .../ThreadUnSafeICryptoTransformGood.cs | 11 +++++++---- .../ThreadUnsafeICryptoTransform.qhelp | 17 ++++++++--------- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index 7e82b2f488ca..c614f8f91592 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,6 @@ /.vs/ql/v15/Browse.VC.db /.vs/ProjectSettings.json +/.vs/ql_ICryptoTransform/v15/Browse.VC.opendb +/.vs/ql_ICryptoTransform/v15/Browse.VC.db +/.vs/ql_ICryptoTransform/v15/.suo diff --git a/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformBad.cs b/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformBad.cs index dbbc3586f981..b698abdb600d 100644 --- a/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformBad.cs +++ b/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformBad.cs @@ -1,9 +1,6 @@ -internal class TokenCacheThreadUnsafeICryptoTransformDemoFixed +internal class TokenCacheThreadUnsafeICryptoTransformDemo { - // We are replacing the static SHA256 field with an instance one - // - //private static SHA256 _sha = SHA256.Create(); - private SHA256 _sha = SHA256.Create(); + private static SHA256 _sha = SHA256.Create(); public string ComputeHash(string data) { @@ -21,8 +18,8 @@ static void Main(string[] args) Action action = (object obj) => { - var safeObj = new TokenCacheThreadUnsafeICryptoTransformDemoFixed(); - if (safeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=") + var unsafeObj = new TokenCacheThreadUnsafeICryptoTransformDemo(); + if (unsafeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=") { Console.WriteLine("**** We got incorrect Results!!! ****"); } diff --git a/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformGood.cs b/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformGood.cs index b698abdb600d..dbbc3586f981 100644 --- a/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformGood.cs +++ b/csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformGood.cs @@ -1,6 +1,9 @@ -internal class TokenCacheThreadUnsafeICryptoTransformDemo +internal class TokenCacheThreadUnsafeICryptoTransformDemoFixed { - private static SHA256 _sha = SHA256.Create(); + // 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) { @@ -18,8 +21,8 @@ static void Main(string[] args) Action action = (object obj) => { - var unsafeObj = new TokenCacheThreadUnsafeICryptoTransformDemo(); - if (unsafeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=") + var safeObj = new TokenCacheThreadUnsafeICryptoTransformDemoFixed(); + if (safeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=") { Console.WriteLine("**** We got incorrect Results!!! ****"); } diff --git a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp index b72fa19140e3..23046f5217a4 100644 --- a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp +++ b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp @@ -4,23 +4,22 @@

    Classes that implement System.Security.Cryptography.ICryptoTransform are not thread safe.

    -

    The root cause of the problem is because of the way these class are implemented using Microsoft CAPI/CNG patterns.

    -

    For example, a hash class implementing this interface, typically there would be an instance-specific hash object (i.e. using BCryptCreateHash function), which can be called multiple times to add data to the hash (i.e. BCryptHashData), and finally calling the function that would finish the hash & get back the data (i.e. BCryptFinishHash).

    -

    The implementation would potentially allow the same hash object to be called with data from multiple threads before calling the finish function, thus leading to potentially incorrect results.

    -

    Because of this pattern, you can expect that, if you have multiple threads hashing "abc" on a static hash object, you may occasionally obtain the results (incorrectly) for hashing "abcabc", or face other unexpected behavior.

    -

    It is very unlikely somebody outside Microsoft would write a class that implements ICryptoTransform, and even if they do, it is likely that they will follow the same common pattern than the existing classes implementing this interface.

    +

    This problem is caused by the way these classes are implemented using Microsoft CAPI/CNG patterns.

    +

    For example, when a hash class implements this interface, there would typically be an instance-specific hash object created (for example using BCryptCreateHash function). This object can be called multiple times to add data to the hash (for example BCryptHashData). Finally, a function is called that finishes the hash and returns the data (for example BCryptFinishHash).

    +

    Allowing the same hash object to be called with data from multiple threads before calling the finish function could potentially lead to incorrect results.

    +

    For example, if you have multiple threads hashing "abc" on a static hash object, you may occasionally obtain the results (incorrectly) for hashing "abcabc", or face other unexpected behavior.

    +

    It is very unlikely somebody outside Microsoft would write a class that implements ICryptoTransform, and even if they do, it is likely that they will follow the same common pattern as the existing classes implementing this interface.

    Any object that implements System.Security.Cryptography.ICryptoTransform should not be used in concurrent threads as the instance members of such object are also not thread safe.

    -

    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.

    +

    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.

     -

    Verify that the object is not being shared across threads.

    -

    If it is shared accross instances. Consider changing the code to use a non-static object of type System.Security.Cryptography.ICryptoTransform instead.

    +

    If the object is shared across instances, you should consider changing the code to use a non-static object of type System.Security.Cryptography.ICryptoTransform instead.

    As an alternative, you could also look into using ThreadStatic attribute, but make sure you read the initialization remarks on the documentation.

     -

    This example demonstrates the dangers of using a static System.Security.Cryptography.ICryptoTransform in such a way that the results may be incorrect.

    +

    This example demonstrates the dangers of using a static System.Security.Cryptography.ICryptoTransform in a way that generates incorrect results.

    A simple fix is to change the _sha field from being a static member to an instance one by removing the static keyword.

    From e24ca8ec40407ecdb4ec0c7cfefe85503f313d77 Mon Sep 17 00:00:00 2001 From: Raul Garcia <42392023+raulgarciamsft@users.noreply.github.com> Date: Wed, 27 Feb 2019 18:43:33 -0800 Subject: [PATCH 7/8] Update .gitignore --- .gitignore | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.gitignore b/.gitignore index c614f8f91592..4b055e55a091 100644 --- a/.gitignore +++ b/.gitignore @@ -12,7 +12,3 @@ /.vs/ql/v15/Browse.VC.opendb /.vs/ql/v15/Browse.VC.db /.vs/ProjectSettings.json - -/.vs/ql_ICryptoTransform/v15/Browse.VC.opendb -/.vs/ql_ICryptoTransform/v15/Browse.VC.db -/.vs/ql_ICryptoTransform/v15/.suo From 9eca21cb5a67443856ce3fb03d9bafd31a3e4ed2 Mon Sep 17 00:00:00 2001 From: Raul Garcia <42392023+raulgarciamsft@users.noreply.github.com> Date: Wed, 27 Feb 2019 18:43:51 -0800 Subject: [PATCH 8/8] Update .gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 4b055e55a091..7e82b2f488ca 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,4 @@ /.vs/ql/v15/Browse.VC.opendb /.vs/ql/v15/Browse.VC.db /.vs/ProjectSettings.json +