From abfdd7ee1e5873c9a6a8fc416dfeecf3bbae0132 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 9 Jan 2020 01:09:19 -0800 Subject: [PATCH] OpenUrlRedirect: make functions like isValidRedirect barrier guards --- .../OpenUrlRedirectCustomizations.qll | 8 ++++--- .../OpenUrlRedirect/OpenUrlRedirect.expected | 4 ++++ .../CWE-601/OpenUrlRedirect/stdlib.go | 24 +++++++++++++++++++ .../Security/CWE-601/OpenUrlRedirect/util.go | 5 ++++ 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 4c3d1124e29..6285fbec60e 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -101,11 +101,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` 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 61b5890e01b..c230842b32e 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,7 @@ 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: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 +22,8 @@ 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: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 +32,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 | diff --git a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go index f6d663e3550..7e24a892f83 100644 --- a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go +++ b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go @@ -115,5 +115,29 @@ func serveStdlib() { } }) + 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) } diff --git a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/util.go b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/util.go index 5c074166432..86d2563652c 100644 --- a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/util.go +++ b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/util.go @@ -3,3 +3,8 @@ package main const HASH = "#" func someUrl() string { return "semmle.com" } + +// placeholder +func isValidRedirect(s string) { + return true +}