From 4e6d9b3811993434d4413469547cfeb89f22fd67 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 23 Jun 2020 15:29:18 +0100 Subject: [PATCH 1/3] Teach `OpenUrlRedirect` to propagate out of `URL.Path` and a few other fields. --- ql/src/semmle/go/security/OpenUrlRedirect.qll | 7 ++ .../OpenUrlRedirect/OpenUrlRedirect.expected | 116 ++++++++++-------- .../CWE-601/OpenUrlRedirect/stdlib.go | 11 ++ 3 files changed, 82 insertions(+), 52 deletions(-) diff --git a/ql/src/semmle/go/security/OpenUrlRedirect.qll b/ql/src/semmle/go/security/OpenUrlRedirect.qll index c639aeb00d1..67477b764f3 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirect.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirect.qll @@ -37,6 +37,13 @@ module OpenUrlRedirect { exists(Write w, Field f, SsaWithFields v | f.hasQualifiedName("net/url", "URL", "Host") | w.writesField(v.getAUse(), f, pred) and succ = v.getAUse() ) + or + // propagate out of most URL fields, but not `Scheme` and `User` + exists(Field f | + f.hasQualifiedName("net/url", "URL", ["Fragment", "Host", "Path", "RawPath", "RawQuery"]) + | + succ.(Read).readsField(pred, f) + ) } override predicate isBarrierOut(DataFlow::Node node) { 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 e1c953408ff..67aa2575c29 100644 --- a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected +++ b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected @@ -1,59 +1,71 @@ edges | OpenUrlRedirect.go:10:23:10:28 | selection of Form : Values | OpenUrlRedirect.go:10:23:10:42 | call to Get | -| stdlib.go:12:13:12:18 | selection of Form : Values | stdlib.go:14:30:14:35 | target | -| stdlib.go:21:13:21:18 | selection of Form : Values | stdlib.go:23:30:23:35 | target | -| stdlib.go:30:13:30:18 | selection of Form : Values | stdlib.go:34:30:34:39 | ...+... | -| 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 | -| stdlib.go:146:11:146:15 | selection of URL : pointer type | stdlib.go:146:11:146:15 | selection of URL : pointer type | -| stdlib.go:146:11:146:15 | selection of URL : pointer type | stdlib.go:146:11:146:15 | selection of URL : pointer type | -| stdlib.go:146:11:146:15 | selection of URL : pointer type | stdlib.go:149:24:149:35 | call to String | -| 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 | +| stdlib.go:13:13:13:18 | selection of Form : Values | stdlib.go:15:30:15:35 | target | +| stdlib.go:22:13:22:18 | selection of Form : Values | stdlib.go:24:30:24:35 | target | +| stdlib.go:31:13:31:18 | selection of Form : Values | stdlib.go:35:30:35:39 | ...+... | +| stdlib.go:44:13:44:18 | selection of Form : Values | stdlib.go:46:23:46:28 | target | +| stdlib.go:64:13:64:18 | selection of Form : Values | stdlib.go:67:23:67:40 | ...+... | +| stdlib.go:89:13:89:18 | selection of Form : Values | stdlib.go:92:23:92:28 | target | +| stdlib.go:113:24:113:28 | selection of URL : pointer type | stdlib.go:113:24:113:37 | call to String | +| stdlib.go:113:24:113:28 | selection of URL : pointer type | stdlib.go:113:24:113:37 | call to String | +| stdlib.go:134:13:134:18 | selection of Form : Values | stdlib.go:140:23:140:28 | target | +| stdlib.go:147:11:147:15 | selection of URL : pointer type | stdlib.go:147:11:147:15 | selection of URL : pointer type | +| stdlib.go:147:11:147:15 | selection of URL : pointer type | stdlib.go:147:11:147:15 | selection of URL : pointer type | +| stdlib.go:147:11:147:15 | selection of URL : pointer type | stdlib.go:150:24:150:35 | call to String | +| stdlib.go:147:11:147:15 | selection of URL : pointer type | stdlib.go:150:24:150:35 | call to String | +| stdlib.go:161:35:161:39 | selection of URL : pointer type | stdlib.go:161:24:161:52 | ...+... | +| stdlib.go:161:35:161:39 | selection of URL : pointer type | stdlib.go:161:24:161:52 | ...+... | +| stdlib.go:170:13:170:33 | call to FormValue : string | stdlib.go:172:23:172:28 | target | +| stdlib.go:178:36:178:56 | call to FormValue : string | stdlib.go:180:23:180:28 | implicit dereference : URL | +| stdlib.go:178:36:178:56 | call to FormValue : string | stdlib.go:180:23:180:33 | selection of Path | +| stdlib.go:178:36:178:56 | call to FormValue : string | stdlib.go:182:23:182:42 | call to EscapedPath | +| stdlib.go:180:23:180:28 | implicit dereference : URL | stdlib.go:180:23:180:28 | implicit dereference : URL | +| stdlib.go:180:23:180:28 | implicit dereference : URL | stdlib.go:180:23:180:33 | selection of Path | +| stdlib.go:180:23:180:28 | implicit dereference : URL | stdlib.go:182:23:182:42 | call to EscapedPath | 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 | -| stdlib.go:12:13:12:18 | selection of Form : Values | semmle.label | selection of Form : Values | -| stdlib.go:14:30:14:35 | target | semmle.label | target | -| stdlib.go:21:13:21:18 | selection of Form : Values | semmle.label | selection of Form : Values | -| stdlib.go:23:30:23:35 | target | semmle.label | target | -| stdlib.go:30:13:30:18 | selection of Form : Values | semmle.label | selection of Form : Values | -| stdlib.go:34:30:34:39 | ...+... | semmle.label | ...+... | -| stdlib.go:43:13:43:18 | selection of Form : Values | semmle.label | selection of Form : Values | -| stdlib.go:45:23:45:28 | target | semmle.label | target | -| stdlib.go:63:13:63:18 | selection of Form : Values | semmle.label | selection of Form : Values | -| 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 | -| stdlib.go:146:11:146:15 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | -| stdlib.go:146:11:146:15 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | -| stdlib.go:149:24:149:35 | call to String | semmle.label | call to String | -| stdlib.go:149:24:149:35 | call to String | semmle.label | call to String | -| stdlib.go:160:24:160:52 | ...+... | semmle.label | ...+... | -| 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 | +| stdlib.go:13:13:13:18 | selection of Form : Values | semmle.label | selection of Form : Values | +| stdlib.go:15:30:15:35 | target | semmle.label | target | +| stdlib.go:22:13:22:18 | selection of Form : Values | semmle.label | selection of Form : Values | +| stdlib.go:24:30:24:35 | target | semmle.label | target | +| stdlib.go:31:13:31:18 | selection of Form : Values | semmle.label | selection of Form : Values | +| stdlib.go:35:30:35:39 | ...+... | semmle.label | ...+... | +| stdlib.go:44:13:44:18 | selection of Form : Values | semmle.label | selection of Form : Values | +| stdlib.go:46:23:46:28 | target | semmle.label | target | +| stdlib.go:64:13:64:18 | selection of Form : Values | semmle.label | selection of Form : Values | +| stdlib.go:67:23:67:40 | ...+... | semmle.label | ...+... | +| stdlib.go:89:13:89:18 | selection of Form : Values | semmle.label | selection of Form : Values | +| stdlib.go:92:23:92:28 | target | semmle.label | target | +| stdlib.go:113:24:113:28 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| stdlib.go:113:24:113:28 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| stdlib.go:113:24:113:37 | call to String | semmle.label | call to String | +| stdlib.go:113:24:113:37 | call to String | semmle.label | call to String | +| stdlib.go:134:13:134:18 | selection of Form : Values | semmle.label | selection of Form : Values | +| stdlib.go:140:23:140:28 | target | semmle.label | target | +| stdlib.go:147:11:147:15 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| stdlib.go:147:11:147:15 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| stdlib.go:150:24:150:35 | call to String | semmle.label | call to String | +| stdlib.go:150:24:150:35 | call to String | semmle.label | call to String | +| stdlib.go:161:24:161:52 | ...+... | semmle.label | ...+... | +| stdlib.go:161:24:161:52 | ...+... | semmle.label | ...+... | +| stdlib.go:161:35:161:39 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| stdlib.go:161:35:161:39 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| stdlib.go:170:13:170:33 | call to FormValue : string | semmle.label | call to FormValue : string | +| stdlib.go:172:23:172:28 | target | semmle.label | target | +| stdlib.go:178:36:178:56 | call to FormValue : string | semmle.label | call to FormValue : string | +| stdlib.go:180:23:180:28 | implicit dereference : URL | semmle.label | implicit dereference : URL | +| stdlib.go:180:23:180:33 | selection of Path | semmle.label | selection of Path | +| stdlib.go:182:23:182:42 | call to EscapedPath | semmle.label | call to EscapedPath | #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 | -| stdlib.go:23:30:23:35 | target | stdlib.go:21:13:21:18 | selection of Form : Values | stdlib.go:23:30:23:35 | target | Untrusted URL redirection due to $@. | stdlib.go:21:13:21:18 | selection of Form | user-provided value | -| stdlib.go:34:30:34:39 | ...+... | stdlib.go:30:13:30:18 | selection of Form : Values | stdlib.go:34:30:34:39 | ...+... | Untrusted URL redirection due to $@. | stdlib.go:30:13:30:18 | selection of Form | user-provided value | -| 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 | -| 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 | +| stdlib.go:15:30:15:35 | target | stdlib.go:13:13:13:18 | selection of Form : Values | stdlib.go:15:30:15:35 | target | Untrusted URL redirection due to $@. | stdlib.go:13:13:13:18 | selection of Form | user-provided value | +| stdlib.go:24:30:24:35 | target | stdlib.go:22:13:22:18 | selection of Form : Values | stdlib.go:24:30:24:35 | target | Untrusted URL redirection due to $@. | stdlib.go:22:13:22:18 | selection of Form | user-provided value | +| stdlib.go:35:30:35:39 | ...+... | stdlib.go:31:13:31:18 | selection of Form : Values | stdlib.go:35:30:35:39 | ...+... | Untrusted URL redirection due to $@. | stdlib.go:31:13:31:18 | selection of Form | user-provided value | +| stdlib.go:46:23:46:28 | target | stdlib.go:44:13:44:18 | selection of Form : Values | stdlib.go:46:23:46:28 | target | Untrusted URL redirection due to $@. | stdlib.go:44:13:44:18 | selection of Form | user-provided value | +| stdlib.go:67:23:67:40 | ...+... | stdlib.go:64:13:64:18 | selection of Form : Values | stdlib.go:67:23:67:40 | ...+... | Untrusted URL redirection due to $@. | stdlib.go:64:13:64:18 | selection of Form | user-provided value | +| stdlib.go:92:23:92:28 | target | stdlib.go:89:13:89:18 | selection of Form : Values | stdlib.go:92:23:92:28 | target | Untrusted URL redirection due to $@. | stdlib.go:89:13:89:18 | selection of Form | user-provided value | +| stdlib.go:140:23:140:28 | target | stdlib.go:134:13:134:18 | selection of Form : Values | stdlib.go:140:23:140:28 | target | Untrusted URL redirection due to $@. | stdlib.go:134:13:134:18 | selection of Form | user-provided value | +| stdlib.go:172:23:172:28 | target | stdlib.go:170:13:170:33 | call to FormValue : string | stdlib.go:172:23:172:28 | target | Untrusted URL redirection due to $@. | stdlib.go:170:13:170:33 | call to FormValue | user-provided value | +| stdlib.go:180:23:180:33 | selection of Path | stdlib.go:178:36:178:56 | call to FormValue : string | stdlib.go:180:23:180:33 | selection of Path | Untrusted URL redirection due to $@. | stdlib.go:178:36:178:56 | call to FormValue | user-provided value | +| stdlib.go:182:23:182:42 | call to EscapedPath | stdlib.go:178:36:178:56 | call to FormValue : string | stdlib.go:182:23:182:42 | call to EscapedPath | Untrusted URL redirection due to $@. | stdlib.go:178:36:178:56 | 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 edb152684ef..9416a2a5da6 100644 --- a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go +++ b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go @@ -2,6 +2,7 @@ package main import ( "net/http" + "net/url" "regexp" ) @@ -171,5 +172,15 @@ func serveStdlib() { http.Redirect(w, r, target, 301) }) + http.HandleFunc("/ex10", func(w http.ResponseWriter, r *http.Request) { + r.ParseForm() + + target, _ := url.ParseRequestURI(r.FormValue("target")) + // BAD: Path could start with `//` + http.Redirect(w, r, target.Path, 301) + // BAD: EscapedPath() does not help with that + http.Redirect(w, r, target.EscapedPath(), 301) + }) + http.ListenAndServe(":80", nil) } From 3bf934d64b46416220a0e68c3ad026b477a3a5b0 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 25 Jun 2020 22:23:49 +0100 Subject: [PATCH 2/3] Add change note. --- change-notes/2020-06-24-open-redirect.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change-notes/2020-06-24-open-redirect.md diff --git a/change-notes/2020-06-24-open-redirect.md b/change-notes/2020-06-24-open-redirect.md new file mode 100644 index 00000000000..3916ca733e8 --- /dev/null +++ b/change-notes/2020-06-24-open-redirect.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The query "Open URL redirect" (`go/unvalidated-url-redirection`) now recognizes more problematic fields of `URL` objects, allowing it to flag more results. \ No newline at end of file From 9904b9e9269eb81c31ed33a8637deb90a5d85b24 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 26 Jun 2020 07:50:08 +0100 Subject: [PATCH 3/3] Allow flow through more `URL` fields. --- ql/src/semmle/go/security/OpenUrlRedirect.qll | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ql/src/semmle/go/security/OpenUrlRedirect.qll b/ql/src/semmle/go/security/OpenUrlRedirect.qll index 67477b764f3..4fcc909af52 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirect.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirect.qll @@ -38,9 +38,10 @@ module OpenUrlRedirect { w.writesField(v.getAUse(), f, pred) and succ = v.getAUse() ) or - // propagate out of most URL fields, but not `Scheme` and `User` - exists(Field f | - f.hasQualifiedName("net/url", "URL", ["Fragment", "Host", "Path", "RawPath", "RawQuery"]) + // propagate out of most URL fields, but not `ForceQuery` and `Scheme` + exists(Field f, string fn | + f.hasQualifiedName("net/url", "URL", fn) and + not fn in ["ForceQuery", "Scheme"] | succ.(Read).readsField(pred, f) )