Merge pull request #13825 from boveus/add-cwe-208

Ruby: Add Unsafe HMAC Comparison Query.

Author: alexrford

ruby/ql/src/experimental/cwe-208/UnsafeHmacComparison.ql

@@ -0,0 +1,45 @@
+/**
+ * @name Unsafe HMAC Comparison
+ * @description An HMAC is being compared using the equality operator.  This may be vulnerable to a cryptographic timing attack
+ *              because the equality operation does not occur in constant time."
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 6.0
+ * @precision high
+ * @id rb/unsafe-hmac-comparison
+ * @tags security
+ *       external/cwe/cwe-208
+ */
+
+private import codeql.ruby.AST
+private import codeql.ruby.DataFlow
+private import codeql.ruby.ApiGraphs
+
+private class OpenSslHmacSource extends DataFlow::Node {
+  OpenSslHmacSource() {
+    exists(API::Node hmacNode | hmacNode = API::getTopLevelMember("OpenSSL").getMember("HMAC") |
+      this = hmacNode.getAMethodCall(["hexdigest", "to_s", "digest", "base64digest"])
+      or
+      this = hmacNode.getAnInstantiation()
+    )
+  }
+}
+
+private module UnsafeHmacComparison {
+  private module Config implements DataFlow::ConfigSig {
+    predicate isSource(DataFlow::Node source) { source instanceof OpenSslHmacSource }
+
+    // Holds if a given sink is an Equality Operation (== or !=)
+    predicate isSink(DataFlow::Node sink) {
+      any(EqualityOperation eqOp).getAnOperand() = sink.asExpr().getExpr()
+    }
+  }
+
+  import DataFlow::Global<Config>
+}
+
+private import UnsafeHmacComparison::PathGraph
+
+from UnsafeHmacComparison::PathNode source, UnsafeHmacComparison::PathNode sink
+where UnsafeHmacComparison::flowPath(source, sink)
+select sink.getNode(), source, sink, "This comparison is potentially vulnerable to a timing attack."

ruby/ql/src/experimental/cwe-208/UnsafeHmacComparison.qlhelp

@@ -0,0 +1,21 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+  <overview>
+    <p>
+      Using the `==` or `!=` operator to compare a known valid HMAC with a user-supplied HMAC digest could lead to a timing attack, as these operations do not occur in constant time.
+    </p>
+  </overview>
+  <recommendation>
+    <p>
+      Instead of using `==` or `!=` to compare a known valid HMAC with a user-supplied HMAC digest use Rack::Utils#secure_compare, ActiveSupport::SecurityUtils#secure_compare or OpenSSL.secure_compare
+    </p>
+  </recommendation>
+  <example>
+    <p>
+    In this example, the HMAC is validated using the `==` operation.
+    </p>
+    <sample src="./examples/unsafe_hmac_comparison.rb" />
+  </example>
+</qhelp>

ruby/ql/src/experimental/cwe-208/examples/unsafe_hmac_comparison.rb

@@ -0,0 +1,11 @@
+class UnsafeHmacComparison
+    def verify_hmac(host, hmac, salt)
+      sha1 = OpenSSL::Digest.new('sha1')
+      if OpenSSL::HMAC.digest(sha1, salt, host) == hmac
+        puts "HMAC verified"
+      else
+        puts "HMAC not verified"
+      end
+    end
+  end
+