Enhance the query to check more scenarios

2026-04-30 19:26:02 +02:00 · 2021-06-14 03:24:16 +00:00
parent 6bfe2f2ba6
commit 6a2c7d54cd
4 changed files with 230 additions and 44 deletions
--- a/Features/CWE-759/HashWithoutSalt.ql
+++ b/Features/CWE-759/HashWithoutSalt.ql
@@ -33,8 +33,9 @@ class KeyedHashAlgorithm extends RefType {
 }

 /**
- * The method `ComputeHash()`, `ComputeHashAsync`, `TryComputeHash`, `HashData`, or `TryHashData` declared in `System.Security.Cryptography.HashAlgorithm` and
- * the method `HashData()` declared in `Windows.Security.Cryptography.Core.HashAlgorithmProvider`.
+ * The method `ComputeHash()`, `ComputeHashAsync`, `TryComputeHash`, `HashData`, or
+ * `TryHashData` declared in `System.Security.Cryptography.HashAlgorithm` and the method
+ * `HashData()` declared in `Windows.Security.Cryptography.Core.HashAlgorithmProvider`.
 */
 class HashMethod extends Method {
  HashMethod() {
@@ -46,8 +47,11 @@ class HashMethod extends Method {
  }
 }

-/** Gets a regular expression for matching common names of variables that indicate the value being held is a password. */
-string getPasswordRegex() { result = "(?i).*pass(wd|word|code|phrase).*" }
+/**
+ * Gets a regular expression for matching common names of variables that indicate the
+ * value being held is a password.
+ */
+string getPasswordRegex() { result = "(?i)pass(wd|word|code|phrase)" }

 /** Finds variables that hold password information judging by their names. */
 class PasswordVarExpr extends Expr {
@@ -56,21 +60,65 @@ class PasswordVarExpr extends Expr {
  }
 }

-/** Holds if there is another hashing method call. */
-predicate hasAnotherHashCall(MethodCall mc) {
-  exists(Call mc2, DataFlow2::Node src, DataFlow2::Node sink |
-    (
-      mc2.getTarget() instanceof HashMethod or
-      mc2.(ObjectCreation).getObjectType().getABaseType+() instanceof HashAlgorithm
-    ) and
-    mc2 != mc and
-    src.asExpr() = mc.getAReachableElement() and
-    sink.asExpr() = mc2.getAChildExpr() and
-    TaintTracking2::localTaint(src, sink)
+/**
+ * Holds if `mc` is a hashing method call or invokes a hashing method call
+ * directly or indirectly.
+ */
+predicate isHashCall(MethodCall mc) {
+  mc.getTarget() instanceof HashMethod
+  or
+  exists(MethodCall mcc |
+    mc.getTarget().calls(mcc.getTarget()) and
+    isHashCall(mcc) and
+    DataFlow::localExprFlow(mc.getTarget().getAParameter().getAnAccess(), mcc.getAnArgument())
  )
 }

-/** Taint configuration tracking flow from an expression whose name suggests it holds password data to a method call that generates a hash without a salt. */
+/** Holds if there is another hashing method call. */
+predicate hasAnotherHashCall(MethodCall mc) {
+  exists(MethodCall mc2, DataFlow2::Node src, DataFlow2::Node sink |
+    isHashCall(mc2) and
+    mc2 != mc and
+    (
+      src.asExpr() = mc.getQualifier() or
+      src.asExpr() = mc.getAnArgument() or
+      src.asExpr() = mc
+    ) and
+    (
+      sink.asExpr() = mc2.getQualifier() or
+      sink.asExpr() = mc2.getAnArgument()
+    ) and
+    DataFlow::localFlow(src, sink)
+  )
+}
+
+/** Holds if a password hash without salt is further processed in another method call. */
+predicate hasFurtherProcessing(MethodCall mc) {
+  mc.getTarget().fromLibrary() and
+  (
+    mc.getTarget().hasQualifiedName("System.Array", "Copy") or // Array.Copy(passwordHash, 0, password.Length), 0, key, 0, keyLen);
+    mc.getTarget().hasQualifiedName("System.String", "Concat") or // string.Concat(passwordHash, saltkey)
+    mc.getTarget().hasQualifiedName("System.Buffer", "BlockCopy") or // Buffer.BlockCopy(passwordHash, 0, allBytes, 0, 20)
+    mc.getTarget().hasQualifiedName("System.String", "Format") // String.Format("{0}:{1}:{2}", username, salt, password)
+  )
+}
+
+/**
+ * Holds if `mc` is part of a call graph that satisfies `isHashCall` but is not at the
+ * top of the call hierarchy.
+ */
+predicate hasHashAncestor(MethodCall mc) {
+  exists(MethodCall mpc |
+    mpc.getTarget().calls(mc.getTarget()) and
+    isHashCall(mpc) and
+    DataFlow::localExprFlow(mpc.getTarget().getAParameter().getAnAccess(), mc.getAnArgument())
+  )
+}
+
+/**
+ * Taint configuration tracking flow from an expression whose name suggests it holds
+ * password data to a method call that generates a hash without a salt.
+ */
 class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
  HashWithoutSaltConfiguration() { this = "HashWithoutSaltConfiguration" }

@@ -79,8 +127,23 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
  override predicate isSink(DataFlow::Node sink) {
    exists(MethodCall mc |
      sink.asExpr() = mc.getArgument(0) and
-      mc.getTarget() instanceof HashMethod and
-      not hasAnotherHashCall(mc)
+      isHashCall(mc) and
+      not hasAnotherHashCall(mc) and
+      not hasHashAncestor(mc) and
+      not exists(MethodCall mmc |
+        hasFurtherProcessing(mmc) and
+        DataFlow::localExprFlow(mc, mmc.getAnArgument())
+      ) and
+      not exists(Call c |
+        (
+          c.getTarget().getDeclaringType().getABaseType*() instanceof HashAlgorithm or
+          c.getTarget()
+              .getDeclaringType()
+              .getABaseType*()
+              .hasQualifiedName("System.Security.Cryptography", "DeriveBytes")
+        ) and
+        DataFlow::localExprFlow(mc, c.getAnArgument())
+      )
    )
  }

@@ -104,13 +167,7 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
   */
  override predicate isSanitizer(DataFlow::Node node) {
    exists(MethodCall mc |
-      mc.getTarget().fromLibrary() and
-      (
-        mc.getTarget().hasQualifiedName("System.Array", "CopyTo") or // passBytes.CopyTo(rawSalted, 0)
-        mc.getTarget().hasQualifiedName("System.String", "Concat") or // string.Concat(password, saltkey)
-        mc.getTarget().hasQualifiedName("System.Buffer", "BlockCopy") or // Buffer.BlockCopy(salt, 0, allBytes, 0, 20)
-        mc.getTarget().hasQualifiedName("System.String", "Format") // String.Format("{0}:{1}:{2}", username, salt, password)
-      ) and
+      hasFurtherProcessing(mc) and
      mc.getAnArgument() = node.asExpr()
    )
    or
@@ -118,12 +175,19 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
    or
    exists(InterpolatedStringExpr e | node.asExpr() = e.getAnInsert())
    or
+    exists(Call c |
+      c.getTarget()
+          .getDeclaringType()
+          .getABaseType*()
+          .hasQualifiedName("System.Security.Cryptography", "DeriveBytes")
+    )
+    or
    // a salt or key is included in subclasses of `KeyedHashAlgorithm`
-    exists(MethodCall mc, Assignment ass, ObjectCreation oc |
-      ass.getRValue() = oc and
+    exists(MethodCall mc, Assignment a, ObjectCreation oc |
+      a.getRValue() = oc and
      oc.getObjectType().getABaseType+() instanceof KeyedHashAlgorithm and
      mc.getTarget() instanceof HashMethod and
-      ass.getLValue() = mc.getQualifier().(VariableAccess).getTarget().getAnAccess() and
+      a.getLValue() = mc.getQualifier().(VariableAccess).getTarget().getAnAccess() and
      mc.getArgument(0) = node.asExpr()
    )
  }
--- a/csharp/ql/test/experimental/Security
+++ b/csharp/ql/test/experimental/Security
@@ -1,6 +1,7 @@
 // semmle-extractor-options: /r:System.Security.Cryptography.Primitives.dll /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll

 using System;
+using System.Text;
 using System.Security.Cryptography;

 using Windows.Security.Cryptography;
@@ -130,4 +131,72 @@ public class Test
        }
        return hmacbytes;
    }
+
+    private byte[] TryDecrypt(byte[] buffer, int offset, int length, byte[] password, int keyLen) {
+        byte[] key = new byte[16];
+        Array.Copy(SHA1.Create().ComputeHash(password, 0, password.Length), 0, key, 0, keyLen);
+        byte[] ret = Aes.Create().CreateDecryptor(key, null).TransformFinalBlock(buffer, offset, length);
+        return ret;
+    }
+
+    // GOOD - Use password hash without a salt having further processing.
+    public byte[] encrypt(byte[] pass, byte[] salt, byte[] blob) {
+        byte[] key = new byte[salt.Length + pass.Length];
+        Array.Copy(salt, 0, key, 0, salt.Length);
+        Array.Copy(pass, 0, key, salt.Length, pass.Length);
+        byte[] pkb = TryDecrypt(blob, 8, blob.Length - 8, key, 16);
+        return pkb;
+    }
+
+    public string CreatePasswordHash(string password, string saltkey)
+    {
+        var saltAndPassword = string.Concat(password, saltkey);
+        HashAlgorithm algorithm = SHA256.Create();
+        var hashByteArray = algorithm.ComputeHash(Encoding.UTF8.GetBytes(saltAndPassword));
+        return BitConverter.ToString(hashByteArray).Replace("-", "");
+    }
+
+    private string GetMD5HashBinHex (string toBeHashed)
+    {
+        MD5 hash = MD5.Create ();
+        byte[] result = hash.ComputeHash (Encoding.ASCII.GetBytes (toBeHashed));
+
+        StringBuilder sb = new StringBuilder ();
+        foreach (byte b in result)
+            sb.Append (b.ToString ("x2"));
+        
+        return sb.ToString ();
+    }
+
+    // GOOD: Password concatenated with other information before hashing
+    public string CreatePasswordHash2(string username, string realm, string password)
+    {
+        string A1 = String.Format ("{0}:{1}:{2}", username, realm, password);
+
+        string HA1 = GetMD5HashBinHex (A1);
+        return HA1;
+    }
+
+    private byte[] Xor(byte[] array1, byte[] array2) {
+        var result = new byte[array1.Length];
+
+        for (int i = 0; i < array1.Length; i++) {
+            result[i] = (byte)(array1[i] ^ array2[i]);
+        }
+
+        return result;
+    }
+
+    // GOOD: Password hash without salt is further hashed with salt
+    public byte[] GetScrable(byte[] password, byte[] decodedSalt) {
+        var first20SaltBytes = new byte[20];
+        Array.Copy(decodedSalt, first20SaltBytes, 20);
+
+        var step1 = Sha1Utils.Hash(password);
+        var step2 = Sha1Utils.Hash(step1);
+        var step3 = Sha1Utils.Hash(first20SaltBytes, step2);
+        var scrambleBytes = Xor(step1, step3);
+
+        return scrambleBytes;
+    }
 }
--- a/Features/CWE-759/HashWithoutSalt.expected
+++ b/Features/CWE-759/HashWithoutSalt.expected
@@ -1,19 +1,19 @@
 edges
-| HashWithoutSalt.cs:17:70:17:77 | access to parameter password : String | HashWithoutSalt.cs:19:49:19:56 | access to local variable passBuff |
-| HashWithoutSalt.cs:37:28:37:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:38:51:38:59 | access to local variable passBytes |
-| HashWithoutSalt.cs:37:64:37:71 | access to parameter password : String | HashWithoutSalt.cs:37:28:37:72 | call to method GetBytes : Byte[] |
-| HashWithoutSalt.cs:69:28:69:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:70:48:70:56 | access to local variable passBytes |
-| HashWithoutSalt.cs:69:64:69:71 | access to parameter password : String | HashWithoutSalt.cs:69:28:69:72 | call to method GetBytes : Byte[] |
+| HashWithoutSalt.cs:18:70:18:77 | access to parameter password : String | HashWithoutSalt.cs:20:49:20:56 | access to local variable passBuff |
+| HashWithoutSalt.cs:38:28:38:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:39:51:39:59 | access to local variable passBytes |
+| HashWithoutSalt.cs:38:64:38:71 | access to parameter password : String | HashWithoutSalt.cs:38:28:38:72 | call to method GetBytes : Byte[] |
+| HashWithoutSalt.cs:70:28:70:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:71:48:71:56 | access to local variable passBytes |
+| HashWithoutSalt.cs:70:64:70:71 | access to parameter password : String | HashWithoutSalt.cs:70:28:70:72 | call to method GetBytes : Byte[] |
 nodes
-| HashWithoutSalt.cs:17:70:17:77 | access to parameter password : String | semmle.label | access to parameter password : String |
-| HashWithoutSalt.cs:19:49:19:56 | access to local variable passBuff | semmle.label | access to local variable passBuff |
-| HashWithoutSalt.cs:37:28:37:72 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] |
-| HashWithoutSalt.cs:37:64:37:71 | access to parameter password : String | semmle.label | access to parameter password : String |
-| HashWithoutSalt.cs:38:51:38:59 | access to local variable passBytes | semmle.label | access to local variable passBytes |
-| HashWithoutSalt.cs:69:28:69:72 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] |
-| HashWithoutSalt.cs:69:64:69:71 | access to parameter password : String | semmle.label | access to parameter password : String |
-| HashWithoutSalt.cs:70:48:70:56 | access to local variable passBytes | semmle.label | access to local variable passBytes |
+| HashWithoutSalt.cs:18:70:18:77 | access to parameter password : String | semmle.label | access to parameter password : String |
+| HashWithoutSalt.cs:20:49:20:56 | access to local variable passBuff | semmle.label | access to local variable passBuff |
+| HashWithoutSalt.cs:38:28:38:72 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] |
+| HashWithoutSalt.cs:38:64:38:71 | access to parameter password : String | semmle.label | access to parameter password : String |
+| HashWithoutSalt.cs:39:51:39:59 | access to local variable passBytes | semmle.label | access to local variable passBytes |
+| HashWithoutSalt.cs:70:28:70:72 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] |
+| HashWithoutSalt.cs:70:64:70:71 | access to parameter password : String | semmle.label | access to parameter password : String |
+| HashWithoutSalt.cs:71:48:71:56 | access to local variable passBytes | semmle.label | access to local variable passBytes |
 #select
-| HashWithoutSalt.cs:19:49:19:56 | access to local variable passBuff | HashWithoutSalt.cs:17:70:17:77 | access to parameter password : String | HashWithoutSalt.cs:19:49:19:56 | access to local variable passBuff | $@ is hashed without a salt. | HashWithoutSalt.cs:17:70:17:77 | access to parameter password | The password |
-| HashWithoutSalt.cs:38:51:38:59 | access to local variable passBytes | HashWithoutSalt.cs:37:64:37:71 | access to parameter password : String | HashWithoutSalt.cs:38:51:38:59 | access to local variable passBytes | $@ is hashed without a salt. | HashWithoutSalt.cs:37:64:37:71 | access to parameter password | The password |
-| HashWithoutSalt.cs:70:48:70:56 | access to local variable passBytes | HashWithoutSalt.cs:69:64:69:71 | access to parameter password : String | HashWithoutSalt.cs:70:48:70:56 | access to local variable passBytes | $@ is hashed without a salt. | HashWithoutSalt.cs:69:64:69:71 | access to parameter password | The password |
+| HashWithoutSalt.cs:20:49:20:56 | access to local variable passBuff | HashWithoutSalt.cs:18:70:18:77 | access to parameter password : String | HashWithoutSalt.cs:20:49:20:56 | access to local variable passBuff | $@ is hashed without a salt. | HashWithoutSalt.cs:18:70:18:77 | access to parameter password | The password |
+| HashWithoutSalt.cs:39:51:39:59 | access to local variable passBytes | HashWithoutSalt.cs:38:64:38:71 | access to parameter password : String | HashWithoutSalt.cs:39:51:39:59 | access to local variable passBytes | $@ is hashed without a salt. | HashWithoutSalt.cs:38:64:38:71 | access to parameter password | The password |
+| HashWithoutSalt.cs:71:48:71:56 | access to local variable passBytes | HashWithoutSalt.cs:70:64:70:71 | access to parameter password : String | HashWithoutSalt.cs:71:48:71:56 | access to local variable passBytes | $@ is hashed without a salt. | HashWithoutSalt.cs:70:64:70:71 | access to parameter password | The password |
--- a/csharp/ql/test/experimental/Security
+++ b/csharp/ql/test/experimental/Security
@@ -0,0 +1,53 @@
+using System;
+using System.Security.Cryptography;
+using System.Text;
+
+internal static class Sha1Utils
+{
+    public static byte[] Hash(string str)
+    {
+        var bytes = str == null ? new byte[0] : Encoding.UTF8.GetBytes(str);
+
+        return Hash(bytes);
+    }
+
+    public static byte[] Hash(byte[] bytes)
+    {
+        var sha1 = SHA1.Create();
+        var hashBytes = sha1.ComputeHash(bytes);
+
+        return hashBytes;
+    }
+
+    public static string HexStringFromBytes(byte[] bytes)
+    {
+        var sb = new StringBuilder();
+        foreach (var b in bytes)
+        {
+            var hex = b.ToString("x2");
+            sb.Append(hex);
+        }
+        return sb.ToString();
+    }
+
+    public static byte[] Hash(byte[] salt, byte[] str)
+    {
+        var salted = new byte[salt.Length + str.Length];
+        Array.Copy(salt, salted, salt.Length);
+        Array.Copy(str, 0, salted, salt.Length, str.Length);
+
+        return Hash(salted);
+    }
+
+    public static byte[] Xor(byte[] array1, byte[] array2)
+    {
+        var result = new byte[array1.Length];
+
+        for (int i = 0; i < array1.Length; i++)
+        {
+            result[i] = (byte)(array1[i] ^ array2[i]);
+        }
+
+        return result;
+    }
+}