From aa28724f7c95cf3cbad237f46fe0c41ac759c2b8 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Wed, 8 Jan 2020 18:15:23 -0800 Subject: [PATCH] Add `BadRedirectCheck` query --- change-notes/1.24/analysis-go.md | 1 + ql/src/Security/CWE-601/BadRedirectCheck.go | 8 ++ .../Security/CWE-601/BadRedirectCheck.qhelp | 40 +++++++++ ql/src/Security/CWE-601/BadRedirectCheck.ql | 84 +++++++++++++++++++ .../Security/CWE-601/BadRedirectCheckGood.go | 8 ++ .../BadRedirectCheck.expected | 5 ++ .../BadRedirectCheck/BadRedirectCheck.go | 8 ++ .../BadRedirectCheck/BadRedirectCheck.qlref | 1 + .../BadRedirectCheck/BadRedirectCheckGood.go | 8 ++ .../Security/CWE-601/BadRedirectCheck/cves.go | 32 +++++++ .../Security/CWE-601/BadRedirectCheck/main.go | 23 +++++ 11 files changed, 218 insertions(+) create mode 100644 ql/src/Security/CWE-601/BadRedirectCheck.go create mode 100644 ql/src/Security/CWE-601/BadRedirectCheck.qhelp create mode 100644 ql/src/Security/CWE-601/BadRedirectCheck.ql create mode 100644 ql/src/Security/CWE-601/BadRedirectCheckGood.go create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.go create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.qlref create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheckGood.go create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/cves.go create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/main.go diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index 94aca0bf065..04323ca0467 100644 --- a/change-notes/1.24/analysis-go.md +++ b/change-notes/1.24/analysis-go.md @@ -13,6 +13,7 @@ | Constant length comparison (`go/constant-length-comparison`) | correctness | Highlights code that checks the length of an array or slice against a constant before indexing it using a variable, suggesting a logic error. Results are shown on LGTM by default. | | Impossible interface nil check (`go/impossible-interface-nil-check`) | correctness | Highlights code that compares an interface value that cannot be `nil` to `nil`, suggesting a logic error. Results are shown on LGTM by default. | | Incomplete URL scheme check (`go/incomplete-url-scheme-check`) | correctness, security, external/cwe/cwe-020 | Highlights checks for `javascript` URLs that do not take `data` or `vbscript` URLs into account. Results are shown on LGTM by default. | +| Bad check of redirect URL (`go/bad-redirect-check`) | correctness, security, external/cwe/cwe-601 | Highlights checks for redirect URLs ensuring they start with `/` but don't check for `//` or `/\`. Results are shown on LGTM by default. | ## Changes to existing queries diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.go b/ql/src/Security/CWE-601/BadRedirectCheck.go new file mode 100644 index 00000000000..279e59c9cfb --- /dev/null +++ b/ql/src/Security/CWE-601/BadRedirectCheck.go @@ -0,0 +1,8 @@ +package main + +func sanitizeUrl(redir string) string { + if len(redir) > 0 && redir[0] == '/' { + return redir + } + return "/" +} diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.qhelp b/ql/src/Security/CWE-601/BadRedirectCheck.qhelp new file mode 100644 index 00000000000..45eb906ca73 --- /dev/null +++ b/ql/src/Security/CWE-601/BadRedirectCheck.qhelp @@ -0,0 +1,40 @@ + + + + +

+Redirect URLs should be checked in order to ensure that user input cannot cause a site to redirect +to arbitrary domains. This is often done with a check that the redirect URL begins with a slash, +which most of the time is an absolute redirect on the same host. However, browsers interpret URLs +beginning with // or /\ as absolute URLs. For example, a redirect to +//lgtm.com will redirect to https://lgtm.com. Thus, redirect checks must +also check the second character of redirect URLs. +

+ + + +

+Also disallow the patterns //* and /\* when checking redirect URLs. +

+ + + +

+The following function validates a (presumably untrusted) redirect URL redir. If it +does not begin with /, the harmless placeholder redirect URL, / is +returned to prevent an open redirect; otherwise redir itself is returned. +

+ +

+While this check provides partial protection, it should be extended to cover // and +/\ as well: +

+ + + + +
  • OWASP: + XSS Unvalidated Redirects and Forwards Cheat Sheet.
    • +     + +     diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.ql b/ql/src/Security/CWE-601/BadRedirectCheck.ql new file mode 100644 index 00000000000..72738fd554c --- /dev/null +++ b/ql/src/Security/CWE-601/BadRedirectCheck.ql @@ -0,0 +1,84 @@ +/** + * @name Bad redirect check + * @description A redirect check that checks for a leading slash but not two + * leading slashes or a leading slash then backslash is + * incomplete. + * @kind problem + * @problem.severity warning + * @id go/bad-redirect-check + * @tags security + * external/cwe/cwe-601 + * @precision high + */ + +import go + +predicate checksForLeadingSlash(Expr e, ValueEntity v) { + exists(LogicalExpr le | le = e | checksForLeadingSlash(le.getAnOperand(), v)) + or + exists(StringOps::HasPrefix hp | + hp.getBaseString().(Read).reads(v) and + // ASCII value for '/' + (hp.getSubstring().getStringValue() = "/" or hp.getSubstring().getIntValue() = 47) + ) +} + +predicate checksForSecondSlash(Expr e, ValueEntity v) { + exists(LogicalExpr le | le = e | checksForSecondSlash(le.getAnOperand(), v)) + or + exists(StringOps::HasPrefix hp | + hp.getBaseString().(Read).reads(v) and + hp.getSubstring().getStringValue() = "//" + ) + or + exists(EqualityTestExpr eq, Expr slash, IndexExpr ie | e = eq | + slash.getIntValue() = 47 and // ASCII value for '/' + ie.getBase() = v.getAUse() and + ie.getIndex().getIntValue() = 1 and + eq.hasOperands(ie, slash) + ) +} + +predicate checksForSecondBackslash(Expr e, ValueEntity v) { + exists(LogicalExpr le | le = e | checksForSecondBackslash(le.getAnOperand(), v)) + or + exists(StringOps::HasPrefix hp | + hp.getBaseString().(Read).reads(v) and + hp.getSubstring().getStringValue() = "/\\" + ) + or + exists(EqualityTestExpr eq, Expr slash, IndexExpr ie | e = eq | + slash.getIntValue() = 92 and // ASCII value for '\' + ie.getBase() = v.getAUse() and + ie.getIndex().getIntValue() = 1 and + eq.hasOperands(ie, slash) + ) +} + +predicate isBadRedirectCheck(Expr e, ValueEntity v) { + checksForLeadingSlash(e, v) and + not (checksForSecondSlash(e, v) and checksForSecondBackslash(e, v)) +} + +predicate isCond(Expr e) { + e = any(ForStmt fs).getCond() + or + e = any(IfStmt is).getCond() + or + e = any(ExpressionSwitchStmt ess | not exists(ess.getExpr())).getACase().getAnExpr() +} + +from Expr e, ValueEntity v +where + isBadRedirectCheck(e, v) and + ( + // this expression is a condition + isCond(e) + or + // or is returned from a function + DataFlow::exprNode(e).getASuccessor*() instanceof DataFlow::ResultNode + ) and + v.getName().regexpMatch("(?i).*url.*|.*redir.*") +select e, + "This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position.", + v, v.getName() diff --git a/ql/src/Security/CWE-601/BadRedirectCheckGood.go b/ql/src/Security/CWE-601/BadRedirectCheckGood.go new file mode 100644 index 00000000000..89af5110a29 --- /dev/null +++ b/ql/src/Security/CWE-601/BadRedirectCheckGood.go @@ -0,0 +1,8 @@ +package main + +func sanitizeUrl1(redir string) string { + if len(redir) > 1 && redir[0] == '/' && redir[1] != '/' && redir[1] != '\\' { + return redir + } + return "/" +} diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected new file mode 100644 index 00000000000..7ba64bf8346 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected @@ -0,0 +1,5 @@ +| BadRedirectCheck.go:4:5:4:37 | ...&&... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redir | redir | +| cves.go:11:9:11:55 | ...&&... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | url | url | +| cves.go:22:5:22:37 | !... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redirect | redirect | +| cves.go:29:5:29:74 | ...\|\|... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redirect | redirect | +| main.go:8:7:8:76 | ...&&... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redirect | redirect | diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.go b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.go new file mode 100644 index 00000000000..279e59c9cfb --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.go @@ -0,0 +1,8 @@ +package main + +func sanitizeUrl(redir string) string { + if len(redir) > 0 && redir[0] == '/' { + return redir + } + return "/" +} diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.qlref b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.qlref new file mode 100644 index 00000000000..fa21b83def6 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.qlref @@ -0,0 +1 @@ +Security/CWE-601/BadRedirectCheck.ql diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheckGood.go b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheckGood.go new file mode 100644 index 00000000000..89af5110a29 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheckGood.go @@ -0,0 +1,8 @@ +package main + +func sanitizeUrl1(redir string) string { + if len(redir) > 1 && redir[0] == '/' && redir[1] != '/' && redir[1] != '\\' { + return redir + } + return "/" +} diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/cves.go b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/cves.go new file mode 100644 index 00000000000..cd91df4d4a7 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/cves.go @@ -0,0 +1,32 @@ +package main + +import ( + "net/http" + "strings" +) + +// CVE-2018-15178 +// Code from github.com/gogs/gogs +func isValidRedirect(url string) bool { + return len(url) >= 2 && url[0] == '/' && url[1] != '/' // NOT OK +} + +func isValidRedirect1(url string) bool { + return len(url) >= 2 && url[0] == '/' && url[1] != '/' && url[1] != '\\' // OK +} + +// CVE-2017-1000070 (both vulnerable!) +// Code from github.com/bitly/oauth2_proxy +func OAuthCallback(rw http.ResponseWriter, req *http.Request) { + redirect := req.Form.Get("state") + if !strings.HasPrefix(redirect, "/") { // NOT OK + redirect = "/" + } +} + +func OAuthCallback1(rw http.ResponseWriter, req *http.Request) { + redirect := req.Form.Get("state") + if !strings.HasPrefix(redirect, "/") || strings.HasPrefix(redirect, "//") { // NOT OK + redirect = "/" + } +} diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/main.go b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/main.go new file mode 100644 index 00000000000..97b7b9c5f89 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/main.go @@ -0,0 +1,23 @@ +package main + +import "strings" + +func isValidRedir(redirect string) bool { + switch { + // Not OK: does not check for '/\' + case strings.HasPrefix(redirect, "/") && !strings.HasPrefix(redirect, "//"): + return true + default: + return false + } +} + +func isValidRedir1(redirect string) bool { + switch { + // OK + case strings.HasPrefix(redirect, "/") && !strings.HasPrefix(redirect, "//") && !strings.HasPrefix(redirect, "/\\"): + return true + default: + return false + } +}