Skip to content

Commit 33dab17

Browse files
erik-krogherik-krogh
committed
treat nodes with type "Location" as a ___location source - but not if we can track it from an original node with type "Location"
1 parent 7d38b2d commit 33dab17

File tree

4 files changed

+94
-0
lines changed

4 files changed

+94
-0
lines changed

‎javascript/ql/src/semmle/javascript/DOM.qll

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,10 +374,26 @@ module DOM {
374374
this = DOM::domValueRef().getAPropertyRead("baseUri")
375375
or
376376
this = DataFlow::globalVarRef("___location")
377+
or
378+
this = any(DataFlow::Node n | n.hasUnderlyingType("Location")).getALocalSource() and
379+
not this = nonFirstLocationType(DataFlow::TypeTracker::end()) // only start from the source, and not the locations we can type-track to.
377380
}
378381
}
379382
}
380383

384+
/**
385+
* Get a reference to a node of type `Location` that has gone through at least 1 type-tracking step.
386+
*/
387+
private DataFlow::SourceNode nonFirstLocationType(DataFlow::TypeTracker t) {
388+
// One step inlined in the beginning.
389+
exists(DataFlow::TypeTracker t2 |
390+
result =
391+
any(DataFlow::Node n | n.hasUnderlyingType("Location")).getALocalSource().track(t2, t)
392+
)
393+
or
394+
exists(DataFlow::TypeTracker t2 | result = nonFirstLocationType(t2).track(t2, t))
395+
}
396+
381397
/** Gets a data flow node that directly refers to a DOM `___location` object. */
382398
DataFlow::SourceNode locationSource() { result instanceof LocationSource::Range }
383399

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,29 @@ nodes
157157
| tst.js:22:34:22:50 | document.___location |
158158
| tst.js:22:34:22:50 | document.___location |
159159
| tst.js:22:34:22:55 | documen ... on.href |
160+
| typed.ts:3:15:3:72 | ___location |
161+
| typed.ts:3:17:3:24 | ___location |
162+
| typed.ts:3:17:3:24 | ___location |
163+
| typed.ts:4:13:4:36 | params |
164+
| typed.ts:4:22:4:29 | ___location |
165+
| typed.ts:4:22:4:36 | ___location.search |
166+
| typed.ts:5:25:5:30 | params |
167+
| typed.ts:7:24:7:34 | redirectUri |
168+
| typed.ts:8:33:8:43 | redirectUri |
169+
| typed.ts:8:33:8:43 | redirectUri |
170+
| typed.ts:14:15:14:72 | ___location |
171+
| typed.ts:14:17:14:24 | ___location |
172+
| typed.ts:14:17:14:24 | ___location |
173+
| typed.ts:17:18:17:25 | ___location |
174+
| typed.ts:19:13:19:37 | secondLoc |
175+
| typed.ts:19:25:19:37 | container.loc |
176+
| typed.ts:21:33:21:41 | secondLoc |
177+
| typed.ts:24:32:24:34 | loc |
178+
| typed.ts:25:25:25:27 | loc |
179+
| typed.ts:25:25:25:34 | loc.search |
180+
| typed.ts:28:24:28:34 | redirectUri |
181+
| typed.ts:29:33:29:43 | redirectUri |
182+
| typed.ts:29:33:29:43 | redirectUri |
160183
edges
161184
| sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url |
162185
| sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url |
@@ -304,6 +327,27 @@ edges
304327
| tst.js:22:34:22:50 | document.___location | tst.js:22:34:22:55 | documen ... on.href |
305328
| tst.js:22:34:22:50 | document.___location | tst.js:22:34:22:55 | documen ... on.href |
306329
| tst.js:22:34:22:55 | documen ... on.href | tst.js:22:20:22:56 | indirec ... n.href) |
330+
| typed.ts:3:15:3:72 | ___location | typed.ts:4:22:4:29 | ___location |
331+
| typed.ts:3:17:3:24 | ___location | typed.ts:3:15:3:72 | ___location |
332+
| typed.ts:3:17:3:24 | ___location | typed.ts:3:15:3:72 | ___location |
333+
| typed.ts:4:13:4:36 | params | typed.ts:5:25:5:30 | params |
334+
| typed.ts:4:22:4:29 | ___location | typed.ts:4:22:4:36 | ___location.search |
335+
| typed.ts:4:22:4:36 | ___location.search | typed.ts:4:13:4:36 | params |
336+
| typed.ts:5:25:5:30 | params | typed.ts:7:24:7:34 | redirectUri |
337+
| typed.ts:7:24:7:34 | redirectUri | typed.ts:8:33:8:43 | redirectUri |
338+
| typed.ts:7:24:7:34 | redirectUri | typed.ts:8:33:8:43 | redirectUri |
339+
| typed.ts:14:15:14:72 | ___location | typed.ts:17:18:17:25 | ___location |
340+
| typed.ts:14:17:14:24 | ___location | typed.ts:14:15:14:72 | ___location |
341+
| typed.ts:14:17:14:24 | ___location | typed.ts:14:15:14:72 | ___location |
342+
| typed.ts:17:18:17:25 | ___location | typed.ts:19:25:19:37 | container.loc |
343+
| typed.ts:19:13:19:37 | secondLoc | typed.ts:21:33:21:41 | secondLoc |
344+
| typed.ts:19:25:19:37 | container.loc | typed.ts:19:13:19:37 | secondLoc |
345+
| typed.ts:21:33:21:41 | secondLoc | typed.ts:24:32:24:34 | loc |
346+
| typed.ts:24:32:24:34 | loc | typed.ts:25:25:25:27 | loc |
347+
| typed.ts:25:25:25:27 | loc | typed.ts:25:25:25:34 | loc.search |
348+
| typed.ts:25:25:25:34 | loc.search | typed.ts:28:24:28:34 | redirectUri |
349+
| typed.ts:28:24:28:34 | redirectUri | typed.ts:29:33:29:43 | redirectUri |
350+
| typed.ts:28:24:28:34 | redirectUri | typed.ts:29:33:29:43 | redirectUri |
307351
#select
308352
| 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 due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
309353
| 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 due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
@@ -344,3 +388,5 @@ edges
344388
| tst.js:14:20:14:59 | indirec ... ref)[1] | tst.js:14:34:14:50 | document.___location | tst.js:14:20:14:59 | indirec ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:14:34:14:50 | document.___location | user-provided value |
345389
| tst.js:18:19:18:84 | new Reg ... ref)[1] | tst.js:18:59:18:75 | document.___location | tst.js:18:19:18:84 | new Reg ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:18:59:18:75 | document.___location | user-provided value |
346390
| tst.js:22:20:22:59 | indirec ... ref)[1] | tst.js:22:34:22:50 | document.___location | tst.js:22:20:22:59 | indirec ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:22:34:22:50 | document.___location | user-provided value |
391+
| typed.ts:8:33:8:43 | redirectUri | typed.ts:3:17:3:24 | ___location | typed.ts:8:33:8:43 | redirectUri | Untrusted URL redirection due to $@. | typed.ts:3:17:3:24 | ___location | user-provided value |
392+
| typed.ts:29:33:29:43 | redirectUri | typed.ts:14:17:14:24 | ___location | typed.ts:29:33:29:43 | redirectUri | Untrusted URL redirection due to $@. | typed.ts:14:17:14:24 | ___location | user-provided value |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{}

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
export class MyComponent {
2+
componentDidMount() {
3+
const { ___location }: { ___location: Location } = (this as any).props;
4+
var params = ___location.search;
5+
this.doRedirect(params);
6+
}
7+
private doRedirect(redirectUri: string) {
8+
window.___location.replace(redirectUri);
9+
}
10+
}
11+
12+
export class MyTrackingComponent {
13+
componentDidMount() {
14+
const { ___location }: { ___location: Location } = (this as any).props; // ___location source
15+
16+
var container = {
17+
loc: ___location
18+
};
19+
var secondLoc = container.loc; // type-tracking step 1 - not the source
20+
21+
this.myIndirectRedirect(secondLoc);
22+
}
23+
24+
private myIndirectRedirect(loc) { // type-tracking step 2 - also not the source
25+
this.doRedirect(loc.search);
26+
}
27+
28+
private doRedirect(redirectUri: string) {
29+
window.___location.replace(redirectUri);
30+
}
31+
}

0 commit comments

Comments
 (0)