Merge pull request github#13119 from porcupineyhairs/goTiming

Go : Add query to detect potential timing attacks

Author: smowton

go/ql/lib/semmle/go/security/SensitiveActions.qll

@@ -35,7 +35,7 @@ module HeuristicNames {
    */
   string maybePassword() {
     result = "(?is).*pass(wd|word|code|phrase)(?!.*question).*" or
-    result = "(?is).*(auth(entication|ori[sz]ation)?)key.*"
+    result = "(?is).*(auth(entication|ori[sz]ation)?|api)key.*"
   }
 
   /**

go/ql/src/experimental/CWE-203/Timing.qhelp

@@ -0,0 +1,36 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+  <overview>
+    <p>
+      Using a non-constant time comparision to compare sensitive information can lead to auth
+      vulnerabilities.
+</p>
+  </overview>
+
+  <recommendation>
+    <p>Use of a constant time comparision function such as <code>crypto/subtle</code> package's <code>
+      ConstantTimeCompare</code> function can prevent this vulnerability. </p>
+  </recommendation>
+
+  <example>
+    <p>In the following examples, the code accepts a secret via a HTTP header in variable <code>
+      secretHeader</code> and a secret from the user in the <code>headerSecret</code> variable, which
+      are then compared with a system stored secret to perform authentication.</p>
+
+
+    <sample src="timingBad.go" />
+
+    <p>In the following example, the input provided by the user is compared using the <code>
+      ConstantTimeComapre</code> function. This ensures that timing attacks are not possible in this
+      case.</p>
+
+    <sample src="timingGood.go" />
+  </example>
+
+  <references>
+    <li>National Vulnerability Database: <a href="https://nvd.nist.gov/vuln/detail/CVE-2022-24912">
+      CVE-2022-24912</a>.</li>
+    <li>Verbose Logging:<a href="https://verboselogging.com/2012/08/20/a-timing-attack-in-action"> A
+      timing attack in action </a></li>
+  </references>
+</qhelp>

go/ql/src/experimental/CWE-203/Timing.ql

@@ -0,0 +1,72 @@
+/**
+ * @name Timing attacks due to comparison of sensitive secrets
+ * @description using a non-constant time comparison method to compare secrets can lead to authoriztion vulnerabilities
+ * @kind path-problem
+ * @problem.severity warning
+ * @id go/timing-attack
+ * @tags security
+ *       experimental
+ *       external/cwe/cwe-203
+ */
+
+import go
+import DataFlow::PathGraph
+import semmle.go.security.SensitiveActions
+
+private predicate isBadResult(DataFlow::Node e) {
+  exists(string path | path = e.asExpr().getFile().getAbsolutePath().toLowerCase() |
+    path.matches(["%fake%", "%dummy%", "%test%", "%example%"]) and not path.matches("%ql/test%")
+  )
+}
+
+/**
+ * A data flow sink for timing attack vulnerabilities.
+ */
+abstract class Sink extends DataFlow::Node { }
+
+/** A taint-tracking sink which models comparisons of sensitive variables. */
+private class SensitiveCompareSink extends Sink {
+  ComparisonExpr c;
+
+  SensitiveCompareSink() {
+    // We select a comparison where a secret or password is tested.
+    exists(SensitiveVariableAccess op1, Expr op2 |
+      op1.getClassification() = [SensitiveExpr::secret(), SensitiveExpr::password()] and
+      // exclude grant to avoid FP from OAuth
+      not op1.getClassification().matches("%grant%") and
+      op1 = c.getAnOperand() and
+      op2 = c.getAnOperand() and
+      not op1 = op2 and
+      not (
+        // Comparisons with `nil` should be excluded.
+        op2 = Builtin::nil().getAReference()
+        or
+        // Comparisons with empty string should also be excluded.
+        op2.getStringValue().length() = 0
+      )
+    |
+      // It is important to note that the name of both the operands need not be
+      // `sensitive`. Even if one of the operands appears to be sensitive, we consider it a potential sink.
+      c.getAnOperand() = this.asExpr()
+    )
+  }
+
+  DataFlow::Node getOtherOperand() { result.asExpr() = c.getAnOperand() and not result = this }
+}
+
+class SecretTracking extends TaintTracking::Configuration {
+  SecretTracking() { this = "SecretTracking" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof UntrustedFlowSource and not isBadResult(source)
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink and not isBadResult(sink) }
+}
+
+from SecretTracking cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where
+  cfg.hasFlowPath(source, sink) and
+  not cfg.hasFlowTo(sink.getNode().(SensitiveCompareSink).getOtherOperand())
+select sink.getNode(), source, sink, "$@ may be vulnerable to timing attacks.", source.getNode(),
+  "Hardcoded String"

go/ql/src/experimental/CWE-203/timingBad.go

@@ -0,0 +1,11 @@
+func bad(w http.ResponseWriter, req *http.Request, []byte secret) (interface{}, error) {
+
+	secretHeader := "X-Secret"
+
+	headerSecret := req.Header.Get(secretHeader)
+	secretStr := string(secret)
+	if len(secret) != 0 && headerSecret != secretStr {
+		return nil, fmt.Errorf("header %s=%s did not match expected secret", secretHeader, headerSecret)
+	}
+	return nil, nil
+}

go/ql/src/experimental/CWE-203/timingGood.go

@@ -0,0 +1,10 @@
+func good(w http.ResponseWriter, req *http.Request, []byte secret) (interface{}, error) {
+
+	secretHeader := "X-Secret"
+
+	headerSecret := req.Header.Get(secretHeader)
+	if len(secret) != 0 && subtle.ConstantTimeCompare(secret, []byte(headerSecret)) != 1 {
+		return nil, fmt.Errorf("header %s=%s did not match expected secret", secretHeader, headerSecret)
+	}
+	return nil, nil
+}

go/ql/test/experimental/CWE-203/Timing.expected

@@ -0,0 +1,10 @@
+edges
+| timing.go:14:18:14:27 | selection of Header | timing.go:14:18:14:45 | call to Get |
+| timing.go:14:18:14:45 | call to Get | timing.go:16:25:16:36 | headerSecret |
+nodes
+| timing.go:14:18:14:27 | selection of Header | semmle.label | selection of Header |
+| timing.go:14:18:14:45 | call to Get | semmle.label | call to Get |
+| timing.go:16:25:16:36 | headerSecret | semmle.label | headerSecret |
+subpaths
+#select
+| timing.go:16:25:16:36 | headerSecret | timing.go:14:18:14:27 | selection of Header | timing.go:16:25:16:36 | headerSecret | $@ may be vulnerable to timing attacks. | timing.go:14:18:14:27 | selection of Header | Hardcoded String |

go/ql/test/experimental/CWE-203/Timing.qlref

@@ -0,0 +1 @@
+experimental/CWE-203/Timing.ql

go/ql/test/experimental/CWE-203/timing.go

@@ -0,0 +1,37 @@
+package main
+
+import (
+	"crypto/subtle"
+	"fmt"
+	"net/http"
+)
+
+func bad(w http.ResponseWriter, req *http.Request) (interface{}, error) {
+
+	secret := "MySuperSecretPasscode"
+	secretHeader := "X-Secret"
+
+	headerSecret := req.Header.Get(secretHeader)
+	secretStr := string(secret)
+	if len(secret) != 0 && headerSecret != secretStr {
+		return nil, fmt.Errorf("header %s=%s did not match expected secret", secretHeader, headerSecret)
+	}
+	return nil, nil
+}
+
+func good(w http.ResponseWriter, req *http.Request) (interface{}, error) {
+
+	secret := []byte("MySuperSecretPasscode")
+	secretHeader := "X-Secret"
+
+	headerSecret := req.Header.Get(secretHeader)
+	if len(secret) != 0 && subtle.ConstantTimeCompare(secret, []byte(headerSecret)) != 1 {
+		return nil, fmt.Errorf("header %s=%s did not match expected secret", secretHeader, headerSecret)
+	}
+	return nil, nil
+}
+
+func main() {
+	bad(nil, nil)
+	good(nil, nil)
+}