Skip to content

Commit be617ce

Browse files
asgerfasgerf
committed
JS: More precise handling of .exec()
1 parent 703cad9 commit be617ce

File tree

4 files changed

+26
-57
lines changed

4 files changed

+26
-57
lines changed

‎javascript/ql/lib/semmle/javascript/security/TaintedUrlSuffixCustomizations.qll‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -106,12 +106,29 @@ module TaintedUrlSuffix {
106106
read.getPropertyName() = "query"
107107
)
108108
or
109-
// Assume calls to regexp.exec always extract query/fragment parameters.
110-
exists(MethodCallNode call |
111-
call = any(DataFlow::RegExpCreationNode re).getAMethodCall("exec") and
109+
exists(MethodCallNode call, DataFlow::RegExpCreationNode re |
110+
call = re.getAMethodCall("exec") and
112111
src = call.getArgument(0) and
113-
dst = call
112+
dst = call and
113+
captureAfterSuffixIndicator(re.getRoot().getAChild*())
114114
)
115115
)
116116
}
117+
118+
private predicate containsSuffixIndicator(RegExpSequence seq, int n) {
119+
// Also include '=' as it usually only appears in the URL suffix
120+
seq.getChild(n).getAChild*().(RegExpConstant).getValue().regexpMatch(".*[?#=].*")
121+
}
122+
123+
private predicate containsCaptureGroup(RegExpSequence seq, int n) {
124+
seq.getChild(n).getAChild*().(RegExpGroup).isCapture()
125+
}
126+
127+
private predicate captureAfterSuffixIndicator(RegExpSequence seq) {
128+
exists(int suffix, int capture |
129+
containsSuffixIndicator(seq, suffix) and
130+
containsCaptureGroup(seq, capture) and
131+
suffix < capture
132+
)
133+
}
117134
}

‎javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected‎

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -21,21 +21,6 @@ nodes
2121
| regexp-exec.js:9:24:9:58 | /\\?(.*) ... n.href) | semmle.label | /\\?(.*) ... n.href) |
2222
| regexp-exec.js:9:38:9:57 | window.location.href | semmle.label | window.location.href |
2323
| regexp-exec.js:10:28:10:33 | group1 | semmle.label | group1 |
24-
| regexp-exec.js:14:11:14:20 | [, group1] | semmle.label | [, group1] |
25-
| regexp-exec.js:14:11:14:62 | group1 | semmle.label | group1 |
26-
| regexp-exec.js:14:24:14:62 | /^([a-z ... n.href) | semmle.label | /^([a-z ... n.href) |
27-
| regexp-exec.js:14:42:14:61 | window.location.href | semmle.label | window.location.href |
28-
| regexp-exec.js:15:28:15:33 | group1 | semmle.label | group1 |
29-
| regexp-exec.js:19:11:19:20 | [, group1] | semmle.label | [, group1] |
30-
| regexp-exec.js:19:11:19:56 | group1 | semmle.label | group1 |
31-
| regexp-exec.js:19:24:19:56 | /(.*)/. ... n.href) | semmle.label | /(.*)/. ... n.href) |
32-
| regexp-exec.js:19:36:19:55 | window.location.href | semmle.label | window.location.href |
33-
| regexp-exec.js:20:28:20:33 | group1 | semmle.label | group1 |
34-
| regexp-exec.js:24:11:24:20 | [, group1] | semmle.label | [, group1] |
35-
| regexp-exec.js:24:11:24:60 | group1 | semmle.label | group1 |
36-
| regexp-exec.js:24:24:24:60 | /blah#b ... n.href) | semmle.label | /blah#b ... n.href) |
37-
| regexp-exec.js:24:40:24:59 | window.location.href | semmle.label | window.location.href |
38-
| regexp-exec.js:25:28:25:33 | group1 | semmle.label | group1 |
3924
| sanitizer.js:2:9:2:25 | url | semmle.label | url |
4025
| sanitizer.js:2:15:2:25 | window.name | semmle.label | window.name |
4126
| sanitizer.js:4:27:4:29 | url | semmle.label | url |
@@ -159,15 +144,6 @@ nodes
159144
| tst.js:14:20:14:56 | indirec ... n.href) | semmle.label | indirec ... n.href) |
160145
| tst.js:14:20:14:59 | indirec ... ref)[1] | semmle.label | indirec ... ref)[1] |
161146
| tst.js:14:34:14:55 | documen ... on.href | semmle.label | documen ... on.href |
162-
| tst.js:18:19:18:81 | new Reg ... n.href) | semmle.label | new Reg ... n.href) |
163-
| tst.js:18:19:18:84 | new Reg ... ref)[1] | semmle.label | new Reg ... ref)[1] |
164-
| tst.js:18:59:18:80 | documen ... on.href | semmle.label | documen ... on.href |
165-
| tst.js:22:20:22:56 | indirec ... n.href) | semmle.label | indirec ... n.href) |
166-
| tst.js:22:20:22:59 | indirec ... ref)[1] | semmle.label | indirec ... ref)[1] |
167-
| tst.js:22:34:22:55 | documen ... on.href | semmle.label | documen ... on.href |
168-
| tst.js:26:22:26:79 | new Reg ... n.href) | semmle.label | new Reg ... n.href) |
169-
| tst.js:26:22:26:82 | new Reg ... ref)[1] | semmle.label | new Reg ... ref)[1] |
170-
| tst.js:26:62:26:78 | win.location.href | semmle.label | win.location.href |
171147
| typed.ts:4:13:4:49 | params | semmle.label | params |
172148
| typed.ts:4:22:4:36 | location.search | semmle.label | location.search |
173149
| typed.ts:4:22:4:49 | locatio ... ring(1) | semmle.label | locatio ... ring(1) |
@@ -201,18 +177,6 @@ edges
201177
| regexp-exec.js:9:11:9:58 | group1 | regexp-exec.js:10:28:10:33 | group1 | provenance | |
202178
| regexp-exec.js:9:24:9:58 | /\\?(.*) ... n.href) | regexp-exec.js:9:11:9:20 | [, group1] | provenance | |
203179
| regexp-exec.js:9:38:9:57 | window.location.href | regexp-exec.js:9:24:9:58 | /\\?(.*) ... n.href) | provenance | Config |
204-
| regexp-exec.js:14:11:14:20 | [, group1] | regexp-exec.js:14:11:14:62 | group1 | provenance | |
205-
| regexp-exec.js:14:11:14:62 | group1 | regexp-exec.js:15:28:15:33 | group1 | provenance | |
206-
| regexp-exec.js:14:24:14:62 | /^([a-z ... n.href) | regexp-exec.js:14:11:14:20 | [, group1] | provenance | |
207-
| regexp-exec.js:14:42:14:61 | window.location.href | regexp-exec.js:14:24:14:62 | /^([a-z ... n.href) | provenance | Config |
208-
| regexp-exec.js:19:11:19:20 | [, group1] | regexp-exec.js:19:11:19:56 | group1 | provenance | |
209-
| regexp-exec.js:19:11:19:56 | group1 | regexp-exec.js:20:28:20:33 | group1 | provenance | |
210-
| regexp-exec.js:19:24:19:56 | /(.*)/. ... n.href) | regexp-exec.js:19:11:19:20 | [, group1] | provenance | |
211-
| regexp-exec.js:19:36:19:55 | window.location.href | regexp-exec.js:19:24:19:56 | /(.*)/. ... n.href) | provenance | Config |
212-
| regexp-exec.js:24:11:24:20 | [, group1] | regexp-exec.js:24:11:24:60 | group1 | provenance | |
213-
| regexp-exec.js:24:11:24:60 | group1 | regexp-exec.js:25:28:25:33 | group1 | provenance | |
214-
| regexp-exec.js:24:24:24:60 | /blah#b ... n.href) | regexp-exec.js:24:11:24:20 | [, group1] | provenance | |
215-
| regexp-exec.js:24:40:24:59 | window.location.href | regexp-exec.js:24:24:24:60 | /blah#b ... n.href) | provenance | Config |
216180
| sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url | provenance | |
217181
| sanitizer.js:2:9:2:25 | url | sanitizer.js:16:27:16:29 | url | provenance | |
218182
| sanitizer.js:2:9:2:25 | url | sanitizer.js:19:27:19:29 | url | provenance | |
@@ -311,12 +275,6 @@ edges
311275
| tst.js:10:59:10:80 | documen ... on.href | tst.js:10:19:10:81 | new Reg ... n.href) | provenance | Config |
312276
| tst.js:14:20:14:56 | indirec ... n.href) | tst.js:14:20:14:59 | indirec ... ref)[1] | provenance | |
313277
| tst.js:14:34:14:55 | documen ... on.href | tst.js:14:20:14:56 | indirec ... n.href) | provenance | Config |
314-
| tst.js:18:19:18:81 | new Reg ... n.href) | tst.js:18:19:18:84 | new Reg ... ref)[1] | provenance | |
315-
| tst.js:18:59:18:80 | documen ... on.href | tst.js:18:19:18:81 | new Reg ... n.href) | provenance | Config |
316-
| tst.js:22:20:22:56 | indirec ... n.href) | tst.js:22:20:22:59 | indirec ... ref)[1] | provenance | |
317-
| tst.js:22:34:22:55 | documen ... on.href | tst.js:22:20:22:56 | indirec ... n.href) | provenance | Config |
318-
| tst.js:26:22:26:79 | new Reg ... n.href) | tst.js:26:22:26:82 | new Reg ... ref)[1] | provenance | |
319-
| tst.js:26:62:26:78 | win.location.href | tst.js:26:22:26:79 | new Reg ... n.href) | provenance | Config |
320278
| typed.ts:4:13:4:49 | params | typed.ts:5:25:5:30 | params | provenance | |
321279
| typed.ts:4:22:4:36 | location.search | typed.ts:4:22:4:49 | locatio ... ring(1) | provenance | Config |
322280
| typed.ts:4:22:4:49 | locatio ... ring(1) | typed.ts:4:13:4:49 | params | provenance | |
@@ -341,9 +299,6 @@ subpaths
341299
| react.js:43:19:43:50 | documen ... bstr(1) | react.js:43:19:43:40 | documen ... on.hash | react.js:43:19:43:50 | documen ... bstr(1) | Untrusted URL redirection depends on a $@. | react.js:43:19:43:40 | documen ... on.hash | user-provided value |
342300
| regexp-exec.js:5:28:5:33 | group1 | regexp-exec.js:4:37:4:56 | window.location.href | regexp-exec.js:5:28:5:33 | group1 | Untrusted URL redirection depends on a $@. | regexp-exec.js:4:37:4:56 | window.location.href | user-provided value |
343301
| regexp-exec.js:10:28:10:33 | group1 | regexp-exec.js:9:38:9:57 | window.location.href | regexp-exec.js:10:28:10:33 | group1 | Untrusted URL redirection depends on a $@. | regexp-exec.js:9:38:9:57 | window.location.href | user-provided value |
344-
| regexp-exec.js:15:28:15:33 | group1 | regexp-exec.js:14:42:14:61 | window.location.href | regexp-exec.js:15:28:15:33 | group1 | Untrusted URL redirection depends on a $@. | regexp-exec.js:14:42:14:61 | window.location.href | user-provided value |
345-
| regexp-exec.js:20:28:20:33 | group1 | regexp-exec.js:19:36:19:55 | window.location.href | regexp-exec.js:20:28:20:33 | group1 | Untrusted URL redirection depends on a $@. | regexp-exec.js:19:36:19:55 | window.location.href | user-provided value |
346-
| regexp-exec.js:25:28:25:33 | group1 | regexp-exec.js:24:40:24:59 | window.location.href | regexp-exec.js:25:28:25:33 | group1 | Untrusted URL redirection depends on a $@. | regexp-exec.js:24:40:24:59 | window.location.href | user-provided value |
347302
| sanitizer.js:4:27:4:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:4:27:4:29 | url | Untrusted URL redirection depends on a $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
348303
| sanitizer.js:16:27:16:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:16:27:16:29 | url | Untrusted URL redirection depends on a $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
349304
| sanitizer.js:19:27:19:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:19:27:19:29 | url | Untrusted URL redirection depends on a $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
@@ -396,9 +351,6 @@ subpaths
396351
| tst.js:6:20:6:59 | indirec ... ref)[1] | tst.js:6:34:6:55 | documen ... on.href | tst.js:6:20:6:59 | indirec ... ref)[1] | Untrusted URL redirection depends on a $@. | tst.js:6:34:6:55 | documen ... on.href | user-provided value |
397352
| tst.js:10:19:10:84 | new Reg ... ref)[1] | tst.js:10:59:10:80 | documen ... on.href | tst.js:10:19:10:84 | new Reg ... ref)[1] | Untrusted URL redirection depends on a $@. | tst.js:10:59:10:80 | documen ... on.href | user-provided value |
398353
| tst.js:14:20:14:59 | indirec ... ref)[1] | tst.js:14:34:14:55 | documen ... on.href | tst.js:14:20:14:59 | indirec ... ref)[1] | Untrusted URL redirection depends on a $@. | tst.js:14:34:14:55 | documen ... on.href | user-provided value |
399-
| tst.js:18:19:18:84 | new Reg ... ref)[1] | tst.js:18:59:18:80 | documen ... on.href | tst.js:18:19:18:84 | new Reg ... ref)[1] | Untrusted URL redirection depends on a $@. | tst.js:18:59:18:80 | documen ... on.href | user-provided value |
400-
| tst.js:22:20:22:59 | indirec ... ref)[1] | tst.js:22:34:22:55 | documen ... on.href | tst.js:22:20:22:59 | indirec ... ref)[1] | Untrusted URL redirection depends on a $@. | tst.js:22:34:22:55 | documen ... on.href | user-provided value |
401-
| tst.js:26:22:26:82 | new Reg ... ref)[1] | tst.js:26:62:26:78 | win.location.href | tst.js:26:22:26:82 | new Reg ... ref)[1] | Untrusted URL redirection depends on a $@. | tst.js:26:62:26:78 | win.location.href | user-provided value |
402354
| typed.ts:8:33:8:43 | redirectUri | typed.ts:4:22:4:36 | location.search | typed.ts:8:33:8:43 | redirectUri | Untrusted URL redirection depends on a $@. | typed.ts:4:22:4:36 | location.search | user-provided value |
403355
| typed.ts:29:33:29:43 | redirectUri | typed.ts:25:25:25:34 | loc.search | typed.ts:29:33:29:43 | redirectUri | Untrusted URL redirection depends on a $@. | typed.ts:25:25:25:34 | loc.search | user-provided value |
404356
| typed.ts:52:33:52:43 | redirectUri | typed.ts:47:25:47:34 | loc.search | typed.ts:52:33:52:43 | redirectUri | Untrusted URL redirection depends on a $@. | typed.ts:47:25:47:34 | loc.search | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/regexp-exec.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,15 @@ function extractFromQuery() {
1212

1313
function extractFromProtocol() {
1414
const [, group1] = /^([a-z]+:)/.exec(window.location.href);
15-
window.location.href = group1; // OK [INCONSISTENCY]
15+
window.location.href = group1; // OK
1616
}
1717

1818
function extractTooMuch() {
1919
const [, group1] = /(.*)/.exec(window.location.href);
20-
window.location.href = group1; // OK [INCONSISTENCY]
20+
window.location.href = group1; // OK
2121
}
2222

2323
function extractNothing() {
2424
const [, group1] = /blah#baz/.exec(window.location.href);
25-
window.location.href = group1; // OK [INCONSISTENCY]
25+
window.location.href = group1; // OK
2626
}

‎javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ window.location = new RegExp('.*redirect=([^&]*).*').exec(document.location.href
1414
window.location = indirect.exec(document.location.href)[1];
1515
});
1616

17-
// NOT OK
17+
// NOT OK [INCONSISTENCY]
1818
window.location = new RegExp(/.*redirect=([^&]*).*/).exec(document.location.href)[1];
1919

2020
(function(){
@@ -23,7 +23,7 @@ window.location = new RegExp(/.*redirect=([^&]*).*/).exec(document.location.href
2323
});
2424

2525
function foo(win) {
26-
win.location.assign(new RegExp(/.*redirect=([^&]*).*/).exec(win.location.href)[1]); // NOT OK
26+
win.location.assign(new RegExp(/.*redirect=([^&]*).*/).exec(win.location.href)[1]); // NOT OK [INCONSISTENCY]
2727
}
2828

2929
foo(window);

0 commit comments

Comments
 (0)