From ab583b7994e72299206d9917e2758d872b185415 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 8 Nov 2019 11:22:45 +0000 Subject: [PATCH] JavaScript: Add query `IncompleteUrlSchemeCheck.ql`. --- change-notes/1.23/analysis-javascript.md | 1 + javascript/config/suites/javascript/security | 1 + .../CWE-020/IncompleteUrlSchemeCheck.qhelp | 42 ++++++++++++++ .../CWE-020/IncompleteUrlSchemeCheck.ql | 57 +++++++++++++++++++ .../examples/IncompleteUrlSchemeCheck.js | 6 ++ .../examples/IncompleteUrlSchemeCheckGood.js | 6 ++ .../CWE-020/IncompleteUrlSchemeCheck.expected | 1 + .../CWE-020/IncompleteUrlSchemeCheck.js | 6 ++ .../CWE-020/IncompleteUrlSchemeCheck.qlref | 1 + .../CWE-020/IncompleteUrlSchemeCheckGood.js | 6 ++ 10 files changed, 127 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp create mode 100644 javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql create mode 100644 javascript/ql/src/Security/CWE-020/examples/IncompleteUrlSchemeCheck.js create mode 100644 javascript/ql/src/Security/CWE-020/examples/IncompleteUrlSchemeCheckGood.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.expected create mode 100644 javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.qlref create mode 100644 javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheckGood.js diff --git a/change-notes/1.23/analysis-javascript.md b/change-notes/1.23/analysis-javascript.md index f01d384f104..5bc9ec053f2 100644 --- a/change-notes/1.23/analysis-javascript.md +++ b/change-notes/1.23/analysis-javascript.md @@ -28,6 +28,7 @@ | Useless regular expression character escape (`js/useless-regexp-character-escape`) | correctness, security, external/cwe/cwe-20 | Highlights regular expression strings with useless character escapes, indicating a possible violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default. | | Unreachable method overloads (`js/unreachable-method-overloads`) | correctness, typescript | Highlights method overloads that are impossible to use from client code. Results are shown on LGTM by default. | | Ignoring result from pure array method (`js/ignore-array-result`) | maintainability, correctness | Highlights calls to array methods without side effects where the return value is ignored. Results are shown on LGTM by default. | +| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | security, correctness, external/cwe/cwe-020 | Highlights checks for `javascript:` URLs that do not take `data:` or `vbscript:` URLs into account. | ## Changes to existing queries diff --git a/javascript/config/suites/javascript/security b/javascript/config/suites/javascript/security index d8eb5e805d5..d3501346458 100644 --- a/javascript/config/suites/javascript/security +++ b/javascript/config/suites/javascript/security @@ -1,6 +1,7 @@ + semmlecode-javascript-queries/DOM/TargetBlank.ql: /Security/CWE/CWE-200 + semmlecode-javascript-queries/Electron/EnablingNodeIntegration.ql: /Security/CWE/CWE-094 + semmlecode-javascript-queries/Security/CWE-020/IncompleteHostnameRegExp.ql: /Security/CWE/CWE-020 ++ semmlecode-javascript-queries/Security/CWE-020/IncompleteUrlSchemeCheck.ql: /Security/CWE/CWE-020 + semmlecode-javascript-queries/Security/CWE-020/IncompleteUrlSubstringSanitization.ql: /Security/CWE/CWE-020 + semmlecode-javascript-queries/Security/CWE-020/IncorrectSuffixCheck.ql: /Security/CWE/CWE-020 + semmlecode-javascript-queries/Security/CWE-020/MissingRegExpAnchor.ql: /Security/CWE/CWE-020 diff --git a/javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp b/javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp new file mode 100644 index 00000000000..2ae2a9422c3 --- /dev/null +++ b/javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp @@ -0,0 +1,42 @@ + + + +

+URLs starting with 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 url. If it starts with +javascript: (case-insensitive and potentially preceded by whitespace), the harmless +placeholder URL about:blank is returned to prevent code injection; otherwise +url 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/javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql b/javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql new file mode 100644 index 00000000000..b77eb15a5a4 --- /dev/null +++ b/javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql @@ -0,0 +1,57 @@ +/** + * @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 js/incomplete-url-scheme-check + * @tags security + * correctness + * external/cwe/cwe-020 + */ + +import javascript +import semmle.javascript.dataflow.internal.AccessPaths + +/** 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 `nd` against the given `scheme`. */ +DataFlow::Node schemeCheck( + DataFlow::Node nd, DangerousScheme scheme +) { + // check of the form `nd.startsWith(scheme)` + exists(StringOps::StartsWith sw | sw = result | + sw.getBaseString() = nd and + sw.getSubstring().mayHaveStringValue(scheme) + ) + or + // propagate through trimming, case conversion, and regexp replace + exists(DataFlow::MethodCallNode stringop | + stringop.getMethodName().matches("trim%") or + stringop.getMethodName().matches("to%Case") or + stringop.getMethodName() = "replace" + | + result = schemeCheck(stringop, scheme) and + nd = stringop.getReceiver() + ) + or + // propagate througb local data flow + result = schemeCheck(nd.getASuccessor(), scheme) +} + +/** Gets a data-flow node that checks an instance of `ap` against the given `scheme`. */ +DataFlow::Node schemeCheckOn(AccessPath ap, DangerousScheme scheme) { + result = schemeCheck(ap.getAnInstance().flow(), scheme) +} + +from AccessPath ap, int n +where + n = strictcount(DangerousScheme s) and + strictcount(DangerousScheme s | exists(schemeCheckOn(ap, s))) < n +select schemeCheckOn(ap, "javascript:"), + "This check does not consider " + + strictconcat(DangerousScheme s | not exists(schemeCheckOn(ap, s)) | s, " and ") + "." diff --git a/javascript/ql/src/Security/CWE-020/examples/IncompleteUrlSchemeCheck.js b/javascript/ql/src/Security/CWE-020/examples/IncompleteUrlSchemeCheck.js new file mode 100644 index 00000000000..270cff2d821 --- /dev/null +++ b/javascript/ql/src/Security/CWE-020/examples/IncompleteUrlSchemeCheck.js @@ -0,0 +1,6 @@ +function sanitizeUrl(url) { + let u = decodeURI(url).trim().toLowerCase(); + if (u.startsWith("javascript:")) + return "about:blank"; + return url; +} diff --git a/javascript/ql/src/Security/CWE-020/examples/IncompleteUrlSchemeCheckGood.js b/javascript/ql/src/Security/CWE-020/examples/IncompleteUrlSchemeCheckGood.js new file mode 100644 index 00000000000..18376b24454 --- /dev/null +++ b/javascript/ql/src/Security/CWE-020/examples/IncompleteUrlSchemeCheckGood.js @@ -0,0 +1,6 @@ +function sanitizeUrl(url) { + let u = decodeURI(url).trim().toLowerCase(); + if (u.startsWith("javascript:") || u.startsWith("data:") || u.startsWith("vbscript:")) + return "about:blank"; + return url; +} diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.expected b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.expected new file mode 100644 index 00000000000..de7800f58fd --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.expected @@ -0,0 +1 @@ +| IncompleteUrlSchemeCheck.js:3:9:3:35 | u.start ... ript:") | This check does not consider data: and vbscript:. | diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.js b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.js new file mode 100644 index 00000000000..270cff2d821 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.js @@ -0,0 +1,6 @@ +function sanitizeUrl(url) { + let u = decodeURI(url).trim().toLowerCase(); + if (u.startsWith("javascript:")) + return "about:blank"; + return url; +} diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.qlref b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.qlref new file mode 100644 index 00000000000..99a744188b5 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.qlref @@ -0,0 +1 @@ +Security/CWE-020/IncompleteUrlSchemeCheck.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheckGood.js b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheckGood.js new file mode 100644 index 00000000000..18376b24454 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheckGood.js @@ -0,0 +1,6 @@ +function sanitizeUrl(url) { + let u = decodeURI(url).trim().toLowerCase(); + if (u.startsWith("javascript:") || u.startsWith("data:") || u.startsWith("vbscript:")) + return "about:blank"; + return url; +}