Merge pull request #7867 from ahmed532009/timing-attacks

Java: Timing attacks while comparing the headers value

Author: smowton

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

@@ -0,0 +1,20 @@
+import javax.servlet.http.HttpServletRequest;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.lang.String;
+
+
+public class Test {
+    private boolean UnsafeComparison(HttpServletRequest request) {
+        String Key = "secret";
+        return Key.equals(request.getHeader("X-Auth-Token"));        
+    }
+
+    private boolean safeComparison(HttpServletRequest request) {
+          String token = request.getHeader("X-Auth-Token");
+          String Key = "secret"; 
+          return MessageDigest.isEqual(Key.getBytes(StandardCharsets.UTF_8), token.getBytes(StandardCharsets.UTF_8));
+    }
+    
+}
+

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

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+A constant-time algorithm should be used for checking the value of sensitive headers.
+In other words, the comparison time should not depend on the content of the input. 
+Otherwise timing information could be used to infer the header's expected, secret value.
+</p>
+</overview>
+
+
+<recommendation>
+<p>
+Use <code>MessageDigest.isEqual()</code> method to check the value of headers.
+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.
+</p>
+</recommendation>
+<example>
+<p>
+The following example uses <code>String.equals()</code> method for validating a csrf token.
+This method implements a non-constant-time algorithm. The example also demonstrates validation using a safe constant-time algorithm.
+</p>
+<sample src="TimingAttackAgainstHeader.java" />
+</example>
+</qhelp>
+

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

@@ -0,0 +1,72 @@
+/**
+ * @name Timing attack against header value
+ * @description Use of a non-constant-time verification routine to check the value of an HTTP header,
+ *              possibly allowing a timing attack to infer the header's expected value.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/timing-attack-against-headers-value
+ * @tags security
+ *       external/cwe/cwe-208
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking
+import DataFlow::PathGraph
+
+/** A static method that uses a non-constant-time algorithm for comparing inputs. */
+private class NonConstantTimeComparisonCall extends StaticMethodAccess {
+  NonConstantTimeComparisonCall() {
+    this.getMethod()
+        .hasQualifiedName("org.apache.commons.lang3", "StringUtils",
+          ["equals", "equalsAny", "equalsAnyIgnoreCase", "equalsIgnoreCase"])
+  }
+}
+
+/** Methods that use a non-constant-time algorithm for comparing inputs. */
+private class NonConstantTimeEqualsCall extends MethodAccess {
+  NonConstantTimeEqualsCall() {
+    this.getMethod()
+        .hasQualifiedName("java.lang", "String", ["equals", "contentEquals", "equalsIgnoreCase"])
+  }
+}
+
+private predicate isNonConstantEqualsCallArgument(Expr e) {
+  exists(NonConstantTimeEqualsCall call | e = [call.getQualifier(), call.getArgument(0)])
+}
+
+private predicate isNonConstantComparisonCallArgument(Expr p) {
+  exists(NonConstantTimeComparisonCall call | p = [call.getArgument(0), call.getArgument(1)])
+}
+
+class ClientSuppliedIpTokenCheck extends DataFlow::Node {
+  ClientSuppliedIpTokenCheck() {
+    exists(MethodAccess ma |
+      ma.getMethod().hasName("getHeader") and
+      ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [
+          "x-auth-token", "x-csrf-token", "http_x_csrf_token", "x-csrf-param", "x-csrf-header",
+          "http_x_csrf_token", "x-api-key", "authorization", "proxy-authorization"
+        ] and
+      ma = this.asExpr()
+    )
+  }
+}
+
+class NonConstantTimeComparisonConfig extends TaintTracking::Configuration {
+  NonConstantTimeComparisonConfig() { this = "NonConstantTimeComparisonConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof ClientSuppliedIpTokenCheck
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    isNonConstantEqualsCallArgument(sink.asExpr()) or
+    isNonConstantComparisonCallArgument(sink.asExpr())
+  }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeComparisonConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Possible timing attack against $@ validation.",
+  source.getNode(), "client-supplied token"

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

@@ -0,0 +1,19 @@
+import javax.servlet.http.HttpServletRequest;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.lang.String;
+
+
+public class Test {
+    private boolean UnsafeComparison(HttpServletRequest request) {
+        String Key = "secret";
+        return Key.equals(request.getHeader("X-Auth-Token"));        
+    }
+
+    private boolean safeComparison(HttpServletRequest request) {
+          String token = request.getHeader("X-Auth-Token");
+          String Key = "secret"; 
+          return MessageDigest.isEqual(Key.getBytes(StandardCharsets.UTF_8), token.getBytes(StandardCharsets.UTF_8));
+    }
+
+}

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

@@ -0,0 +1,6 @@
+edges
+nodes
+| Test.java:10:27:10:59 | getHeader(...) | semmle.label | getHeader(...) |
+subpaths
+#select
+| Test.java:10:27:10:59 | getHeader(...) | Test.java:10:27:10:59 | getHeader(...) | Test.java:10:27:10:59 | getHeader(...) | Possible timing attack against $@ validation. | Test.java:10:27:10:59 | getHeader(...) | client-supplied token |

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

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