diff --git a/change-notes/2020-05-29-open-redirect.md b/change-notes/2020-05-29-open-redirect.md new file mode 100644 index 00000000000..182a001ae09 --- /dev/null +++ b/change-notes/2020-05-29-open-redirect.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The query "Open URL redirect" (`go/unvalidated-url-redirection`) now recognizes values returned by method `http.Request.FormValue` as possibly user controlled, allowing it to flag more true positive results. diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index f32efb143cd..df6785a578e 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -58,7 +58,6 @@ module OpenUrlRedirect { | methName = "Cookie" or methName = "Cookies" or - methName = "FormValue" or methName = "MultipartReader" or methName = "PostFormValues" or methName = "Referer" or 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 88af62567b3..e1c953408ff 100644 --- a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected +++ b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected @@ -15,6 +15,7 @@ edges | stdlib.go:146:11:146:15 | selection of URL : pointer type | stdlib.go:149:24:149:35 | call to String | | stdlib.go:160:35:160:39 | selection of URL : pointer type | stdlib.go:160:24:160:52 | ...+... | | stdlib.go:160:35:160:39 | selection of URL : pointer type | stdlib.go:160:24:160:52 | ...+... | +| stdlib.go:169:13:169:33 | call to FormValue : string | stdlib.go:171:23:171: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 | @@ -44,6 +45,8 @@ nodes | stdlib.go:160:24:160:52 | ...+... | semmle.label | ...+... | | stdlib.go:160:35:160:39 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | | stdlib.go:160:35:160:39 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| stdlib.go:169:13:169:33 | call to FormValue : string | semmle.label | call to FormValue : string | +| stdlib.go:171:23:171: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 | @@ -53,3 +56,4 @@ nodes | 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 | +| stdlib.go:171:23:171:28 | target | stdlib.go:169:13:169:33 | call to FormValue : string | stdlib.go:171:23:171:28 | target | Untrusted URL redirection due to $@. | stdlib.go:169:13:169:33 | call to FormValue | 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 50f74fc8689..edb152684ef 100644 --- a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go +++ b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go @@ -163,5 +163,13 @@ func serveStdlib() { } }) + http.HandleFunc("/ex9", func(w http.ResponseWriter, r *http.Request) { + r.ParseForm() + + target := r.FormValue("target") + // BAD: a request parameter is incorporated without validation into a URL redirect + http.Redirect(w, r, target, 301) + }) + http.ListenAndServe(":80", nil) }