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

Merge pull request #211 from sauyon/open-redirect-fps

Browse Source 
OpenUrlRedirect: resolve some FPs
This commit is contained in:
Max Schaefer
2020-01-29 09:18:07 +00:00
committed by GitHub Enterprise
parent 2b92cd5ba5 41d04f3d96
commit be183596c8
8 changed files with 109 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
ql/src/Security/CWE-601/OpenUrlRedirect.ql
View File

@@ -14,7 +14,12 @@ import go
import semmle.go.security.OpenUrlRedirect::OpenUrlRedirect
import DataFlow::PathGraph
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
from
Configuration cfg, SafeUrlConfiguration scfg, DataFlow::PathNode source, DataFlow::PathNode sink
where
cfg.hasFlowPath(source, sink) and
// this excludes flow from safe parts of request URLs, for example the full URL when the
// doing a redirect from `http://<path>` to `https://<path>`
not scfg.hasFlow(_, sink.getNode())
select sink.getNode(), source, sink, "Untrusted URL redirection due to $@.", source.getNode(),
"user-provided value"

10
ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll
View File

@@ -53,7 +53,7 @@ predicate localAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
tupleStep(pred, succ) or
stringConcatStep(pred, succ) or
sliceStep(pred, succ) or
functionModelStep(pred, succ) or
functionModelStep(_, pred, succ) or
any(AdditionalTaintStep a).step(pred, succ)
}
@@ -92,10 +92,10 @@ predicate sliceStep(DataFlow::Node pred, DataFlow::Node succ) {
}
/** Holds if taint flows from `pred` to `succ` via a function model. */
predicate functionModelStep(DataFlow::Node pred, DataFlow::Node succ) {
exists(FunctionModel m, DataFlow::CallNode c, FunctionInput inp, FunctionOutput outp |
c = m.getACall() and
m.hasTaintFlow(inp, outp) and
predicate functionModelStep(FunctionModel fn, DataFlow::Node pred, DataFlow::Node succ) {
exists(DataFlow::CallNode c, FunctionInput inp, FunctionOutput outp |
c = fn.getACall() and
fn.hasTaintFlow(inp, outp) and
pred = inp.getNode(c) and
succ = outp.getNode(c)
)

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

@@ -6,23 +6,19 @@ import go
private module StdlibHttp {
/** An access to an HTTP request field whose value may be controlled by an untrusted user. */
private class UserControlledRequestField extends UntrustedFlowSource::Range, DataFlow::FieldReadNode {
private class UserControlledRequestField extends UntrustedFlowSource::Range,
DataFlow::FieldReadNode {
UserControlledRequestField() {
exists(Type req, string fieldName |
req.hasQualifiedName("net/http", "Request") and
this.getField() = req.getField(fieldName) |
exists(string fieldName | this.getField().hasQualifiedName("net/http", "Request", fieldName) |
fieldName = "Body" or fieldName = "Form" or fieldName = "Header" or fieldName = "URL"
)
}
}
private class HeaderGetCall extends UntrustedFlowSource::Range, DataFlow::MethodCallNode {
HeaderGetCall() {
this.getTarget().hasQualifiedName("net/http", "Header", "Get")
}
HeaderGetCall() { this.getTarget().hasQualifiedName("net/http", "Header", "Get") }
}
private class StdlibResponseWriter extends HTTP::ResponseWriter::Range {
StdlibResponseWriter() { this.getType().implements("net/http", "ResponseWriter") }

23
ql/src/semmle/go/security/OpenUrlRedirect.qll
View File

@@ -14,7 +14,7 @@ module OpenUrlRedirect {
import OpenUrlRedirectCustomizations::OpenUrlRedirect
/**
* A taint-tracking configuration for reasoning about unvalidated URL redirections.
* A data-flow configuration for reasoning about unvalidated URL redirections.
*/
class Configuration extends DataFlow::Configuration {
Configuration() { this = "OpenUrlRedirect" }
@@ -45,4 +45,25 @@ module OpenUrlRedirect {
guard instanceof BarrierGuard
}
}
/**
* A data-flow configuration for reasoning about safe URLs for unvalidated URL redirections.
*/
class SafeUrlConfiguration extends DataFlow::Configuration {
SafeUrlConfiguration() { this = "SafeUrlFlow" }
override predicate isSource(DataFlow::Node source) {
source.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", "URL")
}
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
TaintTracking::functionModelStep(any(SafeUrlMethod m), pred, succ)
or
exists(DataFlow::FieldReadNode frn | succ = frn |
frn.getBase() = pred and frn.getFieldName() = "Host"
)
}
}
}

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

@@ -28,6 +28,21 @@ module OpenUrlRedirect {
*/
abstract class BarrierGuard extends DataFlow::BarrierGuard { }
/**
* A method on a `net/url.URL` that is considered safe to redirect to.
*/
class SafeUrlMethod extends TaintTracking::FunctionModel, Method {
SafeUrlMethod() {
this instanceof StringMethod
or
exists(string m | this.hasQualifiedName("net/url", "URL", m) | m = "Hostname" or m = "Port")
}
override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {
inp.isReceiver() and outp.isResult()
}
}
/**
* A source of third-party user input, considered as a flow source for URL redirects.
*/
@@ -70,11 +85,13 @@ module OpenUrlRedirect {
}
/**
* A call to a function called `isLocalUrl` or similar, which is
* A call to a function called `isLocalUrl`, `isValidRedirect`, or similar, which is
* considered a barrier for purposes of URL redirection.
*/
class LocalUrlBarrierGuard extends BarrierGuard, DataFlow::CallNode {
LocalUrlBarrierGuard() { this.getCalleeName().regexpMatch("(?i)(is_?)?local_?url") }
class RedirectCheckBarrierGuard extends BarrierGuard, DataFlow::CallNode {
RedirectCheckBarrierGuard() {
this.getCalleeName().regexpMatch("(?i)(is_?)?(local_?url|valid_?redir(ect)?)")
}
override predicate checks(Expr e, boolean outcome) {
// `isLocalUrl(e)` is a barrier for `e` if it evaluates to `true`

10
ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected
View File

@@ -6,6 +6,9 @@ edges
| stdlib.go:43:13:43:18 | selection of Form : Values | stdlib.go:45:23:45:28 | target |
| stdlib.go:63:13:63:18 | selection of Form : Values | stdlib.go:66:23:66:40 | ...+... |
| stdlib.go:88:13:88:18 | selection of Form : Values | stdlib.go:91:23:91:28 | target |
| stdlib.go:112:24:112:28 | selection of URL : pointer type | stdlib.go:112:24:112:37 | call to String |
| stdlib.go:112:24:112:28 | selection of URL : pointer type | stdlib.go:112:24:112:37 | call to String |
| stdlib.go:133:13:133:18 | selection of Form : Values | stdlib.go:139:23:139:28 | target |
nodes
| OpenUrlRedirect.go:10:23:10:28 | selection of Form : Values | semmle.label | selection of Form : Values |
| OpenUrlRedirect.go:10:23:10:42 | call to Get | semmle.label | call to Get |
@@ -21,6 +24,12 @@ nodes
| stdlib.go:66:23:66:40 | ...+... | semmle.label | ...+... |
| stdlib.go:88:13:88:18 | selection of Form : Values | semmle.label | selection of Form : Values |
| stdlib.go:91:23:91:28 | target | semmle.label | target |
| stdlib.go:112:24:112:28 | selection of URL : pointer type | semmle.label | selection of URL : pointer type |
| stdlib.go:112:24:112:28 | selection of URL : pointer type | semmle.label | selection of URL : pointer type |
| stdlib.go:112:24:112:37 | call to String | semmle.label | call to String |
| stdlib.go:112:24:112:37 | call to String | semmle.label | call to String |
| stdlib.go:133:13:133:18 | selection of Form : Values | semmle.label | selection of Form : Values |
| stdlib.go:139:23:139:28 | target | semmle.label | target |
#select
| OpenUrlRedirect.go:10:23:10:42 | call to Get | OpenUrlRedirect.go:10:23:10:28 | selection of Form : Values | OpenUrlRedirect.go:10:23:10:42 | call to Get | Untrusted URL redirection due to $@. | OpenUrlRedirect.go:10:23:10:28 | selection of Form | user-provided value |
| stdlib.go:14:30:14:35 | target | stdlib.go:12:13:12:18 | selection of Form : Values | stdlib.go:14:30:14:35 | target | Untrusted URL redirection due to $@. | stdlib.go:12:13:12:18 | selection of Form | user-provided value |
@@ -29,3 +38,4 @@ nodes
| stdlib.go:45:23:45:28 | target | stdlib.go:43:13:43:18 | selection of Form : Values | stdlib.go:45:23:45:28 | target | Untrusted URL redirection due to $@. | stdlib.go:43:13:43:18 | selection of Form | user-provided value |
| stdlib.go:66:23:66:40 | ...+... | stdlib.go:63:13:63:18 | selection of Form : Values | stdlib.go:66:23:66:40 | ...+... | Untrusted URL redirection due to $@. | stdlib.go:63:13:63:18 | selection of Form | user-provided value |
| stdlib.go:91:23:91:28 | target | stdlib.go:88:13:88:18 | selection of Form : Values | stdlib.go:91:23:91:28 | target | Untrusted URL redirection due to $@. | stdlib.go:88:13:88:18 | selection of Form | user-provided value |
| stdlib.go:139:23:139:28 | target | stdlib.go:133:13:133:18 | selection of Form : Values | stdlib.go:139:23:139:28 | target | Untrusted URL redirection due to $@. | stdlib.go:133:13:133:18 | selection of Form | user-provided value |

36
ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go
View File

@@ -103,5 +103,41 @@ func serveStdlib() {
}
})
http.HandleFunc("/ex8", func(w http.ResponseWriter, r *http.Request) {
r.ParseForm()
// GOOD: this only rewrites the scheme, which is not dangerous as the host cannot change.
if r.URL.Scheme == "http" {
r.URL.Scheme = "https"
http.Redirect(w, r, r.URL.String(), 302)
} else {
// ...
}
})
http.HandleFunc("/ex8", func(w http.ResponseWriter, r *http.Request) {
r.ParseForm()
target := r.Form.Get("target")
// GOOD: a check is done on the URL
if isValidRedirect(target) {
http.Redirect(w, r, target, 302)
} else {
// ...
}
})
http.HandleFunc("/ex9", func(w http.ResponseWriter, r *http.Request) {
r.ParseForm()
target := r.Form.Get("target")
// GOOD, but we catch this anyway: a check is done on the URL
if !isValidRedirect(target) {
target = "/"
}
http.Redirect(w, r, target, 302)
})
http.ListenAndServe(":80", nil)
}

5
ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/util.go
View File

@@ -3,3 +3,8 @@ package main
const HASH = "#"
func someUrl() string { return "semmle.com" }
// placeholder
func isValidRedirect(s string) bool {
return true
}