Add BadRedirectCheck query

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
 

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 "/"
+}

ql/src/Security/CWE-601/BadRedirectCheck.qhelp

@@ -0,0 +1,40 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+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 <code>//</code> or <code>/\</code> as absolute URLs. For example, a redirect to
+<code>//lgtm.com</code> will redirect to <code>https://lgtm.com</code>. Thus, redirect checks must
+also check the second character of redirect URLs.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Also disallow the patterns <code>//*</code> and <code>/\*</code> when checking redirect URLs.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following function validates a (presumably untrusted) redirect URL <code>redir</code>. If it
+does not begin with <code>/</code>, the harmless placeholder redirect URL, <code>/</code> is
+returned to prevent an open redirect; otherwise <code>redir</code> itself is returned.
+</p>
+<sample src="BadRedirectCheck.go"/>
+<p>
+While this check provides partial protection, it should be extended to cover <code>//</code> and
+<code>/\</code> as well:
+</p>
+<sample src="BadRedirectCheckGood.go"/>
+</example>
+
+<references>
+<li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html#validating-urls">
+  XSS Unvalidated Redirects and Forwards Cheat Sheet</a>.</li>
+</references>
+
+</qhelp>

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()

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 "/"
+}

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 |

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 "/"
+}

ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.qlref

@@ -0,0 +1 @@
+Security/CWE-601/BadRedirectCheck.ql

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 "/"
+}

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 = "/"
+	}
+}

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
+	}
+}