Skip to content

Commit b4b37b3

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #880 from esben-semmle/js/better-alert-message-1
Approved by xiemaisi
2 parents 812cba0 + b72441f commit b4b37b3

File tree

3 files changed

+111
-86
lines changed

3 files changed

+111
-86
lines changed

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

Lines changed: 33 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -13,17 +13,27 @@
1313
import javascript
1414
private import semmle.javascript.dataflow.InferredTypes
1515

16-
from DataFlow::MethodCallNode call, string name, DataFlow::Node substring, string target
16+
/**
17+
* A check on a string for whether it contains a given substring, possibly with restrictions on the location of the substring.
18+
*/
19+
class SomeSubstringCheck extends DataFlow::Node {
20+
DataFlow::Node substring;
21+
22+
SomeSubstringCheck() {
23+
this.(StringOps::StartsWith).getSubstring() = substring or
24+
this.(StringOps::Includes).getSubstring() = substring or
25+
this.(StringOps::EndsWith).getSubstring() = substring
26+
}
27+
28+
/**
29+
* Gets the substring.
30+
*/
31+
DataFlow::Node getSubstring() { result = substring }
32+
}
33+
34+
from SomeSubstringCheck check, DataFlow::Node substring, string target, string msg
1735
where
18-
(
19-
name = "indexOf" or
20-
name = "lastIndexOf" or
21-
name = "includes" or
22-
name = "startsWith" or
23-
name = "endsWith"
24-
) and
25-
call.getMethodName() = name and
26-
substring = call.getArgument(0) and
36+
substring = check.getSubstring() and
2737
substring.mayHaveStringValue(target) and
2838
(
2939
// target contains a domain on a common TLD, and perhaps some other URL components
@@ -34,33 +44,22 @@ where
3444
// target is a HTTP URL to a domain on any TLD
3545
target.regexpMatch("(?i)https?://([a-z0-9-]+\\.)+([a-z]+)(:[0-9]+)?/?")
3646
) and
47+
(
48+
if check instanceof StringOps::StartsWith
49+
then msg = "may be followed by an arbitrary host name"
50+
else
51+
if check instanceof StringOps::EndsWith
52+
then msg = "may be preceded by an arbitrary host name"
53+
else msg = "can be anywhere in the URL, and arbitrary hosts may come before or after it"
54+
) and
3755
// whitelist
3856
not (
39-
(name = "indexOf" or name = "lastIndexOf") and
40-
(
41-
// arithmetic on the indexOf-result
42-
any(ArithmeticExpr e).getAnOperand().getUnderlyingValue() = call.asExpr()
43-
or
44-
// non-trivial position check on the indexOf-result
45-
exists(EqualityTest test, Expr n | test.hasOperands(call.asExpr(), n) |
46-
not n.getIntValue() = [-1 .. 0]
47-
)
48-
)
49-
or
5057
// the leading dot in a subdomain sequence makes the suffix-check safe (if it is performed on the host of the url)
51-
name = "endsWith" and
58+
check instanceof StringOps::EndsWith and
5259
target.regexpMatch("(?i)\\.([a-z0-9-]+)(\\.[a-z0-9-]+)+")
5360
or
54-
// the trailing slash makes the prefix-check safe
55-
(
56-
name = "startsWith"
57-
or
58-
name = "indexOf" and
59-
exists(EqualityTest test, Expr n |
60-
test.hasOperands(call.asExpr(), n) and
61-
n.getIntValue() = 0
62-
)
63-
) and
64-
target.regexpMatch(".*/")
61+
// the trailing port or slash makes the prefix-check safe
62+
check instanceof StringOps::StartsWith and
63+
target.regexpMatch(".*(:[0-9]+|/)")
6564
)
66-
select call, "'$@' may be at an arbitrary position in the sanitized URL.", substring, target
65+
select check, "'$@' " + msg + ".", substring, target

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

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,22 @@
1-
| tst-IncompleteUrlSubstringSanitization.js:4:5:4:27 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:4:15:4:26 | "secure.com" | secure.com |
2-
| tst-IncompleteUrlSubstringSanitization.js:5:5:5:27 | x.index ... e.net") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:5:15:5:26 | "secure.net" | secure.net |
3-
| tst-IncompleteUrlSubstringSanitization.js:6:5:6:28 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:6:15:6:27 | ".secure.com" | .secure.com |
4-
| tst-IncompleteUrlSubstringSanitization.js:10:5:10:27 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:10:15:10:26 | "secure.com" | secure.com |
5-
| tst-IncompleteUrlSubstringSanitization.js:11:5:11:27 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:11:15:11:26 | "secure.com" | secure.com |
6-
| tst-IncompleteUrlSubstringSanitization.js:12:5:12:27 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:12:15:12:26 | "secure.com" | secure.com |
7-
| tst-IncompleteUrlSubstringSanitization.js:14:5:14:38 | x.start ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:14:18:14:37 | "https://secure.com" | https://secure.com |
8-
| tst-IncompleteUrlSubstringSanitization.js:15:5:15:28 | x.endsW ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:15:16:15:27 | "secure.com" | secure.com |
9-
| tst-IncompleteUrlSubstringSanitization.js:20:5:20:28 | x.inclu ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:20:16:20:27 | "secure.com" | secure.com |
10-
| tst-IncompleteUrlSubstringSanitization.js:32:5:32:35 | x.index ... e.com") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:32:15:32:34 | "https://secure.com" | https://secure.com |
11-
| tst-IncompleteUrlSubstringSanitization.js:33:5:33:39 | x.index ... m:443") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:33:15:33:38 | "https: ... om:443" | https://secure.com:443 |
12-
| tst-IncompleteUrlSubstringSanitization.js:34:5:34:36 | x.index ... .com/") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:34:15:34:35 | "https: ... e.com/" | https://secure.com/ |
13-
| tst-IncompleteUrlSubstringSanitization.js:52:5:52:41 | x.index ... ernal") | '$@' may be at an arbitrary position in the sanitized URL. | tst-IncompleteUrlSubstringSanitization.js:52:15:52:40 | "https: ... ternal" | https://example.internal |
1+
| tst-IncompleteUrlSubstringSanitization.js:4:5:4:34 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:4:15:4:26 | "secure.com" | secure.com |
2+
| tst-IncompleteUrlSubstringSanitization.js:5:5:5:34 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:5:15:5:26 | "secure.net" | secure.net |
3+
| tst-IncompleteUrlSubstringSanitization.js:6:5:6:35 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:6:15:6:27 | ".secure.com" | .secure.com |
4+
| tst-IncompleteUrlSubstringSanitization.js:10:5:10:34 | x.index ... === -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:10:15:10:26 | "secure.com" | secure.com |
5+
| tst-IncompleteUrlSubstringSanitization.js:11:5:11:33 | x.index ... ) === 0 | '$@' may be followed by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:11:15:11:26 | "secure.com" | secure.com |
6+
| tst-IncompleteUrlSubstringSanitization.js:12:5:12:32 | x.index ... ") >= 0 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:12:15:12:26 | "secure.com" | secure.com |
7+
| tst-IncompleteUrlSubstringSanitization.js:14:5:14:38 | x.start ... e.com") | '$@' may be followed by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:14:18:14:37 | "https://secure.com" | https://secure.com |
8+
| tst-IncompleteUrlSubstringSanitization.js:15:5:15:28 | x.endsW ... e.com") | '$@' may be preceded by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:15:16:15:27 | "secure.com" | secure.com |
9+
| tst-IncompleteUrlSubstringSanitization.js:20:5:20:28 | x.inclu ... e.com") | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:20:16:20:27 | "secure.com" | secure.com |
10+
| tst-IncompleteUrlSubstringSanitization.js:32:5:32:42 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:32:15:32:34 | "https://secure.com" | https://secure.com |
11+
| tst-IncompleteUrlSubstringSanitization.js:33:5:33:46 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:33:15:33:38 | "https: ... om:443" | https://secure.com:443 |
12+
| tst-IncompleteUrlSubstringSanitization.js:34:5:34:43 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:34:15:34:35 | "https: ... e.com/" | https://secure.com/ |
13+
| tst-IncompleteUrlSubstringSanitization.js:52:5:52:48 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:52:15:52:40 | "https: ... ternal" | https://example.internal |
14+
| tst-IncompleteUrlSubstringSanitization.js:55:5:55:44 | x.start ... ernal") | '$@' may be followed by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:55:18:55:43 | "https: ... ternal" | https://example.internal |
15+
| tst-IncompleteUrlSubstringSanitization.js:56:5:56:51 | x.index ... ) !== 0 | '$@' may be followed by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:56:15:56:44 | 'https: ... al.org' | https://example.internal.org |
16+
| tst-IncompleteUrlSubstringSanitization.js:57:5:57:51 | x.index ... ) === 0 | '$@' may be followed by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:57:15:57:44 | 'https: ... al.org' | https://example.internal.org |
17+
| tst-IncompleteUrlSubstringSanitization.js:58:5:58:30 | x.endsW ... l.com") | '$@' may be preceded by an arbitrary host name. | tst-IncompleteUrlSubstringSanitization.js:58:16:58:29 | "internal.com" | internal.com |
18+
| tst-IncompleteUrlSubstringSanitization.js:61:2:61:31 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:61:12:61:23 | "secure.com" | secure.com |
19+
| tst-IncompleteUrlSubstringSanitization.js:62:2:62:31 | x.index ... === -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:62:12:62:23 | "secure.com" | secure.com |
20+
| tst-IncompleteUrlSubstringSanitization.js:63:4:63:33 | x.index ... !== -1 | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:63:14:63:25 | "secure.com" | secure.com |
21+
| tst-IncompleteUrlSubstringSanitization.js:64:3:64:26 | x.inclu ... e.com") | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:64:14:64:25 | "secure.com" | secure.com |
22+
| tst-IncompleteUrlSubstringSanitization.js:66:6:66:29 | x.inclu ... e.com") | '$@' can be anywhere in the URL, and arbitrary hosts may come before or after it. | tst-IncompleteUrlSubstringSanitization.js:66:17:66:28 | "secure.com" | secure.com |

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

Lines changed: 56 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
(function(x){
2-
x.indexOf("internal"); // NOT OK, but not flagged
3-
x.indexOf("localhost"); // NOT OK, but not flagged
4-
x.indexOf("secure.com"); // NOT OK
5-
x.indexOf("secure.net"); // NOT OK
6-
x.indexOf(".secure.com"); // NOT OK
7-
x.indexOf("sub.secure."); // NOT OK, but not flagged
8-
x.indexOf(".sub.secure."); // NOT OK, but not flagged
2+
x.indexOf("internal") !== -1; // NOT OK, but not flagged
3+
x.indexOf("localhost") !== -1; // NOT OK, but not flagged
4+
x.indexOf("secure.com") !== -1; // NOT OK
5+
x.indexOf("secure.net") !== -1; // NOT OK
6+
x.indexOf(".secure.com") !== -1; // NOT OK
7+
x.indexOf("sub.secure.") !== -1; // NOT OK, but not flagged
8+
x.indexOf(".sub.secure.") !== -1; // NOT OK, but not flagged
99

1010
x.indexOf("secure.com") === -1; // NOT OK
1111
x.indexOf("secure.com") === 0; // NOT OK
@@ -19,36 +19,53 @@
1919

2020
x.includes("secure.com"); // NOT OK
2121

22-
x.indexOf("#"); // OK
23-
x.indexOf(":"); // OK
24-
x.indexOf(":/"); // OK
25-
x.indexOf("://"); // OK
26-
x.indexOf("//"); // OK
27-
x.indexOf(":443"); // OK
28-
x.indexOf("/some/path/"); // OK
29-
x.indexOf("some/path"); // OK
30-
x.indexOf("/index.html"); // OK
31-
x.indexOf(":template:"); // OK
32-
x.indexOf("https://secure.com"); // NOT OK
33-
x.indexOf("https://secure.com:443"); // NOT OK
34-
x.indexOf("https://secure.com/"); // NOT OK
35-
36-
x.indexOf(".cn"); // NOT OK, but not flagged
37-
x.indexOf(".jpg"); // OK
38-
x.indexOf("index.html"); // OK
39-
x.indexOf("index.js"); // OK
40-
x.indexOf("index.php"); // OK
41-
x.indexOf("index.css"); // OK
42-
43-
x.indexOf("secure=true"); // OK (query param)
44-
x.indexOf("&auth="); // OK (query param)
45-
46-
x.indexOf(getCurrentDomain()); // NOT OK, but not flagged
47-
x.indexOf(location.origin); // NOT OK, but not flagged
48-
49-
x.indexOf("tar.gz") + offset // OK
50-
x.indexOf("tar.gz") - offset // OK
51-
52-
x.indexOf("https://example.internal"); // NOT OK
53-
x.indexOf("https://"); // OK
22+
x.indexOf("#") !== -1; // OK
23+
x.indexOf(":") !== -1; // OK
24+
x.indexOf(":/") !== -1; // OK
25+
x.indexOf("://") !== -1; // OK
26+
x.indexOf("//") !== -1; // OK
27+
x.indexOf(":443") !== -1; // OK
28+
x.indexOf("/some/path/") !== -1; // OK
29+
x.indexOf("some/path") !== -1; // OK
30+
x.indexOf("/index.html") !== -1; // OK
31+
x.indexOf(":template:") !== -1; // OK
32+
x.indexOf("https://secure.com") !== -1; // NOT OK
33+
x.indexOf("https://secure.com:443") !== -1; // NOT OK
34+
x.indexOf("https://secure.com/") !== -1; // NOT OK
35+
36+
x.indexOf(".cn") !== -1; // NOT OK, but not flagged
37+
x.indexOf(".jpg") !== -1; // OK
38+
x.indexOf("index.html") !== -1; // OK
39+
x.indexOf("index.js") !== -1; // OK
40+
x.indexOf("index.php") !== -1; // OK
41+
x.indexOf("index.css") !== -1; // OK
42+
43+
x.indexOf("secure=true") !== -1; // OK (query param)
44+
x.indexOf("&auth=") !== -1; // OK (query param)
45+
46+
x.indexOf(getCurrentDomain()) !== -1; // NOT OK, but not flagged
47+
x.indexOf(location.origin) !== -1; // NOT OK, but not flagged
48+
49+
x.indexOf("tar.gz") + offset; // OK
50+
x.indexOf("tar.gz") - offset; // OK
51+
52+
x.indexOf("https://example.internal") !== -1; // NOT OK
53+
x.indexOf("https://") !== -1; // OK
54+
55+
x.startsWith("https://example.internal"); // NOT OK
56+
x.indexOf('https://example.internal.org') !== 0; // NOT OK
57+
x.indexOf('https://example.internal.org') === 0; // NOT OK
58+
x.endsWith("internal.com"); // NOT OK
59+
x.startsWith("https://example.internal:80"); // OK
60+
61+
x.indexOf("secure.com") !== -1; // NOT OK
62+
x.indexOf("secure.com") === -1; // OK
63+
!(x.indexOf("secure.com") !== -1); // OK
64+
!x.includes("secure.com"); // OK
65+
66+
if(!x.includes("secure.com")) { // NOT OK
67+
68+
} else {
69+
doSomeThingWithTrustedURL(x);
70+
}
5471
});

0 commit comments

Comments
 (0)