hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-29 18:55:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #141 from sauyon/reflectedxss-fps

Browse Source 
ReflectedXss improvements
This commit is contained in:
Max Schaefer
2020-05-15 07:23:39 +01:00
committed by GitHub
parent 1fbf552ef5 5e633b2c74
commit d41e41812b
12 changed files with 110 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
change-notes/2020-05-11-reflected-xss.md Normal file
View File

@@ -0,0 +1,3 @@
lgtm,codescanning
* The query "Reflected cross-site scripting" has been improved to recognize more cases where the
value should be considered to be safe, which should lead to fewer false positive results.

13
ql/src/Security/CWE-601/BadRedirectCheck.ql
View File

@@ -141,15 +141,6 @@ predicate isBadRedirectCheckOrWrapper(DataFlow::Node check, SsaWithFields v) {
)
}
/**
* Gets an SSA-with-fields variable that is similar to `v` in the sense that it has the same
* root variable and the same sequence of field accesses.
*/
SsaWithFields similar(SsaWithFields v) {
result.getBaseVariable().getSourceVariable() = v.getBaseVariable().getSourceVariable() and
result.getQualifiedName() = v.getQualifiedName()
}
/**
* Holds if `check` checks that `v` has a leading slash, but not whether it has another slash or a
* backslash in its second position.
@@ -161,8 +152,8 @@ predicate isBadRedirectCheck(DataFlow::Node check, SsaWithFields v) {
// (we allow those checks to be on variables that are most likely equivalent to `v`
// to rule out false positives due to minor variations in data flow)
not (
isCheckedForSecondSlash(similar(v)) and
isCheckedForSecondBackslash(similar(v))
isCheckedForSecondSlash(v.similar()) and
isCheckedForSecondBackslash(v.similar())
)
}

6
ql/src/semmle/go/Concepts.qll
View File

@@ -366,8 +366,8 @@ module HTTP {
* extend `HTTP::ResponseWriter` instead.
*/
abstract class Range extends Variable {
/** Gets a data-flow node that represents this response writer. */
DataFlow::Node getANode() { result = this.getARead().getASuccessor*() }
/** Gets a data-flow node that is a use of this response writer. */
abstract DataFlow::Node getANode();
}
}
@@ -391,7 +391,7 @@ module HTTP {
/** Gets a redirect that is sent in this HTTP response. */
Redirect getARedirect() { result.getResponseWriter() = this }
/** Gets a data-flow node that represents this response writer. */
/** Gets a data-flow node that is a use of this response writer. */
DataFlow::Node getANode() { result = self.getANode() }
}

16
ql/src/semmle/go/dataflow/SSA.qll
View File

@@ -338,6 +338,13 @@ class SsaWithFields extends TSsaWithFields {
/** Gets a use that refers to this SSA variable with fields. */
DataFlow::Node getAUse() { this = accessPath(result.asInstruction()) }
/** Gets the type of this SSA variable with fields. */
Type getType() {
exists(SsaVariable var | this = TRoot(var) | result = var.getType())
or
exists(Field f | this = TStep(_, f) | result = f.getType())
}
/** Gets a textual representation of this element. */
string toString() {
exists(SsaVariable var | this = TRoot(var) | result = "(" + var + ")")
@@ -345,6 +352,15 @@ class SsaWithFields extends TSsaWithFields {
exists(SsaWithFields base, Field f | this = TStep(base, f) | result = base + "." + f.getName())
}
/**
* Gets an SSA-with-fields variable that is similar to this SSA-with-fields variable in the
* sense that it has the same root variable and the same sequence of field accesses.
*/
SsaWithFields similar() {
result.getBaseVariable().getSourceVariable() = this.getBaseVariable().getSourceVariable() and
result.getQualifiedName() = this.getQualifiedName()
}
/**
* Gets the qualified name of the source variable or variable and fields that this represents.
*

3
ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
View File

@@ -758,6 +758,9 @@ class EqualityTestNode extends BinaryOperationNode, ExprNode {
outcome = expr.getPolarity() and
expr.hasOperands(lhs.asExpr(), rhs.asExpr())
}
/** Gets the polarity of this equality test, that is, `true` for `==` and `false` for `!=`. */
boolean getPolarity() { result = expr.getPolarity() }
}
/**

14
ql/src/semmle/go/frameworks/HTTP.qll
View File

@@ -54,13 +54,21 @@ private module StdlibHttp {
}
}
/** The declaration of a variable which either is or has a field that implements the http.ResponseWriter type */
private class StdlibResponseWriter extends HTTP::ResponseWriter::Range {
StdlibResponseWriter() { this.getType().implements("net/http", "ResponseWriter") }
SsaWithFields v;
StdlibResponseWriter() {
this = v.getBaseVariable().getSourceVariable() and
exists(Type t | t.implements("net/http", "ResponseWriter") | v.getType() = t)
}
override DataFlow::Node getANode() { result = v.similar().getAUse().getASuccessor*() }
/** Gets a header object that corresponds to this HTTP response. */
DataFlow::MethodCallNode getAHeaderObject() {
result.getTarget().hasQualifiedName("net/http", _, "Header") and
this.getARead() = result.getReceiver()
result.getTarget().getName() = "Header" and
this.getANode() = result.getReceiver()
}
}

9
ql/src/semmle/go/frameworks/Macaron.qll
View File

@@ -6,11 +6,16 @@ import go
private module Macaron {
private class Context extends HTTP::ResponseWriter::Range {
SsaWithFields v;
Context() {
this = v.getBaseVariable().getSourceVariable() and
exists(Method m | m.hasQualifiedName("gopkg.in/macaron.v1", "Context", "Redirect") |
m = this.getType().getMethod("Redirect")
v.getType().getMethod("Redirect") = m
)
}
override DataFlow::Node getANode() { result = v.similar().getAUse().getASuccessor*() }
}
private class RedirectCall extends HTTP::Redirect::Range, DataFlow::MethodCallNode {
@@ -20,6 +25,6 @@ private module Macaron {
override DataFlow::Node getUrl() { result = this.getArgument(0) }
override HTTP::ResponseWriter getResponseWriter() { result.getARead() = this.getReceiver() }
override HTTP::ResponseWriter getResponseWriter() { result.getANode() = this.getReceiver() }
}
}

2
ql/src/semmle/go/frameworks/Stdlib.qll
View File

@@ -791,7 +791,7 @@ module Log {
/** Provides models of some functions in the `encoding/json` package. */
module EncodingJson {
private class MarshalFunction extends TaintTracking::FunctionModel, MarshalingFunction::Range {
class MarshalFunction extends TaintTracking::FunctionModel, MarshalingFunction::Range {
MarshalFunction() {
this.hasQualifiedName("encoding/json", "Marshal") or
this.hasQualifiedName("encoding/json", "MarshalIndent")

2
ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll
View File

@@ -137,7 +137,7 @@ module OpenUrlRedirect {
}
override predicate checks(Expr e, boolean outcome) {
e = url.asExpr() and this.eq(outcome, _, _)
e = url.asExpr() and outcome = this.getPolarity()
}
}

21
ql/src/semmle/go/security/ReflectedXssCustomizations.qll
View File

@@ -59,7 +59,7 @@ module ReflectedXss {
not htmlTypeSpecified(body) and
(
exists(HTTP::HeaderWrite hw | hw = body.getResponseWriter().getAHeaderWrite() |
hw.definesHeader("content-type", _)
hw.getName().getStringValue().toLowerCase() = "content-type"
)
or
exists(DataFlow::CallNode call | call.getTarget().hasQualifiedName("fmt", "Fprintf") |
@@ -69,6 +69,14 @@ module ReflectedXss {
// - '%', which could be a format string.
call.getArgument(1).getStringValue().regexpMatch("^[^<%].*")
)
or
exists(DataFlow::Node pred | body = pred.getASuccessor*() |
// data starting with a character other than `<` cannot cause an HTML content type to be detected.
pred.getStringValue().regexpMatch("^[^<].*")
or
// json data cannot begin with `<`
pred = any(EncodingJson::MarshalFunction mf).getOutput().getExitNode(_)
)
)
}
@@ -95,4 +103,15 @@ module ReflectedXss {
)
}
}
/**
* A check against a constant value, considered a barrier for reflected XSS.
*/
class EqualityTestGuard extends SanitizerGuard, DataFlow::EqualityTestNode {
override predicate checks(Expr e, boolean outcome) {
this.getAnOperand().isConst() and
e = this.getAnOperand().asExpr() and
outcome = this.getPolarity()
}
}
}

12
ql/test/query-tests/Security/CWE-079/ReflectedXss.expected
View File

@@ -2,7 +2,8 @@ edges
| ReflectedXss.go:11:15:11:20 | selection of Form : Values | ReflectedXss.go:14:44:14:51 | username |
| contenttype.go:11:11:11:16 | selection of Form : Values | contenttype.go:17:11:17:22 | type conversion |
| contenttype.go:49:11:49:16 | selection of Form : Values | contenttype.go:53:34:53:37 | data |
| tst.go:11:15:11:20 | selection of Form : Values | tst.go:15:12:15:39 | type conversion |
| tst.go:13:15:13:20 | selection of Form : Values | tst.go:17:12:17:39 | type conversion |
| tst.go:47:14:47:19 | selection of Form : Values | tst.go:52:12:52:26 | type conversion |
nodes
| ReflectedXss.go:11:15:11:20 | selection of Form : Values | semmle.label | selection of Form : Values |
| ReflectedXss.go:14:44:14:51 | username | semmle.label | username |
@@ -10,10 +11,13 @@ nodes
| contenttype.go:17:11:17:22 | type conversion | semmle.label | type conversion |
| contenttype.go:49:11:49:16 | selection of Form : Values | semmle.label | selection of Form : Values |
| contenttype.go:53:34:53:37 | data | semmle.label | data |
| tst.go:11:15:11:20 | selection of Form : Values | semmle.label | selection of Form : Values |
| tst.go:15:12:15:39 | type conversion | semmle.label | type conversion |
| tst.go:13:15:13:20 | selection of Form : Values | semmle.label | selection of Form : Values |
| tst.go:17:12:17:39 | type conversion | semmle.label | type conversion |
| tst.go:47:14:47:19 | selection of Form : Values | semmle.label | selection of Form : Values |
| tst.go:52:12:52:26 | type conversion | semmle.label | type conversion |
#select
| ReflectedXss.go:14:44:14:51 | username | ReflectedXss.go:11:15:11:20 | selection of Form : Values | ReflectedXss.go:14:44:14:51 | username | Cross-site scripting vulnerability due to $@. | ReflectedXss.go:11:15:11:20 | selection of Form | user-provided value |
| contenttype.go:17:11:17:22 | type conversion | contenttype.go:11:11:11:16 | selection of Form : Values | contenttype.go:17:11:17:22 | type conversion | Cross-site scripting vulnerability due to $@. | contenttype.go:11:11:11:16 | selection of Form | user-provided value |
| contenttype.go:53:34:53:37 | data | contenttype.go:49:11:49:16 | selection of Form : Values | contenttype.go:53:34:53:37 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:49:11:49:16 | selection of Form | user-provided value |
| tst.go:15:12:15:39 | type conversion | tst.go:11:15:11:20 | selection of Form : Values | tst.go:15:12:15:39 | type conversion | Cross-site scripting vulnerability due to $@. | tst.go:11:15:11:20 | selection of Form | user-provided value |
| tst.go:17:12:17:39 | type conversion | tst.go:13:15:13:20 | selection of Form : Values | tst.go:17:12:17:39 | type conversion | Cross-site scripting vulnerability due to $@. | tst.go:13:15:13:20 | selection of Form | user-provided value |
| tst.go:52:12:52:26 | type conversion | tst.go:47:14:47:19 | selection of Form : Values | tst.go:52:12:52:26 | type conversion | Cross-site scripting vulnerability due to $@. | tst.go:47:14:47:19 | selection of Form | user-provided value |

35
ql/test/query-tests/Security/CWE-079/tst.go
View File

@@ -1,6 +1,8 @@
package main
import (
"encoding/json"
"fmt"
"net/http"
"strings"
)
@@ -19,3 +21,36 @@ func serve6() {
})
http.ListenAndServe(":80", nil)
}
type User struct {
name string
}
func serve7() {
http.HandleFunc("/user", func(w http.ResponseWriter, r *http.Request) {
r.ParseForm()
username := r.Form.Get("username")
if !isValidUsername(username) {
// OK: json data cannot cause an HTML content type to be detected
a, _ := json.Marshal(User{username})
w.Write(a)
} else {
// TODO: do something exciting
}
})
http.ListenAndServe(":80", nil)
}
func serve8() {
http.HandleFunc("/user", func(w http.ResponseWriter, r *http.Request) {
r.ParseForm()
service := r.Form.Get("service")
if service != "service1" && service != "service2" {
fmt.Fprintln(w, "Service not found")
} else {
// OK (service is known to be either "service1" or "service2" here), but currently flagged
w.Write([]byte(service))
}
})
http.ListenAndServe(":80", nil)
}