Java: Timing attack

java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.ql

@@ -13,7 +13,7 @@
  */
 
 import java
-import NonConstantTimeCheckOnSignatureQuery
+import experimental.semmle.code.java.security.TimingAttack
 import DataFlow::PathGraph
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeCryptoComparisonConfig conf

java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/SafeComparison.java

@@ -0,0 +1,5 @@
+private boolean safeComparison(String pwd, HttpServletRequest request) {
+      String password = request.getParameter("password");
+      return MessageDigest.isEqual(password.getBytes(StandardCharsets.UTF_8), pwd.getBytes(StandardCharsets.UTF_8));
+}
+

java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.qhelp

@@ -0,0 +1,54 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+  Timing Attack is based on the leakage of information of secret parameters by studying
+how long it takes the system to respond to different inputs. 
+  it can be circumvented by using a constant-time algorithm for checking the value of sensitive info, 
+more precisely, the comparison time should not depend on the content of the input. Otherwise the attacker gains
+information that is indirectly leaked by the application. This information is then used for malicious purposes, 
+such as guessing the password of a user.
+</p>
+</overview>
+
+
+<recommendation>
+<p>
+ Two types of countermeasures can be applied against timing attacks. The first one consists 
+in eliminating timing variations whereas the second renders these variations useless for an attacker.
+The only absolute way to prevent timing attacks is to make the computation strictly constant time,
+independent of the input.
+
+ Use <code>MessageDigest.isEqual()</code> method to securely check the value of sensitive info.
+If this method is used, then the calculation time depends only on the length of input byte arrays,
+and does not depend on the contents of the arrays. 
+ Unlike <code>Arrays.equals()</code> is a fail fast method, If the first byte is not equal, it will return immediately.
+</p>
+</recommendation>
+<example>
+<p>
+  The following example uses <code>String.equals()</code> which is a fail fast method for validating a password.
+This method implements a non-constant-time algorithm:
+</p>
+<sample src="UnsafeComparison.java" />
+
+<p> 
+  The next example use a safe constant-time algorithm for validating a password:
+</p>
+<sample src="SafeComparison.java" />
+</example>
+
+<references>
+<li>
+  Wikipedia:
+  <a href="https://en.wikipedia.org/wiki/Timing_attack">Timing attack</a>.
+</li>
+
+<li>
+  Java API Specification:
+  <a href="https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/security/MessageDigest.html#isEqual(byte[],byte[])">MessageDigest.isEqual() method</a>
+</li>
+</references>
+
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.ql

@@ -0,0 +1,40 @@
+/**
+ * @name Timing attack against sensitive info
+ * @description Use of a non-constant-time verification routine to check the value of an sensitive info,
+ *              possibly allowing a timing attack to infer the info's expected value.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/timing-attack-against-sensitive-info
+ * @tags security
+ *       external/cwe/cwe-208
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking
+import DataFlow::PathGraph
+import experimental.semmle.code.java.security.TimingAttack
+
+/**
+ * A configuration that tracks data flow from variable that may hold sensitive data
+ * to methods that compare data using a non-constant-time algorithm.
+ */
+class NonConstantTimeComparisonConfig extends TaintTracking::Configuration {
+  NonConstantTimeComparisonConfig() { this = "NonConstantTimeComparisonConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof SecretSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof NonConstantTimeComparisonSink }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeComparisonConfig conf
+where
+  conf.hasFlowPath(source, sink) and
+  (
+    source.getNode().(SecretSource).includesUserInput() or
+    sink.getNode().(NonConstantTimeComparisonSink).includesUserInput()
+  ) and
+  not sink.getNode().(NonConstantTimeComparisonSink).includesIs()
+select sink.getNode(), source, sink, "timing attack against $@ validation.",
+  source.getNode(), "time constant"

java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/UnsafeComparison.java

@@ -0,0 +1,5 @@
+private boolean UnsafeComparison(String pwd, HttpServletRequest request) {
+    String password = request.getParameter("password");
+    return password.equals(pwd);        
+}
+

java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.ql

@@ -14,7 +14,7 @@
  */
 
 import java
-import NonConstantTimeCheckOnSignatureQuery
+import experimental.semmle.code.java.security.TimingAttack
 import DataFlow::PathGraph
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeCryptoComparisonConfig conf

java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll → java/ql/src/experimental/semmle/code/java/security/TimingAttack.qll

@@ -2,12 +2,55 @@
  * Provides classes and predicates for queries that detect timing attacks.
  */
 
+import java
 import semmle.code.java.controlflow.Guards
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.TaintTracking2
+import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.DataFlow3
+import semmle.code.java.dataflow.DataFlow2
 import semmle.code.java.dataflow.FlowSources
 
+/** A string for `match` that identifies strings that look like they represent secret data. */
+private string suspicious() {
+  result =
+    [
+      "%password", "%passwd", "%pwd%", "%refresh%token%", "%secret%token", "%secret%key",
+      "%passcode", "%passphrase", "%secret%", "%userpass%", "%digest%", "%signature%"
+    ]
+}
+
+/**
+ * A string for `match` that identifies strings that look like they represent secret data that is
+ * hashed or encrypted.
+ */
+private string nonSuspicious() {
+  result = "%hashed%" or
+  result = "%encrypted%" or
+  result = "%crypt%" or
+  result in ["%md5%", "%md2%", "%sha%"]
+}
+
+/** A variable that may hold nonsensitive information, judging by its name. * */
+class NonSensitiveExpr extends Expr {
+  NonSensitiveExpr() {
+    exists(Variable v | this = v.getAnAccess() | 
+       v.getName().toLowerCase().matches(nonSuspicious())
+    ) 
+  }
+}
+
+/** A variable that may hold sensitive information, judging by its name. * */
+class CredentialExpr extends Expr {
+  CredentialExpr() {
+    exists(Variable v | this = v.getAnAccess() |
+      v.getName().toLowerCase().matches(suspicious()) and
+      not v.getName().toLowerCase().matches(nonSuspicious())
+      not v.isFinal()      
+    )
+  }
+}
+
 /** A method call that produces cryptographic result. */
 abstract private class ProduceCryptoCall extends MethodAccess {
   Expr output;
@@ -228,6 +271,20 @@ private class UserInputInComparisonConfig extends TaintTracking2::Configuration
   }
 }
 
+private class UserInputIs extends TaintTracking2::Configuration {
+  UserInputIs() { this = "UserInputIs" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof InSecretSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(NonConstantTimeEqualsCall call |
+      sink.asExpr() = [call.getAnArgument(), call.getQualifier()]
+    )
+    or
+    exists(NonConstantTimeComparisonCall call | sink.asExpr() = call.getAnArgument())
+  }
+}
+
 /** Holds if `expr` looks like a constant. */
 private predicate looksLikeConstant(Expr expr) {
   expr.isCompileTimeConstant()
@@ -308,6 +365,30 @@ class NonConstantTimeComparisonSink extends DataFlow::Node {
       config.hasFlowTo(DataFlow2::exprNode(anotherParameter))
     )
   }
+
+  predicate includesIs() {
+    exists(UserInputIs config |
+      config.hasFlowTo(DataFlow2::exprNode(anotherParameter))
+    ) 
+  }
+}
+
+/** A data flow source of the secret obtained. */
+class SecretSource extends DataFlow::Node {
+  CredentialExpr secret;
+
+  SecretSource() { secret = this.asExpr() }
+
+  /** Holds if the secret was deliverd by remote user. */
+  predicate includesUserInput() {
+    exists(UserInputSecretConfig config | config.hasFlowTo(DataFlow2::exprNode(secret)))
+  }
+}
+
+class InSecretSource extends DataFlow::Node {
+  NonSensitiveExpr insecret;
+
+  InSecretSource() { insecret = this.asExpr() }
 }
 
 /**
@@ -321,3 +402,14 @@ class NonConstantTimeCryptoComparisonConfig extends TaintTracking::Configuration
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof NonConstantTimeComparisonSink }
 }
+
+/**
+ * A config that tracks data flow from remote user input to Variable that hold sensitive info
+ */
+class UserInputSecretConfig extends TaintTracking2::Configuration {
+  UserInputSecretConfig() { this = "UserInputSecretConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof CredentialExpr }
+}

java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/Test.java

@@ -0,0 +1,17 @@
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.lang.String;
+import javax.servlet.http.HttpServletRequest;
+
+public class Test {
+    private boolean UnsafeComparison(String pwd, HttpServletRequest request) {
+        String password = request.getParameter("password");
+        return password.equals(pwd);        
+    }
+
+    private boolean safeComparison(String pwd, HttpServletRequest request) {
+          String password = request.getParameter("password");
+          return MessageDigest.isEqual(password.getBytes(StandardCharsets.UTF_8), pwd.getBytes(StandardCharsets.UTF_8));
+    }
+
+}

java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.expected

@@ -0,0 +1,8 @@
+edges
+nodes
+| Test.java:10:16:10:23 | password | semmle.label | password |
+| Test.java:10:32:10:34 | pwd | semmle.label | pwd |
+subpaths
+#select
+| Test.java:10:16:10:23 | password | Test.java:10:16:10:23 | password | Test.java:10:16:10:23 | password | Possible timing attack against $@ validation. | Test.java:10:16:10:23 | password |
+| Test.java:10:32:10:34 | pwd | Test.java:10:32:10:34 | pwd | Test.java:10:32:10:34 | pwd | Possible timing attack against $@ validation. | Test.java:10:32:10:34 | pwd |

java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/PossibleTimingAttackAgainstSensitiveInfo.ql