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

Apply review comments

Browse Source
This commit is contained in:
Sauyon Lee
2020-01-28 07:39:36 -08:00
parent 3eee780fdd
commit d2e5322b94
4 changed files with 39 additions and 46 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"

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

@@ -14,17 +14,14 @@ 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" }
override predicate isSource(DataFlow::Node source) { source instanceof Source }
override predicate isSink(DataFlow::Node sink) {
sink instanceof Sink and
not sink instanceof NotSink
}
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier }
@@ -48,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"
)
}
}
}

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

@@ -18,14 +18,6 @@ module OpenUrlRedirect {
*/
abstract class Sink extends DataFlow::Node { }
/**
* A data flow node that should not be considered a sink for unvalidated URL redirect
* vulnerabilities.
*
* This takes precedence over `Sink`, and so can be used to remove sinks that are safe.
*/
abstract class NotSink extends DataFlow::Node { }
/**
* A barrier for unvalidated URL redirect vulnerabilities.
*/
@@ -37,17 +29,12 @@ module OpenUrlRedirect {
abstract class BarrierGuard extends DataFlow::BarrierGuard { }
/**
* A source of third-party user input, considered as a flow source for URL redirects.
*
* Excludes the URL field, as it is handled more precisely in `UntrustedUrlField`.
* A method on a `net/url.URL` that is considered safe to redirect to.
*/
class UntrustedFlowAsSource extends Source, UntrustedFlowSource { }
class SafeUrlMethod extends TaintTracking::FunctionModel, Method {
SafeUrlMethod() {
this instanceof StringMethod
or
this instanceof URL::UrlGetter and
exists(string m | this.hasQualifiedName("net/url", "URL", m) | m = "Hostname" or m = "Port")
}
@@ -56,33 +43,10 @@ module OpenUrlRedirect {
}
}
private class SafeUrlConfiguration extends DataFlow2::Configuration {
SafeUrlConfiguration() { this = "FullUrlFlow" }
override predicate isSource(DataFlow::Node source) {
exists(Type req | req.hasQualifiedName("net/http", "Request") |
source.(DataFlow::FieldReadNode).getField() = req.getField("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"
)
}
}
/**
* A safe part of a request URL to redirect to. This includes values such as the hostname and port
* which should not be considered as unsafe when redirected to.
* A source of third-party user input, considered as a flow source for URL redirects.
*/
class SafeUrl extends NotSink, DataFlow::Node {
SafeUrl() { exists(SafeUrlConfiguration conf | conf.hasFlow(_, this)) }
}
class UntrustedFlowAsSource extends Source, UntrustedFlowSource { }
/**
* An HTTP redirect, considered as a sink for `Configuration`.

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

@@ -6,6 +6,8 @@ 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 |
@@ -22,6 +24,10 @@ 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