From e1a361f407c5067afe231fb2ce39801a54772a9f Mon Sep 17 00:00:00 2001 From: Denis Levin Date: Mon, 22 Jul 2019 16:20:10 -0700 Subject: [PATCH] CS: Adding DecryptWithoutHash.ql ("padding oracles") and CertificateValidationDisabled.ql queries (as is) --- .../CWE-295/CertificateValidationDisabled.ql | 50 +++++ .../CWE-327/DecryptWithoutHash.ql | 64 +++++++ .../CertificateValidationDisabled.expected | 7 + .../CertificateValidationDisabled.qlref | 1 + .../CertificateValidationDisabled/Test.cs | 82 ++++++++ .../DecryptWithoutHash.expected | 1 + .../DecryptWithoutHash.qlref | 1 + .../CWE-327/DecryptWithoutHash/Test.cs | 177 ++++++++++++++++++ 8 files changed, 383 insertions(+) create mode 100644 csharp/ql/src/Security Features/CWE-295/CertificateValidationDisabled.ql create mode 100644 csharp/ql/src/Security Features/CWE-327/DecryptWithoutHash.ql create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/CertificateValidationDisabled.expected create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/CertificateValidationDisabled.qlref create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/Test.cs create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/DecryptWithoutHash.expected create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/DecryptWithoutHash.qlref create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/Test.cs diff --git a/csharp/ql/src/Security Features/CWE-295/CertificateValidationDisabled.ql b/csharp/ql/src/Security Features/CWE-295/CertificateValidationDisabled.ql new file mode 100644 index 000000000000..c1b32be49eb5 --- /dev/null +++ b/csharp/ql/src/Security Features/CWE-295/CertificateValidationDisabled.ql @@ -0,0 +1,50 @@ +/** + * @name Do not disable certificate validation + * @description Do not force ServerCertificateValidationCallback to always return 'true'. + * @kind problem + * @id cs/do-not-disable-cert-validation + * @problem.severity error + * @precision high + * @tags security + * external/cwe/cwe-295 + */ +import csharp +/* + * Lambda examples: + * ServicePointManager.ServerCertificateValidationCallback += (a, b, c, d) => { return true; }; + * ServicePointManager.ServerCertificateValidationCallback += (a, b, c, d) => true; + * Anonymous method example: + * ServicePointManager.ServerCertificateValidationCallback += delegate { return true; }; + * Delegate creation example: + * ServicePointManager.ServerCertificateValidationCallback = new RemoteCertificateValidationCallback(AcceptAllCertifications); + */ +from Assignment a, PropertyWrite leftExpr, Callable targetCallable, Property p +where leftExpr = a.getLValue() // Hook up the basic fields + and leftExpr.getTarget() = p + and p.getDeclaringType().hasQualifiedName("System.Net", "ServicePointManager") + and p.getName() = "ServerCertificateValidationCallback" + // Find the target callable being assigned to the ServerCertificateValidationCallback + and exists(Expr assignedValue | + assignedValue = a.getRValue() | + // Either the assigned value is an AnonymousFunctionExpr, such as a lambda or anonymous method expr + targetCallable = assignedValue or + // Or the assigned value is a DelegateCreation + exists(Expr delegateCreationArg | + delegateCreationArg = assignedValue.(DelegateCreation).getArgument() | + // And the argument is an AnonymousFunctionExpr + targetCallable = delegateCreationArg or + // Or the argument is an access to a callable + targetCallable = delegateCreationArg.(CallableAccess).getTarget() + ) + ) + // If the target callable returns true, validation is disabled + and( + exists(ReturnStmt rs, BlockStmt bs | + bs = targetCallable.getBody() and + bs.getNumberOfChildren() = 1 and + bs.getChild(0) = rs and + rs.getExpr().(BoolLiteral).getBoolValue() = true + ) or + targetCallable.getBody().(BoolLiteral).getBoolValue() = true + ) +select a, "Certificate validation disabled." diff --git a/csharp/ql/src/Security Features/CWE-327/DecryptWithoutHash.ql b/csharp/ql/src/Security Features/CWE-327/DecryptWithoutHash.ql new file mode 100644 index 000000000000..2240ea027df8 --- /dev/null +++ b/csharp/ql/src/Security Features/CWE-327/DecryptWithoutHash.ql @@ -0,0 +1,64 @@ +/** + * @name Verify hash before decryption + * @description + * @kind problem + * @id cs/verify-hash-before-decryption + * @problem.severity error + * @precision medium + * @tags security + */ +import csharp +import semmle.code.csharp.dataflow.TaintTracking::TaintTracking + +class CryptoStreamWrite extends MethodCall { + CryptoStreamWrite() { + getTarget().hasQualifiedName("System.Security.Cryptography.CryptoStream", "Write") + } + + /** Holds if this is a decryptor stream. */ + predicate isDecryptor() { + any(CryptoStreamTaintTrackingConfig c).hasFlow(_, DataFlow::exprNode(getQualifier())) + } +} + +class ComputeHash extends MethodCall { + ComputeHash() { + getTarget().hasName("ComputeHash") + } +} + +class ByteArrayTaintTrackingConfig extends TaintTracking::Configuration { + ByteArrayTaintTrackingConfig() { this = "Byte array that is being passed to cryptostream write" } + + override predicate isSource(DataFlow::Node source) { + source.asExpr().getType().(ArrayType).getElementType().hasName("Byte") + } + + override predicate isSink(DataFlow::Node sink) { + sink.asExpr() = any(CryptoStreamWrite cw).getArgument(0) or + sink.asExpr() = any(ComputeHash ch).getArgument(0) + } +} + +class CryptoStreamTaintTrackingConfig extends TaintTracking::Configuration { + CryptoStreamTaintTrackingConfig() { this = "CryptoStreamTaintTracking" } + + override predicate isSource(DataFlow::Node source) { + exists(ObjectCreation oc | + oc = source.asExpr() and + oc.getTarget().getDeclaringType().hasName("CryptoStream") and + oc.getArgument(1).(MethodCall).getTarget().hasQualifiedName("System.Security.Cryptography.SymmetricAlgorithm", "CreateDecryptor") + ) + } + + override predicate isSink(DataFlow::Node sink) { + sink.asExpr() = any(CryptoStreamWrite csw).getQualifier() + } +} + +from ByteArrayTaintTrackingConfig config, Expr arrayDefinition, CryptoStreamWrite writeCall +where config.hasFlow(DataFlow::exprNode(arrayDefinition), DataFlow::exprNode(writeCall.getArgument(0))) + and writeCall.isDecryptor() + and not exists(ComputeHash c | config.hasFlow(DataFlow::exprNode(arrayDefinition), DataFlow::exprNode(c.getArgument(0)))) +select arrayDefinition, "Byte array passed to CryptoStream $@, never has the hash computed.", arrayDefinition, "here" + \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/CertificateValidationDisabled.expected b/csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/CertificateValidationDisabled.expected new file mode 100644 index 000000000000..be66b9189474 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/CertificateValidationDisabled.expected @@ -0,0 +1,7 @@ +| Test.cs:20:13:20:129 | ... += ... | Certificate validation disabled. | +| Test.cs:21:13:21:103 | ... += ... | Certificate validation disabled. | +| Test.cs:22:13:22:91 | ... += ... | Certificate validation disabled. | +| Test.cs:23:13:23:96 | ... += ... | Certificate validation disabled. | +| Test.cs:26:13:26:134 | ... = ... | Certificate validation disabled. | +| Test.cs:29:13:29:145 | ... = ... | Certificate validation disabled. | +| Test.cs:69:13:69:148 | ... = ... | Certificate validation disabled. | diff --git a/csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/CertificateValidationDisabled.qlref b/csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/CertificateValidationDisabled.qlref new file mode 100644 index 000000000000..1244b1a00364 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/CertificateValidationDisabled.qlref @@ -0,0 +1 @@ +Security Features/CWE-295/CertificateValidationDisabled.ql \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/Test.cs b/csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/Test.cs new file mode 100644 index 000000000000..7f74e457edbb --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-295/CertificateValidationDisabled/Test.cs @@ -0,0 +1,82 @@ +// semmle-extractor-options: /nostdlib /noconfig /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\mscorlib.dll /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\System.dll /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\System.Net.dll /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\System.Net.Http.dll /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\System.Linq.dll +using System; +using System.Collections.Generic; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Net.Http.Headers; +using System.Net.Security; +using System.Security.Cryptography.X509Certificates; +using System.Text; +using System.Threading.Tasks; + +namespace SemmleTestCertValidation +{ + class Program + { + static void Main(string[] args) + { + // Bad (Simple) + ServicePointManager.ServerCertificateValidationCallback += (sender, cert, chain, sslPolicyErrors) => { return true; }; + ServicePointManager.ServerCertificateValidationCallback += (a, b, c, d) => { return true; }; + ServicePointManager.ServerCertificateValidationCallback += (a, b, c, d) => true; + ServicePointManager.ServerCertificateValidationCallback += delegate { return true; }; + + // Bad (Simple Function) + ServicePointManager.ServerCertificateValidationCallback = new RemoteCertificateValidationCallback(AcceptAllCertifications); + + var m = new Program(); + ServicePointManager.ServerCertificateValidationCallback = new RemoteCertificateValidationCallback(m.AcceptAllCertificationsNonStatic); + m.Test(); + + // Don't know + ServicePointManager.ServerCertificateValidationCallback += (a, b, c, d) => { + int x = 20; + if (x < 100) + { + return false; + } + else + { + return true; + } + }; + + // Good + ServicePointManager.ServerCertificateValidationCallback += delegate ( + object sender, + X509Certificate cert, + X509Chain chain, + SslPolicyErrors sslPolicyErrors) + { + if (cert == null) + { + return false; + } + + if (sslPolicyErrors != SslPolicyErrors.None) + { + return false; + } + + return true; + }; + } + + void Test() + { + // BAD + ServicePointManager.ServerCertificateValidationCallback = new RemoteCertificateValidationCallback(this.AcceptAllCertificationsNonStatic); + } + + public static bool AcceptAllCertifications(object sender, X509Certificate certification, X509Chain chain, SslPolicyErrors sslPolicyErrors) + { + return true; + } + + public bool AcceptAllCertificationsNonStatic(object sender, X509Certificate certification, X509Chain chain, SslPolicyErrors sslPolicyErrors) + { + return true; + } + } +} diff --git a/csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/DecryptWithoutHash.expected b/csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/DecryptWithoutHash.expected new file mode 100644 index 000000000000..d40b580c5507 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/DecryptWithoutHash.expected @@ -0,0 +1 @@ +| Test.cs:156:31:156:40 | access to parameter cipherText | Byte array passed to CryptoStream $@, never has the hash computed. | Test.cs:156:31:156:40 | access to parameter cipherText | here | \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/DecryptWithoutHash.qlref b/csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/DecryptWithoutHash.qlref new file mode 100644 index 000000000000..955a551440f7 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/DecryptWithoutHash.qlref @@ -0,0 +1 @@ +Security Features/CWE-327/DecryptWithoutHash.ql \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/Test.cs b/csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/Test.cs new file mode 100644 index 000000000000..0fc3f468edb8 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-327/DecryptWithoutHash/Test.cs @@ -0,0 +1,177 @@ +// semmle-extractor-options: /r:System.Security.Cryptography.Algorithms.dll /r:System.Security.Cryptography.Primitives.dll +using System; +using System.IO; +using System.Security.Cryptography; +using System.Text; +namespace EncryptWithoutHash +{ + public class Class1 + { + int Rfc2898KeygenIterations = 100; + int AesKeySizeInBits = 128; + byte[] Salt = new byte[] { 0x15, 0xA0, 0xC3, 0x86, 0x52, 0x03, 0xF2, 0xFB, 0xD2, 0x99, 0xEA, 0xBC, 0x3D, 0xBB, 0xC1, 0xF8 }; + + byte[] hmacKey = new byte[] { 0x3A, 0x6C, 0xC8, 0x4E, 0x36, 0x1D, 0x15, 0x4A, 0x93, 0xBB, 0xD5, 0x49, 0xBB, 0x0D, 0x33, 0x15, 0xA0, 0xC3, 0x86, 0x52, 0x03, 0xF2, 0xFB, 0xD2, 0x99, 0xEA, 0xBC, 0x3D, 0xBB, 0xC1, 0xF8, 0x53, + 0x3A, 0x6C, 0xC8, 0x4E, 0x36, 0x1D, 0x15, 0x4A, 0x93, 0xBB, 0xD5, 0x49, 0xBB, 0x0D, 0x33, 0x15, 0xA0, 0xC3, 0x86, 0x52, 0x03, 0xF2, 0xFB, 0xD2, 0x99, 0xEA, 0xBC, 0x3D, 0xBB, 0xC1, 0xF8, 0x53}; + + public byte[] Enrycpt(string plainText, string password) + { + byte[] rawPlaintext = Encoding.Unicode.GetBytes(plainText); + byte[] cipherText = null; + byte[] hmac; + using (Aes aes = new AesManaged()) + { + aes.Padding = PaddingMode.PKCS7; + aes.Mode = CipherMode.CBC; + aes.KeySize = AesKeySizeInBits; + int KeyStrengthInBytes = aes.KeySize / 8; + Rfc2898DeriveBytes rfc2898 = new Rfc2898DeriveBytes(password, Salt, Rfc2898KeygenIterations); + aes.Key = rfc2898.GetBytes(KeyStrengthInBytes); + aes.IV = rfc2898.GetBytes(KeyStrengthInBytes); + using (MemoryStream ms = new MemoryStream()) + { + using (CryptoStream cs = new CryptoStream(ms, aes.CreateEncryptor(), CryptoStreamMode.Write)) + { + cs.Write(rawPlaintext, 0, rawPlaintext.Length); + } + + cipherText = ms.ToArray(); + HMACSHA256 hmacsha256 = new HMACSHA256(hmacKey); + hmac = hmacsha256.ComputeHash(cipherText); + } + + Console.WriteLine("HMACSHA256: {0}", BytesToString(hmac)); + return cipherText; + } + } + + public byte[] EnrycptHashExternal(string plainText, string password) + { + byte[] rawPlaintext = Encoding.Unicode.GetBytes(plainText); + byte[] cipherText = null; + byte[] hmac; + using (Aes aes = new AesManaged()) + { + aes.Padding = PaddingMode.PKCS7; + aes.Mode = CipherMode.CBC; + aes.KeySize = AesKeySizeInBits; + int KeyStrengthInBytes = aes.KeySize / 8; + Rfc2898DeriveBytes rfc2898 = new Rfc2898DeriveBytes(password, Salt, Rfc2898KeygenIterations); + aes.Key = rfc2898.GetBytes(KeyStrengthInBytes); + aes.IV = rfc2898.GetBytes(KeyStrengthInBytes); + using (MemoryStream ms = new MemoryStream()) + { + using (CryptoStream cs = new CryptoStream(ms, aes.CreateEncryptor(), CryptoStreamMode.Write)) + { + cs.Write(rawPlaintext, 0, rawPlaintext.Length); + } + + cipherText = ms.ToArray(); + hmac = GetHash(cipherText); + } + + Console.WriteLine("HMACSHA256: {0}", BytesToString(hmac)); + return cipherText; + } + } + + public byte[] EnrycptWithoutHash(string plainText, string password) + { + byte[] rawPlaintext = Encoding.Unicode.GetBytes(plainText); + byte[] cipherText = null; + using (Aes aes = new AesManaged()) + { + aes.Padding = PaddingMode.PKCS7; + aes.Mode = CipherMode.CBC; + aes.KeySize = AesKeySizeInBits; + int KeyStrengthInBytes = aes.KeySize / 8; + Rfc2898DeriveBytes rfc2898 = new Rfc2898DeriveBytes(password, Salt, Rfc2898KeygenIterations); + aes.Key = rfc2898.GetBytes(KeyStrengthInBytes); + aes.IV = rfc2898.GetBytes(KeyStrengthInBytes); + using (MemoryStream ms = new MemoryStream()) + { + using (CryptoStream cs = new CryptoStream(ms, aes.CreateEncryptor(), CryptoStreamMode.Write)) + { + cs.Write(rawPlaintext, 0, rawPlaintext.Length); + } + + cipherText = ms.ToArray(); + } + + return cipherText; + } + } + + public byte[] GetHash(byte[] input) + { + HMACSHA256 hmacsha256 = new HMACSHA256(hmacKey); + return hmacsha256.ComputeHash(input); + } + + public string Decrypt(byte[] cipherText, string password) + { + byte[] rawPlaintext; + byte[] hmac; + using (Aes aes = new AesManaged()) + { + aes.Padding = PaddingMode.PKCS7; + aes.KeySize = AesKeySizeInBits; + int KeyStrengthInBytes = aes.KeySize / 8; + Rfc2898DeriveBytes rfc2898 = new Rfc2898DeriveBytes(password, Salt, Rfc2898KeygenIterations); + aes.Key = rfc2898.GetBytes(KeyStrengthInBytes); + aes.IV = rfc2898.GetBytes(KeyStrengthInBytes); + using (MemoryStream ms = new MemoryStream()) + { + using (CryptoStream cs = new CryptoStream(ms, aes.CreateDecryptor(), CryptoStreamMode.Write)) + { + cs.Write(cipherText, 0, cipherText.Length); + } + + HMACSHA256 hmacsha256 = new HMACSHA256(hmacKey); + hmac = hmacsha256.ComputeHash(cipherText); + + rawPlaintext = ms.ToArray(); + } + + Console.WriteLine("HMACSHA256: {0}", BytesToString(hmac)); + return Encoding.Unicode.GetString(rawPlaintext); + } + } + + public string DecryptWithoutHash(byte[] cipherText, string password) + { + byte[] rawPlaintext; + using (Aes aes = new AesManaged()) + { + aes.Padding = PaddingMode.PKCS7; + aes.KeySize = AesKeySizeInBits; + int KeyStrengthInBytes = aes.KeySize / 8; + Rfc2898DeriveBytes rfc2898 = new Rfc2898DeriveBytes(password, Salt, Rfc2898KeygenIterations); + aes.Key = rfc2898.GetBytes(KeyStrengthInBytes); + aes.IV = rfc2898.GetBytes(KeyStrengthInBytes); + using (MemoryStream ms = new MemoryStream()) + { + using (CryptoStream cs = new CryptoStream(ms, aes.CreateDecryptor(), CryptoStreamMode.Write)) + { + cs.Write(cipherText, 0, cipherText.Length); + } + + rawPlaintext = ms.ToArray(); + } + + return Encoding.Unicode.GetString(rawPlaintext); + } + } + + public string BytesToString(byte[] byteArray) + { + StringBuilder str = new StringBuilder(); + foreach (byte b in byteArray) + { + str.Append(b.ToString("X2")); + } + + return str.ToString(); + } + } +}