Skip to content

Commit a63b0b2

Browse files
erik-krogherik-krogh
committed
refactor the history library model, add support for the global variable
1 parent 5419143 commit a63b0b2

File tree

6 files changed

+75
-34
lines changed

6 files changed

+75
-34
lines changed

‎javascript/ql/src/javascript.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ import semmle.javascript.frameworks.FormParsers
9393
import semmle.javascript.frameworks.jQuery
9494
import semmle.javascript.frameworks.JWT
9595
import semmle.javascript.frameworks.Handlebars
96+
import semmle.javascript.frameworks.History
9697
import semmle.javascript.frameworks.Immutable
9798
import semmle.javascript.frameworks.LazyCache
9899
import semmle.javascript.frameworks.LodashUnderscore

‎javascript/ql/src/semmle/javascript/frameworks/History.qll

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/** Provides classes and predicates modelling aspects of the [`history`](https://npmjs.org/package/history) library. */
2+
3+
import javascript
4+
5+
module History {
6+
/** The global variable `HistoryLibrary` as an entry point for API graphs. */
7+
private class HistoryGlobalEntry extends API::EntryPoint {
8+
HistoryGlobalEntry() { this = "HistoryLibrary" }
9+
10+
override DataFlow::SourceNode getAUse() { result = DataFlow::globalVarRef("HistoryLibrary") }
11+
12+
override DataFlow::Node getARhs() { none() }
13+
}
14+
15+
/**
16+
* Gets a reference to the [`history`](https://npmjs.org/package/history) library.
17+
*/
18+
private API::Node history() {
19+
result = [API::moduleImport("history"), API::root().getASuccessor(any(HistoryGlobalEntry h))]
20+
}
21+
22+
/**
23+
* Gets a browser history instance.
24+
* This history instance uses the native browser history API.
25+
*/
26+
API::Node getBrowserHistory() { result = history().getMember("createBrowserHistory").getReturn() }
27+
28+
/**
29+
* Gets a hash history instance.
30+
* This history instance only manipulates the URL hash, which cannot cause XSS.
31+
*/
32+
API::Node getHashHistory() { result = history().getMember("createHashHistory").getReturn() }
33+
34+
/**
35+
* A user-controlled location value read from the [history](http://npmjs.org/package/history) library.
36+
*/
37+
private class HistoryLibaryRemoteFlow extends ClientSideRemoteFlowSource {
38+
ClientSideRemoteFlowKind kind;
39+
40+
HistoryLibaryRemoteFlow() {
41+
exists(API::Node loc | loc = [getBrowserHistory(), getHashHistory()].getMember("location") |
42+
this = loc.getMember("hash").getAnImmediateUse() and kind.isFragment()
43+
or
44+
this = loc.getMember("pathname").getAnImmediateUse() and kind.isPath()
45+
or
46+
this = loc.getMember("search").getAnImmediateUse() and kind.isQuery()
47+
)
48+
}
49+
50+
override string getSourceType() { result = "Window location" }
51+
52+
override ClientSideRemoteFlowKind getKind() { result = kind }
53+
}
54+
}

‎javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -196,13 +196,7 @@ module ClientSideUrlRedirect {
196196
*/
197197
class HistoryWriteUrlSink extends ScriptUrlSink {
198198
HistoryWriteUrlSink() {
199-
this =
200-
API::moduleImport("history")
201-
.getMember("createBrowserHistory")
202-
.getReturn()
203-
.getMember(["push", "replace"])
204-
.getACall()
205-
.getArgument(0)
199+
this = History::getBrowserHistory().getMember(["push", "replace"]).getACall().getArgument(0)
206200
}
207201
}
208202

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

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -272,30 +272,3 @@ private class WindowLocationFlowSource extends ClientSideRemoteFlowSource {
272272

273273
override ClientSideRemoteFlowKind getKind() { result = kind }
274274
}
275-
276-
/**
277-
* A user-controlled location value read from the [history](http://npmjs.org/package/history) library.
278-
*/
279-
private class HistoryLibaryRemoteFlow extends ClientSideRemoteFlowSource {
280-
ClientSideRemoteFlowKind kind;
281-
282-
HistoryLibaryRemoteFlow() {
283-
exists(API::Node loc |
284-
loc =
285-
API::moduleImport("history")
286-
.getMember(["createBrowserHistory", "createHashHistory"])
287-
.getReturn()
288-
.getMember("location")
289-
|
290-
this = loc.getMember("hash").getAnImmediateUse() and kind.isFragment()
291-
or
292-
this = loc.getMember("pathname").getAnImmediateUse() and kind.isPath()
293-
or
294-
this = loc.getMember("search").getAnImmediateUse() and kind.isQuery()
295-
)
296-
}
297-
298-
override string getSourceType() { result = "Window location" }
299-
300-
override ClientSideRemoteFlowKind getKind() { result = kind }
301-
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,12 @@ nodes
141141
| tst13.js:65:19:65:49 | history ... bstr(1) |
142142
| tst13.js:67:21:67:27 | payload |
143143
| tst13.js:67:21:67:27 | payload |
144+
| tst13.js:72:9:72:49 | payload |
145+
| tst13.js:72:19:72:39 | history ... on.hash |
146+
| tst13.js:72:19:72:39 | history ... on.hash |
147+
| tst13.js:72:19:72:49 | history ... bstr(1) |
148+
| tst13.js:74:21:74:27 | payload |
149+
| tst13.js:74:21:74:27 | payload |
144150
| tst.js:2:19:2:69 | /.*redi ... n.href) |
145151
| tst.js:2:19:2:72 | /.*redi ... ref)[1] |
146152
| tst.js:2:19:2:72 | /.*redi ... ref)[1] |
@@ -328,6 +334,11 @@ edges
328334
| tst13.js:65:19:65:39 | history ... on.hash | tst13.js:65:19:65:49 | history ... bstr(1) |
329335
| tst13.js:65:19:65:39 | history ... on.hash | tst13.js:65:19:65:49 | history ... bstr(1) |
330336
| tst13.js:65:19:65:49 | history ... bstr(1) | tst13.js:65:9:65:49 | payload |
337+
| tst13.js:72:9:72:49 | payload | tst13.js:74:21:74:27 | payload |
338+
| tst13.js:72:9:72:49 | payload | tst13.js:74:21:74:27 | payload |
339+
| tst13.js:72:19:72:39 | history ... on.hash | tst13.js:72:19:72:49 | history ... bstr(1) |
340+
| tst13.js:72:19:72:39 | history ... on.hash | tst13.js:72:19:72:49 | history ... bstr(1) |
341+
| tst13.js:72:19:72:49 | history ... bstr(1) | tst13.js:72:9:72:49 | payload |
331342
| tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] |
332343
| tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] |
333344
| tst.js:2:47:2:63 | document.location | tst.js:2:47:2:68 | documen ... on.href |
@@ -421,6 +432,7 @@ edges
421432
| tst13.js:53:28:53:28 | e | tst13.js:52:34:52:34 | e | tst13.js:53:28:53:28 | e | Untrusted URL redirection due to $@. | tst13.js:52:34:52:34 | e | user-provided value |
422433
| tst13.js:61:18:61:24 | payload | tst13.js:59:19:59:42 | documen ... .search | tst13.js:61:18:61:24 | payload | Untrusted URL redirection due to $@. | tst13.js:59:19:59:42 | documen ... .search | user-provided value |
423434
| tst13.js:67:21:67:27 | payload | tst13.js:65:19:65:39 | history ... on.hash | tst13.js:67:21:67:27 | payload | Untrusted URL redirection due to $@. | tst13.js:65:19:65:39 | history ... on.hash | user-provided value |
435+
| tst13.js:74:21:74:27 | payload | tst13.js:72:19:72:39 | history ... on.hash | tst13.js:74:21:74:27 | payload | Untrusted URL redirection due to $@. | tst13.js:72:19:72:39 | history ... on.hash | user-provided value |
424436
| tst.js:2:19:2:72 | /.*redi ... ref)[1] | tst.js:2:47:2:63 | document.location | tst.js:2:19:2:72 | /.*redi ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:2:47:2:63 | document.location | user-provided value |
425437
| tst.js:2:19:2:72 | /.*redi ... ref)[1] | tst.js:2:47:2:68 | documen ... on.href | tst.js:2:19:2:72 | /.*redi ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:2:47:2:68 | documen ... on.href | user-provided value |
426438
| tst.js:6:20:6:59 | indirec ... ref)[1] | tst.js:6:34:6:50 | document.location | tst.js:6:20:6:59 | indirec ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:6:34:6:50 | document.location | user-provided value |

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,5 +64,12 @@ function baz() {
6464
const history = require('history').createBrowserHistory();
6565
var payload = history.location.hash.substr(1);
6666

67+
history.replace(payload); // NOT OK
68+
}
69+
70+
function quz() {
71+
const history = HistoryLibrary.createBrowserHistory();
72+
var payload = history.location.hash.substr(1);
73+
6774
history.replace(payload); // NOT OK
6875
}

0 commit comments

Comments
 (0)