Skip to content

Commit 2ea7d14

Browse files
esbenaesbena
authored
Merge pull request #2310 from max-schaefer/js/insufficient-url-scheme-check
JavaScript: Add query `IncompleteUrlSchemeCheck`
2 parents 0638907 + 3b1e6c3 commit 2ea7d14

10 files changed

Lines changed: 131 additions & 4 deletions

File tree

‎change-notes/1.23/analysis-javascript.md‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,15 @@
2121

2222
| **Query** | **Tags** | **Purpose** |
2323
|---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
24-
| Unused index variable (`js/unused-index-variable`) | correctness | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. |
24+
| 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. |
25+
| 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. Results are shown on LGTM by default. |
2526
| Loop bound injection (`js/loop-bound-injection`) | security, external/cwe/cwe-834 | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are shown on LGTM by default. |
26-
| Suspicious method name (`js/suspicious-method-name-declaration`) | correctness, typescript, methods | Highlights suspiciously named methods where the developer likely meant to write a constructor or function. Results are shown on LGTM by default. |
2727
| Shell command built from environment values (`js/shell-command-injection-from-environment`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights shell commands that may change behavior inadvertently depending on the execution environment, indicating a possible violation of [CWE-78](https://cwe.mitre.org/data/definitions/78.html). Results are shown on LGTM by default.|
28+
| Suspicious method name (`js/suspicious-method-name-declaration`) | correctness, typescript, methods | Highlights suspiciously named methods where the developer likely meant to write a constructor or function. Results are shown on LGTM by default. |
29+
| 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. |
30+
| Unused index variable (`js/unused-index-variable`) | correctness | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. |
2831
| Use of returnless function (`js/use-of-returnless-function`) | maintainability, correctness | Highlights calls where the return value is used, but the callee never returns a value. Results are shown on LGTM by default. |
2932
| 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. |
30-
| 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. |
31-
| 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. |
3233

3334
## Changes to existing queries
3435

‎javascript/config/suites/javascript/security‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
+ semmlecode-javascript-queries/DOM/TargetBlank.ql: /Security/CWE/CWE-200
22
+ semmlecode-javascript-queries/Electron/EnablingNodeIntegration.ql: /Security/CWE/CWE-094
33
+ semmlecode-javascript-queries/Security/CWE-020/IncompleteHostnameRegExp.ql: /Security/CWE/CWE-020
4+
+ semmlecode-javascript-queries/Security/CWE-020/IncompleteUrlSchemeCheck.ql: /Security/CWE/CWE-020
45
+ semmlecode-javascript-queries/Security/CWE-020/IncompleteUrlSubstringSanitization.ql: /Security/CWE/CWE-020
56
+ semmlecode-javascript-queries/Security/CWE-020/IncorrectSuffixCheck.ql: /Security/CWE/CWE-020
67
+ semmlecode-javascript-queries/Security/CWE-020/MissingRegExpAnchor.ql: /Security/CWE/CWE-020

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
URLs starting with <code>javascript:</code> can be used to encode JavaScript code to be executed
8+
when the URL is visited. While this is a powerful mechanism for creating feature-rich and responsive
9+
web applications, it is also a potential security risk: if the URL comes from an untrusted source,
10+
it might contain harmful JavaScript code. For this reason, many frameworks and libraries first check
11+
the URL scheme of any untrusted URL, and reject URLs with the <code>javascript:</code> scheme.
12+
</p>
13+
<p>
14+
However, the <code>data:</code> and <code>vbscript:</code> schemes can be used to represent
15+
executable code in a very similar way, so any validation logic that checks against
16+
<code>javascript:</code>, but not against <code>data:</code> and <code>vbscript:</code>, is likely to
17+
be insufficient.
18+
</p>
19+
</overview>
20+
<recommendation>
21+
<p>
22+
Add checks covering both <code>data:</code> and <code>vbscript:</code>.
23+
</p>
24+
</recommendation>
25+
<example>
26+
<p>
27+
The following function validates a (presumably untrusted) URL <code>url</code>. If it starts with
28+
<code>javascript:</code> (case-insensitive and potentially preceded by whitespace), the harmless
29+
placeholder URL <code>about:blank</code> is returned to prevent code injection; otherwise
30+
<code>url</code> itself is returned.
31+
</p>
32+
<sample src="examples/IncompleteUrlSchemeCheck.js"/>
33+
<p>
34+
While this check provides partial projection, it should be extended to cover <code>data:</code>
35+
and <code>vbscript:</code> as well:
36+
</p>
37+
<sample src="examples/IncompleteUrlSchemeCheckGood.js"/>
38+
</example>
39+
<references>
40+
<li>WHATWG: <a href="https://wiki.whatwg.org/wiki/URL_schemes">URL schemes</a>.</li>
41+
</references>
42+
</qhelp>

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

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
/**
2+
* @name Incomplete URL scheme check
3+
* @description Checking for the "javascript:" URL scheme without also checking for "vbscript:"
4+
* and "data:" suggests a logic error or even a security vulnerability.
5+
* @kind problem
6+
* @problem.severity warning
7+
* @precision high
8+
* @id js/incomplete-url-scheme-check
9+
* @tags security
10+
* correctness
11+
* external/cwe/cwe-020
12+
*/
13+
14+
import javascript
15+
import semmle.javascript.dataflow.internal.AccessPaths
16+
17+
/** A URL scheme that can be used to represent executable code. */
18+
class DangerousScheme extends string {
19+
DangerousScheme() { this = "data:" or this = "javascript:" or this = "vbscript:" }
20+
}
21+
22+
/** Gets a data-flow node that checks `nd` against the given `scheme`. */
23+
DataFlow::Node schemeCheck(
24+
DataFlow::Node nd, DangerousScheme scheme
25+
) {
26+
// check of the form `nd.startsWith(scheme)`
27+
exists(StringOps::StartsWith sw | sw = result |
28+
sw.getBaseString() = nd and
29+
sw.getSubstring().mayHaveStringValue(scheme)
30+
)
31+
or
32+
// propagate through trimming, case conversion, and regexp replace
33+
exists(DataFlow::MethodCallNode stringop |
34+
stringop.getMethodName().matches("trim%") or
35+
stringop.getMethodName().matches("to%Case") or
36+
stringop.getMethodName() = "replace"
37+
|
38+
result = schemeCheck(stringop, scheme) and
39+
nd = stringop.getReceiver()
40+
)
41+
or
42+
// propagate through local data flow
43+
result = schemeCheck(nd.getASuccessor(), scheme)
44+
}
45+
46+
/** Gets a data-flow node that checks an instance of `ap` against the given `scheme`. */
47+
DataFlow::Node schemeCheckOn(AccessPath ap, DangerousScheme scheme) {
48+
result = schemeCheck(ap.getAnInstance().flow(), scheme)
49+
}
50+
51+
from AccessPath ap, int n
52+
where
53+
n = strictcount(DangerousScheme s) and
54+
strictcount(DangerousScheme s | exists(schemeCheckOn(ap, s))) < n
55+
select schemeCheckOn(ap, "javascript:"),
56+
"This check does not consider " +
57+
strictconcat(DangerousScheme s | not exists(schemeCheckOn(ap, s)) | s, " and ") + "."

‎javascript/ql/src/Security/CWE-020/examples/IncompleteUrlSchemeCheck.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
function sanitizeUrl(url) {
2+
let u = decodeURI(url).trim().toLowerCase();
3+
if (u.startsWith("javascript:"))
4+
return "about:blank";
5+
return url;
6+
}

‎javascript/ql/src/Security/CWE-020/examples/IncompleteUrlSchemeCheckGood.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
function sanitizeUrl(url) {
2+
let u = decodeURI(url).trim().toLowerCase();
3+
if (u.startsWith("javascript:") || u.startsWith("data:") || u.startsWith("vbscript:"))
4+
return "about:blank";
5+
return url;
6+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
| IncompleteUrlSchemeCheck.js:3:9:3:35 | u.start ... ript:") | This check does not consider data: and vbscript:. |

‎javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
function sanitizeUrl(url) {
2+
let u = decodeURI(url).trim().toLowerCase();
3+
if (u.startsWith("javascript:"))
4+
return "about:blank";
5+
return url;
6+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-020/IncompleteUrlSchemeCheck.ql

‎javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheckGood.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
function sanitizeUrl(url) {
2+
let u = decodeURI(url).trim().toLowerCase();
3+
if (u.startsWith("javascript:") || u.startsWith("data:") || u.startsWith("vbscript:"))
4+
return "about:blank";
5+
return url;
6+
}

0 commit comments

Comments
 (0)