diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index a2428712ea6..8edb61b33b8 100644 --- a/change-notes/1.24/analysis-go.md +++ b/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 diff --git a/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.go b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.go new file mode 100644 index 00000000000..364ec89d237 --- /dev/null +++ b/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 +} diff --git a/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp new file mode 100644 index 00000000000..0c04811c18d --- /dev/null +++ b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp @@ -0,0 +1,42 @@ + + + +

+URLs with the special scheme javascript 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 +javascript scheme. +

+

+However, the data and vbscript schemes can be used to represent +executable code in a very similar way, so any validation logic that checks against +javascript, but not against data and vbscript, is likely +to be insufficient. +

+
+ +

+Add checks covering both data: and vbscript:. +

+
+ +

+The following function validates a (presumably untrusted) URL urlstr. If its scheme is +javascript, the harmless placeholder URL about:blank is returned to +prevent code injection; otherwise urlstr itself is returned. +

+ +

+While this check provides partial projection, it should be extended to cover data +and vbscript as well: +

+ +
+ +
  • WHATWG: URL schemes.
  • +
    +
    diff --git a/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql new file mode 100644 index 00000000000..e1227b76d6b --- /dev/null +++ b/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 ") + "." diff --git a/ql/src/Security/CWE-020/IncompleteUrlSchemeCheckGood.go b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheckGood.go new file mode 100644 index 00000000000..1d3fb46a65e --- /dev/null +++ b/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 +} diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.expected b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.expected new file mode 100644 index 00000000000..d1bbee8dd70 --- /dev/null +++ b/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. | diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.go b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.go new file mode 100644 index 00000000000..364ec89d237 --- /dev/null +++ b/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 +} diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.qlref b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.qlref new file mode 100644 index 00000000000..b27571781b3 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.qlref @@ -0,0 +1 @@ +Security/CWE-020/IncompleteUrlSchemeCheck.ql diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheckGood.go b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheckGood.go new file mode 100644 index 00000000000..1d3fb46a65e --- /dev/null +++ b/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 +} diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/main.go b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/main.go new file mode 100644 index 00000000000..ebe18f142f8 --- /dev/null +++ b/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 + } +}