Clean up the query

Author: luchua-bc

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

@@ -17,6 +17,7 @@ class MessageDigest extends RefType {
   MessageDigest() { this.hasQualifiedName("java.security", "MessageDigest") }
 }
 
+/** The method call `MessageDigest.getInstance(...)` */
 class MDConstructor extends StaticMethodAccess {
   MDConstructor() {
     exists(Method m | m = this.getMethod() |
@@ -57,46 +58,35 @@ class MDHashMethodAccess extends MethodAccess {
 string getPasswordRegex() { result = "(?i).*pass(wd|word|code|phrase).*" }
 
 /** Finds variables that hold password information judging by their names. */
-class PasswordVarExpr extends Expr {
+class PasswordVarExpr extends VarAccess {
   PasswordVarExpr() {
-    this.(VarAccess).getVariable().getName().toLowerCase().regexpMatch(getPasswordRegex()) and
-    not this.(VarAccess).getVariable().getName().toLowerCase().matches("%hash%") // Exclude variable names such as `passwordHash` since their values were already hashed
+    exists(string name | name = this.getVariable().getName().toLowerCase() |
+      name.regexpMatch(getPasswordRegex()) and not name.matches("%hash%") // Exclude variable names such as `passwordHash` since their values were already hashed
+    )
   }
 }
 
 /** Holds if `Expr` e is a direct or indirect operand of `ae`. */
-predicate hasAddExpr(AddExpr ae, Expr e) { ae.getAnOperand+() = e }
+predicate hasAddExprAncestor(AddExpr ae, Expr e) { ae.getAnOperand+() = e }
 
-/** Holds if `MethodAccess` ma has a flow to another `MDHashMethodAccess` call. */
-predicate hasAnotherHashCall(MethodAccess ma) {
-  exists(MDHashMethodAccess ma2, DataFlow::Node node1, DataFlow::Node node2 |
+/** Holds if `MDHashMethodAccess ma` is a second `MDHashMethodAccess` call by the same object. */
+predicate hasAnotherHashCall(MDHashMethodAccess ma) {
+  exists(MDHashMethodAccess ma2 |
     ma2 != ma and
-    node1.asExpr() = ma.getAChildExpr() and
-    node2.asExpr() = ma2.getAChildExpr() and
-    (
-      TaintTracking2::localTaint(node1, node2) or
-      TaintTracking2::localTaint(node2, node1)
-    )
+    ma2.getQualifier() = ma.getQualifier().(VarAccess).getVariable().getAnAccess()
   )
 }
 
 /** Holds if `MethodAccess` ma is a hashing call without a sibling node making another hashing call. */
 predicate isSingleHashMethodCall(MDHashMethodAccess ma) { not hasAnotherHashCall(ma) }
 
 /** Holds if `MethodAccess` ma is invoked by `MethodAccess` ma2 either directly or indirectly. */
-predicate hasParentCall(MethodAccess ma2, MethodAccess ma) {
-  ma.getCaller() = ma2.getMethod()
-  or
-  exists(MethodAccess ma3 |
-    ma.getCaller() = ma3.getMethod() and
-    hasParentCall(ma2, ma3)
-  )
-}
+predicate hasParentCall(MethodAccess ma2, MethodAccess ma) { ma.getCaller() = ma2.getMethod() }
 
 /** Holds if `MethodAccess` is a single hashing call that is not invoked by a wrapper method. */
 predicate isSink(MethodAccess ma) {
   isSingleHashMethodCall(ma) and
-  not exists(MethodAccess ma2 | hasParentCall(ma2, ma)) // Not invoked by a wrapper method which could invoke MDHashMethod in another call stack to reduce FPs
+  not hasParentCall(_, ma) // Not invoked by a wrapper method which could invoke MDHashMethod in another call stack. This reduces FPs.
 }
 
 /** Sink of hashing calls. */
@@ -131,9 +121,9 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
       ma.getArgument(0) = node.asExpr()
     ) // System.arraycopy(password.getBytes(), ...)
     or
-    exists(AddExpr e | hasAddExpr(e, node.asExpr())) // password+salt
+    exists(AddExpr e | hasAddExprAncestor(e, node.asExpr())) // password+salt
     or
-    exists(ConditionalExpr ce | ce = node.asExpr()) // useSalt?password+":"+salt:password
+    exists(ConditionalExpr ce | ce.getAChildExpr() = node.asExpr()) // useSalt?password+":"+salt:password
     or
     exists(MethodAccess ma |
       ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "StringBuilder") and
@@ -143,7 +133,7 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
     or
     exists(MethodAccess ma |
       ma.getQualifier().(VarAccess).getVariable().getType() instanceof Interface and
-      ma.getAnArgument() = node.asExpr() // Method access of interface type variables requires runtime determination
+      ma.getAnArgument() = node.asExpr() // Method access of interface type variables requires runtime determination thus not handled
     )
   }
 }

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

@@ -54,12 +54,16 @@ public String getSHA256Hash3(String password, byte[] salt) throws NoSuchAlgorith
 
 	// GOOD - Hash with a given salt stored somewhere else.
 	public String getSHA256Hash(String password, String salt) throws NoSuchAlgorithmException {
-		return hash(password+":"+salt);
+		MessageDigest alg = MessageDigest.getInstance("SHA-256");
+		String payload = password+":"+salt;
+		return Base64.getEncoder().encodeToString(alg.digest(payload.getBytes(java.nio.charset.StandardCharsets.UTF_8)));
 	}
 
 	// GOOD - Hash with a given salt stored somewhere else.
 	public String getSHA256Hash2(String password, String salt, boolean useSalt) throws NoSuchAlgorithmException {
-		return hash(useSalt?password+":"+salt:password);
+		MessageDigest alg = MessageDigest.getInstance("SHA-256");
+		String payload = useSalt?password+":"+salt:password;
+		return Base64.getEncoder().encodeToString(alg.digest(payload.getBytes(java.nio.charset.StandardCharsets.UTF_8)));
 	}
 
 	// GOOD - Hash with a salt for a variable named passwordHash, whose value is a hash used as an input for a hashing function.
@@ -73,10 +77,6 @@ public void update(SHA256 sha256, byte[] foo, int start, int len) throws NoSuchA
 		sha256.update(foo, start, len);
 	}
 
-	public void update2(SHA256 sha256, byte[] foo, int start, int len) throws NoSuchAlgorithmException {
-		sha256.update(foo, start, len);
-	}
-
 	// BAD - Invoking a wrapper implementation without a salt is not detected.
 	public String getSHA256Hash4(String password) throws NoSuchAlgorithmException, ClassNotFoundException, IllegalAccessException, InstantiationException {
 		SHA256 sha256 = new SHA256();
@@ -98,15 +98,15 @@ public String getSHA256Hash5(String password) throws NoSuchAlgorithmException {
 	}
 
 	// BAD - Invoking a wrapper implementation without a salt is not detected.
-	public String getSHA256Hash6(String password) throws NoSuchAlgorithmException {
-		SHA256 sha256 = new SHA256();
+	public String getSHA512Hash6(String password) throws NoSuchAlgorithmException {
+		SHA512 sha512 = new SHA512();
 		byte[] passBytes = password.getBytes();
-		sha256.update(passBytes, 0, passBytes.length);
-		return Base64.getEncoder().encodeToString(sha256.digest());
+		sha512.update(passBytes, 0, passBytes.length);
+		return Base64.getEncoder().encodeToString(sha512.digest());
 	}
 
 	// BAD - Invoke a wrapper implementation with a salt, which is not detected with an interface type variable.
-	public String getSHA256Hash7(byte[] passphrase) throws NoSuchAlgorithmException, ClassNotFoundException, IllegalAccessException, InstantiationException {
+	public String getSHA512Hash7(byte[] passphrase) throws NoSuchAlgorithmException, ClassNotFoundException, IllegalAccessException, InstantiationException {
 		Class c = Class.forName("SHA512");
 		HASH sha512 = (HASH) (c.newInstance());
 		byte[] tmp = new byte[4];
@@ -120,11 +120,6 @@ public String getSHA256Hash7(byte[] passphrase) throws NoSuchAlgorithmException,
 		return Base64.getEncoder().encodeToString(key);
 	}
 
-	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)));
-	}
-
 	public static byte[] getSalt() throws NoSuchAlgorithmException {
 		SecureRandom sr = SecureRandom.getInstance("SHA1PRNG");
 		byte[] salt = new byte[16];