User-controlled bypass of a comparision

ql/src/experimental/CWE-840/ConditionBypass.qhelp

@@ -0,0 +1,35 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+    <overview>
+        <p>
+Testing untrusted user input against untrusted user input results in
+a bypass of the conditional check as the attacker may modify parameters to match.
+</p>
+    </overview>
+    <recommendation>
+        <p>
+To guard against this, it is advisable to avoid framing a comparision 
+where both sides are untrusted user inputs.
+Instead, use a configuration to store and access the values required.
+</p>
+    </recommendation>
+    <example>
+        <p>
+The following example shows a comparision where both the sides 
+are from attacker controlled request headers. This should be avoided.:
+</p>
+        <sample src="ConditionBypassBad.go" />
+        <p>
+One way to remedy the problem is to test against a value stored in a configuration:
+</p>
+        <sample src="ConditionBypassGood.go" />
+    </example>
+    <references>
+        <li>
+            MITRE:
+            <a href="https://cwe.mitre.org/data/definitions/840.html">
+                CWE-840.
+            </a>
+        </li>
+    </references>
+</qhelp>

ql/src/experimental/CWE-840/ConditionBypass.ql

@@ -0,0 +1,44 @@
+/**
+ * @name Comparision Expression Check Bypass
+ * @description This query tests for user-controlled bypassing
+ *  of a comparision expression i.e. instances where both the
+ *  lhs and rhs of a comparision are user controlled.
+ * @id go/condition-bypass
+ * @kind problem
+ * @problem.severity medium
+ * @tags external/cwe/cwe-840
+ */
+
+import go
+
+/**
+ * A data-flow configuration for reasoning about Condition Bypass.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "Comparision Expression Check Bypass" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof UntrustedFlowSource
+    or
+    exists(string fieldName |
+      source.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", fieldName)
+    |
+      fieldName = "Host"
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(ComparisonExpr c | c.getAnOperand() = sink.asExpr())
+  }
+}
+
+from
+  Configuration config, DataFlow::PathNode lhsSource, DataFlow::PathNode lhs,
+  DataFlow::PathNode rhsSource, DataFlow::PathNode rhs, ComparisonExpr c
+where
+  config.hasFlowPath(rhsSource, rhs) and
+  rhs.getNode().asExpr() = c.getRightOperand() and
+  config.hasFlowPath(lhsSource, lhs) and
+  lhs.getNode().asExpr() = c.getLeftOperand()
+select c, "This comparision is between user controlled operands and "
++ "hence may be bypassed."

ql/src/experimental/CWE-840/ConditionBypassBad.go

@@ -0,0 +1,12 @@
+package main
+
+import (
+	"net/http"
+)
+
+// bad the origin and the host headers are user controlled
+func ex1(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != "http://"+r.Host {
+		//do something
+	}
+}

ql/src/experimental/CWE-840/ConditionBypassGood.go

@@ -0,0 +1,11 @@
+package main
+
+import (
+	"net/http"
+)
+
+func ex1(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != config.get("Host") {
+		//do something
+	}
+}

ql/test/experimental/CWE-840/ConditionBypass.expected

@@ -0,0 +1,2 @@
+| condition.go:9:5:9:46 | ...!=... | This comparision is between user controlled operands and hence may be bypassed. |
+| condition.go:16:5:16:62 | ...!=... | This comparision is between user controlled operands and hence may be bypassed. |

ql/test/experimental/CWE-840/ConditionBypass.qlref

@@ -0,0 +1 @@
+experimental/CWE-840/ConditionBypass.ql

ql/test/experimental/CWE-840/condition.go

@@ -0,0 +1,26 @@
+package main
+
+import (
+	"net/http"
+)
+
+// bad : taken from https://www.gorillatoolkit.org/pkg/websocket
+func ex1(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != "http://"+r.Host {
+		//do something
+	}
+}
+
+// bad both are from remote sources
+func ex2(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != "http://"+r.Header.Get("Header") {
+		//do something
+	}
+}
+
+// good
+func ex3(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != "http://"+"test" {
+		//do something
+	}
+}