Merge pull request github#18733 from asgerf/js/query-string-parse-fn

JS: Model query-string parsers that strip off a leading '#' or '?'

Author: asgerf

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

@@ -141,6 +141,18 @@ module TaintedUrlSuffix {
         // If the regexp is unknown, assume it will extract the URL suffix
         not exists(re.getRoot())
       )
+      or
+      // Query-string parsers that strip off a leading '#' or '?'.
+      state1.isTaintedUrlSuffix() and
+      state2.isTaint() and
+      exists(DataFlow::CallNode call |
+        node1 = call.getArgument(0) and
+        node2 = call
+      |
+        call = API::moduleImport("query-string").getMember(["parse", "extract"]).getACall()
+        or
+        call = API::moduleImport("querystringify").getMember("parse").getACall()
+      )
     )
   }
 

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

@@ -147,6 +147,14 @@ nodes
 | tst15.js:15:23:15:28 | search | semmle.label | search |
 | tst15.js:15:23:15:66 | search. ... ', 10)) | semmle.label | search. ... ', 10)) |
 | tst15.js:15:23:15:79 | search. ... ring(1) | semmle.label | search. ... ring(1) |
+| tst16.js:5:21:5:54 | querySt ... search) | semmle.label | querySt ... search) |
+| tst16.js:5:21:5:59 | querySt ... h).data | semmle.label | querySt ... h).data |
+| tst16.js:5:39:5:53 | location.search | semmle.label | location.search |
+| tst16.js:6:21:6:56 | querySt ... search) | semmle.label | querySt ... search) |
+| tst16.js:6:41:6:55 | location.search | semmle.label | location.search |
+| tst16.js:7:21:7:57 | queryst ... search) | semmle.label | queryst ... search) |
+| tst16.js:7:21:7:62 | queryst ... h).data | semmle.label | queryst ... h).data |
+| tst16.js:7:42:7:56 | location.search | semmle.label | location.search |
 | tst.js:2:19:2:69 | /.*redi ... n.href) | semmle.label | /.*redi ... n.href) |
 | tst.js:2:19:2:72 | /.*redi ... ref)[1] | semmle.label | /.*redi ... ref)[1] |
 | tst.js:2:47:2:68 | documen ... on.href | semmle.label | documen ... on.href |
@@ -303,6 +311,11 @@ edges
 | tst15.js:14:23:14:45 | search. ... (0, 10) | tst15.js:14:23:14:58 | search. ... ring(1) | provenance | Config |
 | tst15.js:15:23:15:28 | search | tst15.js:15:23:15:66 | search. ... ', 10)) | provenance |  |
 | tst15.js:15:23:15:66 | search. ... ', 10)) | tst15.js:15:23:15:79 | search. ... ring(1) | provenance | Config |
+| tst16.js:5:21:5:54 | querySt ... search) | tst16.js:5:21:5:59 | querySt ... h).data | provenance |  |
+| tst16.js:5:39:5:53 | location.search | tst16.js:5:21:5:54 | querySt ... search) | provenance | Config |
+| tst16.js:6:41:6:55 | location.search | tst16.js:6:21:6:56 | querySt ... search) | provenance | Config |
+| tst16.js:7:21:7:57 | queryst ... search) | tst16.js:7:21:7:62 | queryst ... h).data | provenance |  |
+| tst16.js:7:42:7:56 | location.search | tst16.js:7:21:7:57 | queryst ... search) | provenance | Config |
 | tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] | provenance |  |
 | tst.js:2:47:2:68 | documen ... on.href | tst.js:2:19:2:69 | /.*redi ... n.href) | provenance | Config |
 | tst.js:6:20:6:56 | indirec ... n.href) | tst.js:6:20:6:59 | indirec ... ref)[1] | provenance |  |
@@ -392,6 +405,9 @@ subpaths
 | tst15.js:13:23:13:54 | search. ... ring(1) | tst15.js:12:18:12:41 | documen ... .search | tst15.js:13:23:13:54 | search. ... ring(1) | Untrusted URL redirection depends on a $@. | tst15.js:12:18:12:41 | documen ... .search | user-provided value |
 | tst15.js:14:23:14:58 | search. ... ring(1) | tst15.js:12:18:12:41 | documen ... .search | tst15.js:14:23:14:58 | search. ... ring(1) | Untrusted URL redirection depends on a $@. | tst15.js:12:18:12:41 | documen ... .search | user-provided value |
 | tst15.js:15:23:15:79 | search. ... ring(1) | tst15.js:12:18:12:41 | documen ... .search | tst15.js:15:23:15:79 | search. ... ring(1) | Untrusted URL redirection depends on a $@. | tst15.js:12:18:12:41 | documen ... .search | user-provided value |
+| tst16.js:5:21:5:59 | querySt ... h).data | tst16.js:5:39:5:53 | location.search | tst16.js:5:21:5:59 | querySt ... h).data | Untrusted URL redirection depends on a $@. | tst16.js:5:39:5:53 | location.search | user-provided value |
+| tst16.js:6:21:6:56 | querySt ... search) | tst16.js:6:41:6:55 | location.search | tst16.js:6:21:6:56 | querySt ... search) | Untrusted URL redirection depends on a $@. | tst16.js:6:41:6:55 | location.search | user-provided value |
+| tst16.js:7:21:7:62 | queryst ... h).data | tst16.js:7:42:7:56 | location.search | tst16.js:7:21:7:62 | queryst ... h).data | Untrusted URL redirection depends on a $@. | tst16.js:7:42:7:56 | location.search | user-provided value |
 | 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 depends on a $@. | tst.js:2:47:2:68 | documen ... on.href | user-provided value |
 | 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 |
 | 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 |

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

@@ -0,0 +1,8 @@
+import queryString from 'query-string';
+import querystringify from 'querystringify';
+
+function foo() {
+    location.href = queryString.parse(location.search).data; // NOT OK
+    location.href = queryString.extract(location.search); // NOT OK
+    location.href = querystringify.parse(location.search).data; // NOT OK
+}