Revamp the query to reduce FPs introduced by wrapper calls

Author: luchua-bc

java/ql/src/experimental/Security/CWE/CWE-759/HashWithoutSalt.ql

@@ -55,29 +55,17 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
 
   override predicate isSink(DataFlow::Node sink) {
     exists(
-      MethodAccess mua, MethodAccess mda // invoke `md.digest()` with only one call of `md.update(password)`, that is, without the call of `md.update(digest)`
+      MethodAccess mua // invoke `md.update(password)` without the call of `md.update(digest)`
     |
       sink.asExpr() = mua.getArgument(0) and
-      mua.getMethod() instanceof MDUpdateMethod and // md.update(password)
-      mda.getMethod() instanceof MDDigestMethod and
-      mda.getNumArgument() = 0 and // md.digest()
-      mda.getQualifier() = mua.getQualifier().(VarAccess).getVariable().getAnAccess() and
-      not exists(MethodAccess mua2 |
-        mua2.getMethod() instanceof MDUpdateMethod and // md.update(salt)
-        mua2.getQualifier() = mua.getQualifier().(VarAccess).getVariable().getAnAccess() and
-        mua2 != mua
-      )
+      mua.getMethod() instanceof MDUpdateMethod // md.update(password)
     )
     or
     // invoke `md.digest(password)` without another call of `md.update(salt)`
     exists(MethodAccess mda |
       sink.asExpr() = mda.getArgument(0) and
       mda.getMethod() instanceof MDDigestMethod and // md.digest(password)
-      mda.getNumArgument() = 1 and
-      not exists(MethodAccess mua |
-        mua.getMethod() instanceof MDUpdateMethod and // md.update(salt)
-        mua.getQualifier() = mda.getQualifier().(VarAccess).getVariable().getAnAccess()
-      )
+      mda.getNumArgument() = 1
     )
   }
 
@@ -96,9 +84,37 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
     ) // System.arraycopy(password.getBytes(), ...)
     or
     exists(AddExpr e | node.asExpr() = e.getAnOperand()) // password+salt
+    or
+    exists(MethodAccess mua, MethodAccess ma |
+      ma.getArgument(0) = node.asExpr() and // Detect wrapper methods that invoke `md.update(salt)`
+      ma != mua and
+      (
+        mua.getQualifier().(VarAccess).getVariable().getAnAccess() = ma.getQualifier()
+        or
+        mua.getAnArgument().(VarAccess).getVariable().getAnAccess() = ma.getQualifier()
+        or
+        mua.getQualifier().(VarAccess).getVariable().getAnAccess() = ma.getAnArgument()
+        or
+        mua.getAnArgument().(VarAccess).getVariable().getAnAccess() = ma.getAnArgument()
+      ) and
+      isMDUpdateCall(mua.getMethod())
+    )
   }
 }
 
+/** Holds if a method invokes `md.update(salt)`. */
+predicate isMDUpdateCall(Callable caller) {
+  caller instanceof MDUpdateMethod
+  or
+  exists(Callable callee |
+    caller.polyCalls(callee) and
+    (
+      callee instanceof MDUpdateMethod or
+      isMDUpdateCall(callee)
+    )
+  )
+}
+
 from DataFlow::PathNode source, DataFlow::PathNode sink, HashWithoutSaltConfiguration c
 where c.hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "$@ is hashed without a salt.", source.getNode(),

java/ql/test/experimental/query-tests/security/CWE-759/HashWithoutSalt.java

@@ -64,7 +64,43 @@ public String getSHA256Hash3(String passwordHash) throws NoSuchAlgorithmExceptio
 		return Base64.getEncoder().encodeToString(messageDigest);
 	}
 
-	private String hash(String payload) {
+	public void update(SHA256 sha256, byte[] foo, int start, int len) throws NoSuchAlgorithmException {
+		sha256.update(foo, start, len);
+	}
+
+	public void update2(SHA256 sha256, byte[] foo, int start, int len) throws NoSuchAlgorithmException {
+		sha256.update(foo, start, len);
+	}
+	
+	// GOOD - Invoke a wrapper implementation with a salt.
+	public String getSHA256Hash4(String password) throws NoSuchAlgorithmException {
+		SHA256 sha256 = new SHA256();
+		byte[] salt = getSalt();
+		byte[] passBytes = password.getBytes();
+		sha256.update(passBytes, 0, passBytes.length);
+		sha256.update(salt, 0, salt.length);
+		return Base64.getEncoder().encodeToString(sha256.digest());
+	}
+
+	// GOOD - Invoke a wrapper implementation with a salt.
+	public String getSHA256Hash5(String password) throws NoSuchAlgorithmException {
+		SHA256 sha256 = new SHA256();
+		byte[] salt = getSalt();
+		byte[] passBytes = password.getBytes();
+		sha256.update(passBytes, 0, passBytes.length);
+		update(sha256, salt, 0, salt.length);
+		return Base64.getEncoder().encodeToString(sha256.digest());
+	}
+
+	// BAD - Invoke a wrapper implementation without a salt.
+	public String getSHA256Hash6(String password) throws NoSuchAlgorithmException {
+		SHA256 sha256 = new SHA256();
+		byte[] passBytes = password.getBytes();
+		sha256.update(passBytes, 0, passBytes.length);
+		return Base64.getEncoder().encodeToString(sha256.digest());
+	}
+
+	private String hash(String payload) throws NoSuchAlgorithmException {
 		MessageDigest alg = MessageDigest.getInstance("SHA-256");
 		return Base64.getEncoder().encodeToString(alg.digest(payload.getBytes(java.nio.charset.StandardCharsets.UTF_8)));
 	}

java/ql/test/experimental/query-tests/security/CWE-759/SHA256.java

@@ -1,20 +1,21 @@
 import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
 
 public class SHA256 {
   MessageDigest md;
   public int getBlockSize() {return 32;}
-  public void init() throws Exception {
+  public void init() throws NoSuchAlgorithmException {
     try { md = MessageDigest.getInstance("SHA-256"); }
     catch (Exception e){
       System.err.println(e);
     }
   }
 
-  public void update(byte[] foo, int start, int len) throws Exception {
+  public void update(byte[] foo, int start, int len) throws NoSuchAlgorithmException {
     md.update(foo, start, len);
   }
 
-  public byte[] digest() throws Exception {
+  public byte[] digest() throws NoSuchAlgorithmException {
     return md.digest();
   }
 }