Query to detect hash without salt

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

@@ -0,0 +1,14 @@
+public class HashWithoutSalt {
+	// BAD - Hash without a salt.
+	public void getSHA256Hash(String password) throws NoSuchAlgorithmException {
+		MessageDigest md = MessageDigest.getInstance("SHA-256");
+		byte[] messageDigest = md.digest(password.getBytes());
+	}
+
+	// GOOD - Hash with a salt.
+	public void getSHA256Hash(String password, byte[] salt) throws NoSuchAlgorithmException {
+		MessageDigest md = MessageDigest.getInstance("SHA-256");
+		md.update(salt);
+		byte[] messageDigest = md.digest(password.getBytes());
+	}
+}

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

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+  <overview>
+    <p>In cryptography, "salt" is random data that are used as an additional input to a one-way function that hashes a password or pass-phrase. It makes dictionary attacks more difficult.</p>
+
+    <p>Without a salt, it is much easier for attackers to pre-compute the hash value using dictionary attack techniques such as rainbow tables to crack passwords.</p>
+  </overview>
+
+  <recommendation>
+    <p>Use a long random salt of at least 32 bytes then use the combination of password and salt to hash a password or password phrase.</p>
+  </recommendation>
+
+  <example>
+    <p>The following example shows two ways of hashing. In the 'BAD' case, no salt is provided. In the 'GOOD' case, a salt is provided.</p>
+    <sample src="HashWithoutSalt.java" />
+  </example>
+
+  <references>
+    <li>
+      DZone:
+      <a href="https://dzone.com/articles/a-look-at-java-cryptography">A Look at Java Cryptography</a>
+    </li>
+    <li>
+      CWE:
+      <a href="https://cwe.mitre.org/data/definitions/759.html">CWE-759: Use of a One-Way Hash without a Salt</a>
+    </li>
+  </references>
+</qhelp>

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

@@ -0,0 +1,93 @@
+/**
+ * @id java/hash-without-salt
+ * @name Use of a One-Way Hash without a Salt
+ * @description Hashed passwords without a salt are vulnerable to dictionary attacks.
+ * @kind path-problem
+ * @tags security
+ *       external/cwe-759
+ */
+
+import java
+import semmle.code.java.dataflow.TaintTracking
+import DataFlow::PathGraph
+
+/** The Java class `java.security.MessageDigest` */
+class MessageDigest extends RefType {
+  MessageDigest() { this.hasQualifiedName("java.security", "MessageDigest") }
+}
+
+/** The method `digest()` declared in `java.security.MessageDigest`. */
+class MDDigestMethod extends Method {
+  MDDigestMethod() {
+    getDeclaringType() instanceof MessageDigest and
+    hasName("digest")
+  }
+}
+
+/** The method `update()` declared in `java.security.MessageDigest`. */
+class MDUpdateMethod extends Method {
+  MDUpdateMethod() {
+    getDeclaringType() instanceof MessageDigest and
+    hasName("update")
+  }
+}
+
+/**
+ * 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 {
+  PasswordVarExpr() {
+    exists(Variable v | this = v.getAnAccess() | v.getName().regexpMatch(getPasswordRegex()))
+  }
+}
+
+/** 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" }
+
+  override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof PasswordVarExpr }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(
+      MethodAccess mda, MethodAccess mua // invoke `md.digest()` with only one call of `md.update(password)`, that is, without the call of `md.update(digest)`
+    |
+      sink.asExpr() = mda.getQualifier() and
+      mda.getMethod() instanceof MDDigestMethod and
+      mda.getNumArgument() = 0 and // md.digest()
+      mua.getMethod() instanceof MDUpdateMethod and // md.update(password)
+      mua.getQualifier() = mda.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
+      )
+    )
+    or
+    // invoke `md.digest(password)` without another call of `md.update(salt)`
+    exists(MethodAccess mda |
+      sink.asExpr() = mda 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()
+      )
+    )
+  }
+
+  /** Holds for additional steps such as `passwordStr.getBytes()` */
+  override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(MethodAccess ma |
+      pred.asExpr() = ma.getAnArgument() and
+      (succ.asExpr() = ma or succ.asExpr() = ma.getQualifier())
+    )
+  }
+}
+
+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(),
+  "The password"

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

@@ -0,0 +1,11 @@
+edges
+| HashWithoutSalt.java:9:36:9:43 | password : String | HashWithoutSalt.java:9:26:9:55 | digest(...) |
+| HashWithoutSalt.java:15:13:15:20 | password : String | HashWithoutSalt.java:16:26:16:27 | md |
+nodes
+| HashWithoutSalt.java:9:26:9:55 | digest(...) | semmle.label | digest(...) |
+| HashWithoutSalt.java:9:36:9:43 | password : String | semmle.label | password : String |
+| HashWithoutSalt.java:15:13:15:20 | password : String | semmle.label | password : String |
+| HashWithoutSalt.java:16:26:16:27 | md | semmle.label | md |
+#select
+| HashWithoutSalt.java:9:26:9:55 | digest(...) | HashWithoutSalt.java:9:36:9:43 | password : String | HashWithoutSalt.java:9:26:9:55 | digest(...) | $@ is hashed without a salt. | HashWithoutSalt.java:9:36:9:43 | password | The password |
+| HashWithoutSalt.java:16:26:16:27 | md | HashWithoutSalt.java:15:13:15:20 | password : String | HashWithoutSalt.java:16:26:16:27 | md | $@ is hashed without a salt. | HashWithoutSalt.java:15:13:15:20 | password | The password |

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

@@ -0,0 +1,40 @@
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+
+public class HashWithoutSalt {
+	// BAD - Hash without a salt.
+	public void getSHA256Hash(String password) throws NoSuchAlgorithmException {
+		MessageDigest md = MessageDigest.getInstance("SHA-256");
+		byte[] messageDigest = md.digest(password.getBytes());
+	}
+
+	// BAD - Hash without a salt.
+	public void getSHA256Hash2(String password) throws NoSuchAlgorithmException {
+		MessageDigest md = MessageDigest.getInstance("SHA-256");
+		md.update(password.getBytes());
+		byte[] messageDigest = md.digest();
+	}
+
+	// GOOD - Hash with a salt.
+	public void getSHA256Hash(String password, byte[] salt) throws NoSuchAlgorithmException {
+		MessageDigest md = MessageDigest.getInstance("SHA-256");
+		md.update(salt);
+		byte[] messageDigest = md.digest(password.getBytes());
+	}
+
+	// GOOD - Hash with a salt.
+	public void getSHA256Hash2(String password, byte[] salt) throws NoSuchAlgorithmException {
+		MessageDigest md = MessageDigest.getInstance("SHA-256");
+		md.update(salt);
+		md.update(password.getBytes());
+		byte[] messageDigest = md.digest();
+	}
+
+	public static byte[] getSalt() throws NoSuchAlgorithmException {
+        SecureRandom sr = SecureRandom.getInstance("SHA1PRNG");
+        byte[] salt = new byte[16];
+        sr.nextBytes(salt);
+        return salt;
+    }	
+}

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

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-759/HashWithoutSalt.ql