From 92576e9c117316b422178968fc447402828c77a5 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Tue, 28 Apr 2020 23:11:48 +0530 Subject: [PATCH] User-controlled bypass of sensitive action --- .../CWE-807/SensitiveConditionBypass.qhelp | 29 ++++++++++ .../CWE-807/SensitiveConditionBypass.ql | 35 ++++++++++++ .../CWE-807/SensitiveConditionBypass.qll | 57 +++++++++++++++++++ .../CWE-807/SensitiveConditionBypassBad.go | 6 ++ .../CWE-807/SensitiveConditionBypass.expected | 3 + .../CWE-807/SensitiveConditionBypass.qlref | 1 + ql/test/experimental/CWE-807/condition.go | 33 +++++++++++ 7 files changed, 164 insertions(+) create mode 100644 ql/src/experimental/CWE-807/SensitiveConditionBypass.qhelp create mode 100644 ql/src/experimental/CWE-807/SensitiveConditionBypass.ql create mode 100644 ql/src/experimental/CWE-807/SensitiveConditionBypass.qll create mode 100644 ql/src/experimental/CWE-807/SensitiveConditionBypassBad.go create mode 100644 ql/test/experimental/CWE-807/SensitiveConditionBypass.expected create mode 100644 ql/test/experimental/CWE-807/SensitiveConditionBypass.qlref create mode 100644 ql/test/experimental/CWE-807/condition.go diff --git a/ql/src/experimental/CWE-807/SensitiveConditionBypass.qhelp b/ql/src/experimental/CWE-807/SensitiveConditionBypass.qhelp new file mode 100644 index 00000000000..67422e53a28 --- /dev/null +++ b/ql/src/experimental/CWE-807/SensitiveConditionBypass.qhelp @@ -0,0 +1,29 @@ + + + +

+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. +

+ + +

+ 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. + + +

+The following example shows a comparision where an user controlled +expression is used to guard a sensitive method. This should be avoided.: +

+ + + diff --git a/ql/src/experimental/CWE-807/SensitiveConditionBypass.ql b/ql/src/experimental/CWE-807/SensitiveConditionBypass.ql new file mode 100644 index 00000000000..61798836e77 --- /dev/null +++ b/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" diff --git a/ql/src/experimental/CWE-807/SensitiveConditionBypass.qll b/ql/src/experimental/CWE-807/SensitiveConditionBypass.qll new file mode 100644 index 00000000000..77a503e7dc7 --- /dev/null +++ b/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()) + } +} diff --git a/ql/src/experimental/CWE-807/SensitiveConditionBypassBad.go b/ql/src/experimental/CWE-807/SensitiveConditionBypassBad.go new file mode 100644 index 00000000000..e2ca02615db --- /dev/null +++ b/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() + } +} diff --git a/ql/test/experimental/CWE-807/SensitiveConditionBypass.expected b/ql/test/experimental/CWE-807/SensitiveConditionBypass.expected new file mode 100644 index 00000000000..dcc3edfce51 --- /dev/null +++ b/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 | diff --git a/ql/test/experimental/CWE-807/SensitiveConditionBypass.qlref b/ql/test/experimental/CWE-807/SensitiveConditionBypass.qlref new file mode 100644 index 00000000000..da2ab35074a --- /dev/null +++ b/ql/test/experimental/CWE-807/SensitiveConditionBypass.qlref @@ -0,0 +1 @@ +experimental/CWE-807/SensitiveConditionBypass.ql diff --git a/ql/test/experimental/CWE-807/condition.go b/ql/test/experimental/CWE-807/condition.go new file mode 100644 index 00000000000..d8861b53201 --- /dev/null +++ b/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() + } +}