diff --git a/ql/src/Security/CWE-601/OpenUrlRedirect.ql b/ql/src/Security/CWE-601/OpenUrlRedirect.ql index 8ef2d574702..daa79e9258a 100644 --- a/ql/src/Security/CWE-601/OpenUrlRedirect.ql +++ b/ql/src/Security/CWE-601/OpenUrlRedirect.ql @@ -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://` to `https://` + not scfg.hasFlow(_, sink.getNode()) select sink.getNode(), source, sink, "Untrusted URL redirection due to $@.", source.getNode(), "user-provided value" diff --git a/ql/src/semmle/go/security/OpenUrlRedirect.qll b/ql/src/semmle/go/security/OpenUrlRedirect.qll index c85baa641f0..d32105c68a0 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirect.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirect.qll @@ -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" + ) + } + } } diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 7fcee322490..c0a91d81e15 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -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`. diff --git a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected index c230842b32e..b07fd605931 100644 --- a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected +++ b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected @@ -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