Revamp the query to handle more cases

Author: luchua-bc

csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.ql

@@ -8,7 +8,9 @@
  */
 
 import csharp
+import semmle.code.csharp.dataflow.DataFlow2
 import semmle.code.csharp.dataflow.TaintTracking
+import semmle.code.csharp.dataflow.TaintTracking2
 import DataFlow::PathGraph
 
 /** The C# class `Windows.Security.Cryptography.Core.HashAlgorithmProvider`. */
@@ -23,6 +25,13 @@ class HashAlgorithm extends RefType {
   HashAlgorithm() { this.hasQualifiedName("System.Security.Cryptography", "HashAlgorithm") }
 }
 
+/** The C# class `System.Security.Cryptography.KeyedHashAlgorithm`. */
+class KeyedHashAlgorithm extends RefType {
+  KeyedHashAlgorithm() {
+    this.hasQualifiedName("System.Security.Cryptography", "KeyedHashAlgorithm")
+  }
+}
+
 /**
  * The method `ComputeHash()` declared in `System.Security.Cryptography.HashAlgorithm` and
  * the method `HashData()` declared in `Windows.Security.Cryptography.Core.HashAlgorithmProvider`.
@@ -47,6 +56,20 @@ 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)
+  )
+}
+
 /** 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" }
@@ -56,7 +79,8 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
   override predicate isSink(DataFlow::Node sink) {
     exists(MethodCall mc |
       sink.asExpr() = mc.getArgument(0) and
-      mc.getTarget() instanceof HashMethod
+      mc.getTarget() instanceof HashMethod and
+      not hasAnotherHashCall(mc)
     )
   }
 
@@ -71,7 +95,7 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
   }
 
   /**
-   * Holds if a password is concatenated with a salt then hashed together through the call `System.Array.CopyTo()`, for example,
+   * Holds if a password is concatenated with a salt then hashed together through calls such as `System.Array.CopyTo()`, for example,
    *  `byte[] rawSalted  = new byte[passBytes.Length + salt.Length];`
    *  `passBytes.CopyTo(rawSalted, 0);`
    *  `salt.CopyTo(rawSalted, passBytes.Length);`
@@ -81,11 +105,27 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
   override predicate isSanitizer(DataFlow::Node node) {
     exists(MethodCall mc |
       mc.getTarget().fromLibrary() and
-      mc.getTarget().hasQualifiedName("System.Array", "CopyTo") and
-      mc.getArgument(0) = node.asExpr()
-    ) // passBytes.CopyTo(rawSalted, 0)
+      (
+        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
+      mc.getAnArgument() = node.asExpr()
+    )
     or
     exists(AddExpr e | node.asExpr() = e.getAnOperand()) // password+salt
+    or
+    exists(InterpolatedStringExpr e | node.asExpr() = e.getAnInsert())
+    or
+    // a salt or key is included in subclasses of `KeyedHashAlgorithm`
+    exists(MethodCall mc, Assignment ass, ObjectCreation oc |
+      ass.getRValue() = oc and
+      oc.getObjectType().getABaseType+() instanceof KeyedHashAlgorithm and
+      mc.getTarget() instanceof HashMethod and
+      ass.getLValue() = mc.getQualifier().(VariableAccess).getTarget().getAnAccess() and
+      mc.getArgument(0) = node.asExpr()
+    )
   }
 }
 

csharp/ql/test/experimental/Security Features/CWE-759/HashWithoutSalt.cs

@@ -62,6 +62,21 @@ public static string HashPassword2(string password)
         return Convert.ToBase64String(dbPassword);
     }
 
+    // GOOD - Hash with a salt.
+    public bool VerifyPasswordHash(string password, byte[] passwordHash, byte[] passwordSalt)
+    {
+        using(var hmac = new System.Security.Cryptography.HMACSHA512(passwordSalt))
+        {
+            var computedHash = hmac.ComputeHash(System.Text.Encoding.UTF8.GetBytes(password));
+            for(int i = 0;i<computedHash.Length;i++)
+            {
+                if (computedHash[i] != passwordHash[i])
+                    return false;
+            }
+            return true;
+        }
+    }
+
     public static byte[] GenerateSalt()
     {
         using (var rng = new RNGCryptoServiceProvider())
@@ -71,4 +86,39 @@ public static byte[] GenerateSalt()
             return randomNumber;
         }
     }
+
+    public static byte[] Combine(byte[] first, byte[] second)
+    {
+        // helper to combine two byte arrays
+        byte[] ret = new byte[first.Length + second.Length];
+        Buffer.BlockCopy(first, 0, ret, 0, first.Length);
+        Buffer.BlockCopy(second, 0, ret, first.Length, second.Length);
+        return ret;
+    }
+
+    // GOOD - Hash with a salt.
+    public static byte[] CalculateKeys(string password, string userid)
+    {
+        var utf16pass = System.Text.Encoding.UTF8.GetBytes(password);
+        var utf16sid = System.Text.Encoding.UTF8.GetBytes(userid);
+
+        var utf16sidfinal = new byte[utf16sid.Length + 2];
+        utf16sid.CopyTo(utf16sidfinal, 0);
+        utf16sidfinal[utf16sidfinal.Length - 2] = 0x00;
+
+        byte[] sha1bytes_password;
+        byte[] hmacbytes;
+
+        //Calculate SHA1 from user password
+        using (var sha1 = new SHA1Managed())
+        {
+            sha1bytes_password = sha1.ComputeHash(utf16pass);
+        }
+        var combined = Combine(sha1bytes_password, utf16sidfinal);
+        using (var hmac = new HMACSHA1(sha1bytes_password))
+        {
+            hmacbytes = hmac.ComputeHash(utf16sidfinal);
+        }
+        return hmacbytes;
+    }
 }