Add IncompleteUrlSchemeCheck query.

change-notes/1.24/analysis-go.md

@@ -10,6 +10,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. |
 
 ## Changes to existing queries
 

ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.go

@@ -0,0 +1,11 @@
+package main
+
+import "net/url"
+
+func sanitizeUrl(urlstr string) string {
+    u, err := url.Parse(urlstr)
+    if err != nil || u.Scheme == "javascript" {
+        return "about:blank"
+    }
+    return urlstr
+}

ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp

@@ -0,0 +1,42 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+URLs with the special scheme <code>javascript</code> can be used to encode JavaScript code to be
+executed when the URL is visited. While this is a powerful mechanism for creating feature-rich and
+responsive web applications, it is also a potential security risk: if the URL comes from an
+untrusted source, it might contain harmful JavaScript code. For this reason, many frameworks and
+libraries first check the URL scheme of any untrusted URL, and reject URLs with the
+<code>javascript</code> scheme.
+</p>
+<p>
+However, the <code>data</code> and <code>vbscript</code> schemes can be used to represent
+executable code in a very similar way, so any validation logic that checks against
+<code>javascript</code>, but not against <code>data</code> and <code>vbscript</code>, is likely
+to be insufficient.
+</p>
+</overview>
+<recommendation>
+<p>
+Add checks covering both <code>data:</code> and <code>vbscript:</code>.
+</p>
+</recommendation>
+<example>
+<p>
+The following function validates a (presumably untrusted) URL <code>urlstr</code>. If its scheme is
+<code>javascript</code>, the harmless placeholder URL <code>about:blank</code> is returned to
+prevent code injection; otherwise <code>urlstr</code> itself is returned.
+</p>
+<sample src="IncompleteUrlSchemeCheck.go"/>
+<p>
+While this check provides partial projection, it should be extended to cover <code>data</code>
+and <code>vbscript</code> as well:
+</p>
+<sample src="IncompleteUrlSchemeCheckGood.go"/>
+</example>
+<references>
+<li>WHATWG: <a href="https://wiki.whatwg.org/wiki/URL_schemes">URL schemes</a>.</li>
+</references>
+</qhelp>

ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql

@@ -0,0 +1,60 @@
+/**
+ * @name Incomplete URL scheme check
+ * @description Checking for the "javascript:" URL scheme without also checking for "vbscript:"
+ *              and "data:" suggests a logic error or even a security vulnerability.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id go/incomplete-url-scheme-check
+ * @tags security
+ *       correctness
+ *       external/cwe/cwe-020
+ */
+
+import go
+
+/** A URL scheme that can be used to represent executable code. */
+class DangerousScheme extends string {
+  DangerousScheme() { this = "data" or this = "javascript" or this = "vbscript" }
+}
+
+/** Gets a data-flow node that checks an instance of `g` against the given `scheme`. */
+DataFlow::Node schemeCheck(GVN g, DangerousScheme scheme) {
+  // check of the form `nd.Scheme == scheme`
+  exists(NamedType url, DataFlow::FieldReadNode fr, DataFlow::Node s |
+    url.hasQualifiedName("net/url", "URL") and
+    fr.readsField(g.getANode(), url.getField("Scheme")) and
+    s.getStringValue() = scheme and
+    result.(DataFlow::EqualityTestNode).eq(_, fr, s)
+  )
+  or
+  // check of the form `strings.HasPrefix(nd, "scheme:")`
+  exists(StringOps::HasPrefix hasPrefix | result = hasPrefix |
+    hasPrefix.getBaseString() = g.getANode() and
+    hasPrefix.getSubstring().getStringValue() = scheme + ":"
+  )
+  or
+  // propagate through various string transformations
+  exists(string stringop, DataFlow::CallNode c |
+    stringop = "Map" or
+    stringop.matches("Replace%") or
+    stringop.matches("To%") or
+    stringop.matches("Trim%")
+  |
+    c.getTarget().hasQualifiedName("strings", stringop) and
+    c.getAnArgument() = g.getANode() and
+    result = schemeCheck(globalValueNumber(c), scheme)
+  )
+}
+
+/**
+ * Gets a scheme that `g`, which is checked against at least one scheme, is not checked against.
+ */
+DangerousScheme getAMissingScheme(GVN g) {
+  exists(schemeCheck(g, _)) and
+  not exists(schemeCheck(g, result))
+}
+
+from GVN g
+select schemeCheck(g, "javascript"),
+  "This check does not consider " + strictconcat(getAMissingScheme(g), " and ") + "."

ql/src/Security/CWE-020/IncompleteUrlSchemeCheckGood.go

@@ -0,0 +1,11 @@
+package main
+
+import "net/url"
+
+func sanitizeUrlGod(urlstr string) string {
+    u, err := url.Parse(urlstr)
+    if err != nil || u.Scheme == "javascript" || u.Scheme == "data" || u.Scheme == "vbscript" {
+        return "about:blank"
+    }
+    return urlstr
+}

ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.expected

@@ -0,0 +1,2 @@
+| IncompleteUrlSchemeCheck.go:7:22:7:45 | ...==... | This check does not consider data and vbscript. |
+| main.go:17:5:17:44 | call to HasPrefix | This check does not consider vbscript. |

ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.go

@@ -0,0 +1,11 @@
+package main
+
+import "net/url"
+
+func sanitizeUrl(urlstr string) string {
+    u, err := url.Parse(urlstr)
+    if err != nil || u.Scheme == "javascript" {
+        return "about:blank"
+    }
+    return urlstr
+}

ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.qlref

@@ -0,0 +1 @@
+Security/CWE-020/IncompleteUrlSchemeCheck.ql

ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheckGood.go

@@ -0,0 +1,11 @@
+package main
+
+import "net/url"
+
+func sanitizeUrlGod(urlstr string) string {
+    u, err := url.Parse(urlstr)
+    if err != nil || u.Scheme == "javascript" || u.Scheme == "data" || u.Scheme == "vbscript" {
+        return "about:blank"
+    }
+    return urlstr
+}

ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/main.go

@@ -0,0 +1,20 @@
+package main
+
+import (
+	"net/url"
+	"strings"
+)
+
+func test(urlstr string) {
+	u, _ := url.Parse(urlstr)
+	sch := u.Scheme
+	if sch == "javascript" || u.Scheme == "data" || sch == "vbscript" { // OK
+		return
+	}
+
+	urlstr = strings.NewReplacer("\n", "", "\r", "", "\t", "", "\u0000", "").Replace(urlstr)
+	urlstr = strings.ToLower(urlstr)
+	if strings.HasPrefix(urlstr, "javascript:") || strings.HasPrefix(urlstr, "data:") { // NOT OK
+		return
+	}
+}