include changes from review

This commit is contained in:
Porcupiney Hairs
2020-05-11 04:05:41 +05:30
parent 9948596e2c
commit 4aba80b0bd
3 changed files with 10 additions and 23 deletions

View File

@@ -8,15 +8,15 @@ a bypass of the conditional check as the attacker may modify parameters to match
</overview>
<recommendation>
<p>
To guard against this, it is advisable to avoid framing a comparision
To guard against this, it is advisable to avoid framing a comparison
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.:
The following example shows a comparison where both the sides
are from attacker-controlled request headers. This should be avoided.:
</p>
<sample src="ConditionBypassBad.go" />
<p>
@@ -24,12 +24,4 @@ One way to remedy the problem is to test against a value stored in a configurati
</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>

View File

@@ -1,11 +1,10 @@
/**
* @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.
* @description Comparing two user controlled inputs may
* lead to an effective bypass of the comparison check.
* @id go/condition-bypass
* @kind problem
* @problem.severity medium
* @problem.severity warning
* @tags external/cwe/cwe-840
*/
@@ -20,11 +19,7 @@ class Configuration extends TaintTracking::Configuration {
override predicate isSource(DataFlow::Node source) {
source instanceof UntrustedFlowSource
or
exists(string fieldName |
source.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", fieldName)
|
fieldName = "Host"
)
source = any(Field f | f.hasQualifiedName("net/http", "Request", "Host")).getARead()
}
override predicate isSink(DataFlow::Node sink) {
@@ -40,5 +35,5 @@ where
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."
select c, "This comparision is between user controlled operands derived from $@", lhsSource,
" and $@", rhsSource, "hence may be bypassed."

View File

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