User-controlled bypass of sensitive action

ql/src/experimental/CWE-807/SensitiveConditionBypass.qhelp

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+    <overview>
+        <p>
+Testing untrusted user input against a fixed constant results in
+a bypass of the conditional check as the attacker may alter the input to match the constant.
+When an incorrect check of this type is used to guard a potentially sensitive block, 
+it results an attacker gaining access to the sensitive block.
+</p>
+    </overview>
+    <recommendation>
+        <p>
+ Never decide whether to authenticate a user based on data that may be controlled by that user. 
+ If necessary, ensure that the data is validated extensively when it is input before any 
+ authentication checks are performed.
+
+It is still possible to have a system that "remembers" users, thus not requiring 
+the user to login on every interaction. For example, personalization settings can be applied 
+without authentication because this is not sensitive information. However, users
+should be allowed to take sensitive actions only when they have been fully authenticated. 
+    </recommendation>
+    <example>
+        <p>
+The following example shows a comparision where an user controlled 
+expression is used to guard a sensitive method. This should be avoided.:
+</p>
+        <sample src="SensitiveConditionBypassBad.go" />
+    </example>
+</qhelp>

ql/src/experimental/CWE-807/SensitiveConditionBypass.ql

@@ -0,0 +1,35 @@
+/**
+ * @name User-controlled bypassing of sensitive action
+ * @description This query tests for user-controlled bypassing
+ *  of sensitive actions.
+ * @id go/sensitive-condition-bypass
+ * @kind problem
+ * @problem.severity high
+ * @tags external/cwe/cwe-807
+ *       external/cwe/cwe-247
+ *       external/cwe/cwe-350
+ */
+
+import go
+import SensitiveConditionBypass
+
+from
+  ControlFlow::ConditionGuardNode guard, DataFlow::Node sensitiveSink,
+  SensitiveExpr::Classification classification, Configuration config, DataFlow::PathNode source,
+  DataFlow::PathNode operand, DataFlow::PathNode constOperand, DataFlow::PathNode constSource,
+  ComparisonExpr comp, ConstConfiguration constConfig
+where
+  // there should be a flow between source and the operand sink
+  config.hasFlowPath(source, operand) and
+  // A constant string value should flow to a sink
+  constConfig.hasFlowPath(constSource, constOperand) and
+  // both the operand should belong to the same comparision expression
+  operand.getNode().asExpr() = comp.getAnOperand() and
+  constOperand.getNode().asExpr() = comp.getAnOperand() and
+  // get the ConditionGuardNode corresponding to the comparision expr.
+  guard.getCondition() = comp and
+  // the sink `sensitiveSink` should be sensitive,
+  isSensitive(sensitiveSink, classification) and
+  // the guard should control the sink
+  guard.dominates(sensitiveSink.getBasicBlock())
+select comp, "This sensitive comparision check can potentially be bypassed"

ql/src/experimental/CWE-807/SensitiveConditionBypass.qll

@@ -0,0 +1,57 @@
+import go
+import semmle.go.security.SensitiveActions
+
+/**
+ * Holds if `sink` is used in a context that suggests it may hold sensitive data of
+ * the given `type`.
+ */
+predicate isSensitive(DataFlow::Node sink, SensitiveExpr::Classification type) {
+  exists(Write write, string name |
+    write.getRhs() = sink and
+    name = write.getLhs().getName() and
+    // whitelist obvious test password variables
+    not name.regexpMatch(HeuristicNames::notSensitive())
+  |
+    name.regexpMatch(HeuristicNames::maybeSensitive(type))
+  )
+  or
+  exists(SensitiveCall a | sink.asExpr() = a and a.getClassification() = type)
+  or
+  exists(SensitiveExpr a | sink.asExpr() = a and a.getClassification() = type)
+  or
+  exists(SensitiveAction a | a = sink and type = SensitiveExpr::secret())
+}
+
+/**
+ * A data-flow configuration for reasoning about
+ * user-controlled bypassing of sensitive actions.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "Condtional Expression Check Bypass" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof UntrustedFlowSource
+    or
+    exists(DataFlow::FieldReadNode f |
+      f.getField().hasQualifiedName("net/http", "Request", "Host")
+    |
+      source = f
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(ComparisonExpr c | c.getAnOperand() = sink.asExpr())
+  }
+}
+
+class ConstConfiguration extends DataFlow::Configuration {
+  ConstConfiguration() { this = "Constant expression flow" }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists(string val | source.getStringValue() = val)
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(ComparisonExpr c | c.getAnOperand() = sink.asExpr())
+  }
+}

ql/src/experimental/CWE-807/SensitiveConditionBypassBad.go

@@ -0,0 +1,6 @@
+func ex3(w http.ResponseWriter, r *http.Request) {
+	test2 := "test"
+	if r.Header.Get("X-Password") != test2 {
+		login()
+	}
+}

ql/test/experimental/CWE-807/SensitiveConditionBypass.expected

@@ -0,0 +1,3 @@
+| condition.go:14:5:14:34 | ...!=... | This sensitive comparision check can potentially be bypassed |
+| condition.go:22:5:22:35 | ...!=... | This sensitive comparision check can potentially be bypassed |
+| condition.go:30:5:30:35 | ...!=... | This sensitive comparision check can potentially be bypassed |

ql/test/experimental/CWE-807/SensitiveConditionBypass.qlref

@@ -0,0 +1 @@
+experimental/CWE-807/SensitiveConditionBypass.ql

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

@@ -0,0 +1,33 @@
+package main
+
+import (
+	"io"
+	"net/http"
+)
+
+func use(xs ...interface{}) {}
+
+var test = "localhost"
+
+// bad both are from remote sources
+func ex1(w http.ResponseWriter, r *http.Request) {
+	if r.Header.Get("Origin") != test {
+		authkey := "randomDatta"
+		io.WriteString(w, authkey)
+	}
+}
+
+func ex2(w http.ResponseWriter, r *http.Request) {
+	test2 := "test"
+	if r.Header.Get("Origin") != test2 {
+		authkey := "randomDatta2"
+		io.WriteString(w, authkey)
+	}
+}
+
+func ex3(w http.ResponseWriter, r *http.Request) {
+	test2 := "test"
+	if r.Header.Get("Origin") != test2 {
+		login()
+	}
+}