JS: Do not use legacy taint steps in TaintedUrlSuffix

Tainted URL suffix steps are added as configuration-specific additional steps, which means implicit reads may occur before any of these steps. These steps accidentally included the legacy taint steps which include a step from 'arguments' to all positional parameters. Combined with the implicit read, arguments could escape their array index and flow to any parameter while in the tainted-url flow state.
github · Aug 29, 2024 · 4568967 · 4568967
1 parent 65a36b0
commit 4568967
Show file tree

Hide file tree

Showing 4 changed files with 3 additions and 37 deletions.
diff --git a/javascript/ql/lib/semmle/javascript/security/TaintedUrlSuffix.qll b/javascript/ql/lib/semmle/javascript/security/TaintedUrlSuffix.qll
@@ -54,7 +54,7 @@ module TaintedUrlSuffix {
     // Inherit all ordinary taint steps except `x -> x.p` steps
     srclbl = label() and
     dstlbl = label() and
-    TaintTracking::sharedTaintStep(src, dst) and
+    TaintTracking::AdditionalTaintStep::step(src, dst) and
     not isSafeLocationProp(dst)
     or
     // Transition from URL suffix to full taint when extracting the query/fragment part.

diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
@@ -330,13 +330,8 @@ nodes
 | string-manipulations.js:9:36:9:57 | documen ... on.href | semmle.label | documen ... on.href |
 | string-manipulations.js:10:16:10:45 | String( ... n.href) | semmle.label | String( ... n.href) |
 | string-manipulations.js:10:23:10:44 | documen ... on.href | semmle.label | documen ... on.href |
-| tainted-url-suffix-arguments.js:3:1:8:1 | 'arguments' object of function foo [1] | semmle.label | 'arguments' object of function foo [1] |
-| tainted-url-suffix-arguments.js:3:14:3:14 | x | semmle.label | x |
 | tainted-url-suffix-arguments.js:3:17:3:17 | y | semmle.label | y |
-| tainted-url-suffix-arguments.js:3:20:3:20 | z | semmle.label | z |
-| tainted-url-suffix-arguments.js:5:22:5:22 | x | semmle.label | x |
 | tainted-url-suffix-arguments.js:6:22:6:22 | y | semmle.label | y |
-| tainted-url-suffix-arguments.js:7:22:7:22 | z | semmle.label | z |
 | tainted-url-suffix-arguments.js:11:11:11:36 | url | semmle.label | url |
 | tainted-url-suffix-arguments.js:11:17:11:36 | window.location.href | semmle.label | window.location.href |
 | tainted-url-suffix-arguments.js:12:17:12:19 | url | semmle.label | url |
@@ -426,7 +421,6 @@ nodes
 | tst.js:64:25:64:48 | documen ... .search | semmle.label | documen ... .search |
 | tst.js:65:25:65:48 | documen ... .search | semmle.label | documen ... .search |
 | tst.js:68:16:68:20 | bar() | semmle.label | bar() |
-| tst.js:70:1:70:27 | [,docum ... search] | semmle.label | [,docum ... search] |
 | tst.js:70:1:70:27 | [,docum ... search] [1] | semmle.label | [,docum ... search] [1] |
 | tst.js:70:3:70:26 | documen ... .search | semmle.label | documen ... .search |
 | tst.js:70:46:70:46 | x | semmle.label | x |
@@ -959,15 +953,9 @@ edges
 | string-manipulations.js:9:36:9:57 | documen ... on.href | string-manipulations.js:9:16:9:58 | String. ... n.href) | provenance | Config |
 | string-manipulations.js:10:23:10:44 | documen ... on.href | string-manipulations.js:10:16:10:45 | String( ... n.href) | provenance |  |
 | string-manipulations.js:10:23:10:44 | documen ... on.href | string-manipulations.js:10:16:10:45 | String( ... n.href) | provenance | Config |
-| tainted-url-suffix-arguments.js:3:1:8:1 | 'arguments' object of function foo [1] | tainted-url-suffix-arguments.js:3:14:3:14 | x | provenance | Config |
-| tainted-url-suffix-arguments.js:3:1:8:1 | 'arguments' object of function foo [1] | tainted-url-suffix-arguments.js:3:17:3:17 | y | provenance | Config |
-| tainted-url-suffix-arguments.js:3:1:8:1 | 'arguments' object of function foo [1] | tainted-url-suffix-arguments.js:3:20:3:20 | z | provenance | Config |
-| tainted-url-suffix-arguments.js:3:14:3:14 | x | tainted-url-suffix-arguments.js:5:22:5:22 | x | provenance |  |
 | tainted-url-suffix-arguments.js:3:17:3:17 | y | tainted-url-suffix-arguments.js:6:22:6:22 | y | provenance |  |
-| tainted-url-suffix-arguments.js:3:20:3:20 | z | tainted-url-suffix-arguments.js:7:22:7:22 | z | provenance |  |
 | tainted-url-suffix-arguments.js:11:11:11:36 | url | tainted-url-suffix-arguments.js:12:17:12:19 | url | provenance |  |
 | tainted-url-suffix-arguments.js:11:17:11:36 | window.location.href | tainted-url-suffix-arguments.js:11:11:11:36 | url | provenance |  |
-| tainted-url-suffix-arguments.js:12:17:12:19 | url | tainted-url-suffix-arguments.js:3:1:8:1 | 'arguments' object of function foo [1] | provenance |  |
 | tainted-url-suffix-arguments.js:12:17:12:19 | url | tainted-url-suffix-arguments.js:3:17:3:17 | y | provenance |  |
 | tooltip.jsx:6:11:6:30 | source | tooltip.jsx:10:25:10:30 | source | provenance |  |
 | tooltip.jsx:6:11:6:30 | source | tooltip.jsx:11:25:11:30 | source | provenance |  |
@@ -1053,11 +1041,7 @@ edges
 | tst.js:60:34:60:34 | s | tst.js:62:18:62:18 | s | provenance |  |
 | tst.js:64:25:64:48 | documen ... .search | tst.js:60:34:60:34 | s | provenance |  |
 | tst.js:65:25:65:48 | documen ... .search | tst.js:60:34:60:34 | s | provenance |  |
-| tst.js:70:1:70:27 | [,docum ... search] | tst.js:70:46:70:46 | x | provenance |  |
-| tst.js:70:1:70:27 | [,docum ... search] | tst.js:70:46:70:46 | x | provenance | Config |
 | tst.js:70:1:70:27 | [,docum ... search] [1] | tst.js:70:46:70:46 | x | provenance |  |
-| tst.js:70:1:70:27 | [,docum ... search] [1] | tst.js:70:46:70:46 | x | provenance | Config |
-| tst.js:70:3:70:26 | documen ... .search | tst.js:70:1:70:27 | [,docum ... search] | provenance | Config |
 | tst.js:70:3:70:26 | documen ... .search | tst.js:70:1:70:27 | [,docum ... search] [1] | provenance |  |
 | tst.js:70:46:70:46 | x | tst.js:73:20:73:20 | x | provenance |  |
 | tst.js:107:7:107:44 | v | tst.js:110:18:110:18 | v | provenance |  |
@@ -1398,9 +1382,7 @@ subpaths
 | string-manipulations.js:8:16:8:48 | documen ... mLeft() | string-manipulations.js:8:16:8:37 | documen ... on.href | string-manipulations.js:8:16:8:48 | documen ... mLeft() | Cross-site scripting vulnerability due to $@. | string-manipulations.js:8:16:8:37 | documen ... on.href | user-provided value |
 | string-manipulations.js:9:16:9:58 | String. ... n.href) | string-manipulations.js:9:36:9:57 | documen ... on.href | string-manipulations.js:9:16:9:58 | String. ... n.href) | Cross-site scripting vulnerability due to $@. | string-manipulations.js:9:36:9:57 | documen ... on.href | user-provided value |
 | string-manipulations.js:10:16:10:45 | String( ... n.href) | string-manipulations.js:10:23:10:44 | documen ... on.href | string-manipulations.js:10:16:10:45 | String( ... n.href) | Cross-site scripting vulnerability due to $@. | string-manipulations.js:10:23:10:44 | documen ... on.href | user-provided value |
-| tainted-url-suffix-arguments.js:5:22:5:22 | x | tainted-url-suffix-arguments.js:11:17:11:36 | window.location.href | tainted-url-suffix-arguments.js:5:22:5:22 | x | Cross-site scripting vulnerability due to $@. | tainted-url-suffix-arguments.js:11:17:11:36 | window.location.href | user-provided value |
 | tainted-url-suffix-arguments.js:6:22:6:22 | y | tainted-url-suffix-arguments.js:11:17:11:36 | window.location.href | tainted-url-suffix-arguments.js:6:22:6:22 | y | Cross-site scripting vulnerability due to $@. | tainted-url-suffix-arguments.js:11:17:11:36 | window.location.href | user-provided value |
-| tainted-url-suffix-arguments.js:7:22:7:22 | z | tainted-url-suffix-arguments.js:11:17:11:36 | window.location.href | tainted-url-suffix-arguments.js:7:22:7:22 | z | Cross-site scripting vulnerability due to $@. | tainted-url-suffix-arguments.js:11:17:11:36 | window.location.href | user-provided value |
 | tooltip.jsx:10:25:10:30 | source | tooltip.jsx:6:20:6:30 | window.name | tooltip.jsx:10:25:10:30 | source | Cross-site scripting vulnerability due to $@. | tooltip.jsx:6:20:6:30 | window.name | user-provided value |
 | tooltip.jsx:11:25:11:30 | source | tooltip.jsx:6:20:6:30 | window.name | tooltip.jsx:11:25:11:30 | source | Cross-site scripting vulnerability due to $@. | tooltip.jsx:6:20:6:30 | window.name | user-provided value |
 | tooltip.jsx:18:51:18:59 | provide() | tooltip.jsx:22:20:22:30 | window.name | tooltip.jsx:18:51:18:59 | provide() | Cross-site scripting vulnerability due to $@. | tooltip.jsx:22:20:22:30 | window.name | user-provided value |

diff --git a/...script/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected b/...script/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected
@@ -335,13 +335,8 @@ nodes
 | string-manipulations.js:9:36:9:57 | documen ... on.href | semmle.label | documen ... on.href |
 | string-manipulations.js:10:16:10:45 | String( ... n.href) | semmle.label | String( ... n.href) |
 | string-manipulations.js:10:23:10:44 | documen ... on.href | semmle.label | documen ... on.href |
-| tainted-url-suffix-arguments.js:3:1:8:1 | 'arguments' object of function foo [1] | semmle.label | 'arguments' object of function foo [1] |
-| tainted-url-suffix-arguments.js:3:14:3:14 | x | semmle.label | x |
 | tainted-url-suffix-arguments.js:3:17:3:17 | y | semmle.label | y |
-| tainted-url-suffix-arguments.js:3:20:3:20 | z | semmle.label | z |
-| tainted-url-suffix-arguments.js:5:22:5:22 | x | semmle.label | x |
 | tainted-url-suffix-arguments.js:6:22:6:22 | y | semmle.label | y |
-| tainted-url-suffix-arguments.js:7:22:7:22 | z | semmle.label | z |
 | tainted-url-suffix-arguments.js:11:11:11:36 | url | semmle.label | url |
 | tainted-url-suffix-arguments.js:11:17:11:36 | window.location.href | semmle.label | window.location.href |
 | tainted-url-suffix-arguments.js:12:17:12:19 | url | semmle.label | url |
@@ -431,7 +426,6 @@ nodes
 | tst.js:64:25:64:48 | documen ... .search | semmle.label | documen ... .search |
 | tst.js:65:25:65:48 | documen ... .search | semmle.label | documen ... .search |
 | tst.js:68:16:68:20 | bar() | semmle.label | bar() |
-| tst.js:70:1:70:27 | [,docum ... search] | semmle.label | [,docum ... search] |
 | tst.js:70:1:70:27 | [,docum ... search] [1] | semmle.label | [,docum ... search] [1] |
 | tst.js:70:3:70:26 | documen ... .search | semmle.label | documen ... .search |
 | tst.js:70:46:70:46 | x | semmle.label | x |
@@ -984,15 +978,9 @@ edges
 | string-manipulations.js:9:36:9:57 | documen ... on.href | string-manipulations.js:9:16:9:58 | String. ... n.href) | provenance | Config |
 | string-manipulations.js:10:23:10:44 | documen ... on.href | string-manipulations.js:10:16:10:45 | String( ... n.href) | provenance |  |
 | string-manipulations.js:10:23:10:44 | documen ... on.href | string-manipulations.js:10:16:10:45 | String( ... n.href) | provenance | Config |
-| tainted-url-suffix-arguments.js:3:1:8:1 | 'arguments' object of function foo [1] | tainted-url-suffix-arguments.js:3:14:3:14 | x | provenance | Config |
-| tainted-url-suffix-arguments.js:3:1:8:1 | 'arguments' object of function foo [1] | tainted-url-suffix-arguments.js:3:17:3:17 | y | provenance | Config |
-| tainted-url-suffix-arguments.js:3:1:8:1 | 'arguments' object of function foo [1] | tainted-url-suffix-arguments.js:3:20:3:20 | z | provenance | Config |
-| tainted-url-suffix-arguments.js:3:14:3:14 | x | tainted-url-suffix-arguments.js:5:22:5:22 | x | provenance |  |
 | tainted-url-suffix-arguments.js:3:17:3:17 | y | tainted-url-suffix-arguments.js:6:22:6:22 | y | provenance |  |
-| tainted-url-suffix-arguments.js:3:20:3:20 | z | tainted-url-suffix-arguments.js:7:22:7:22 | z | provenance |  |
 | tainted-url-suffix-arguments.js:11:11:11:36 | url | tainted-url-suffix-arguments.js:12:17:12:19 | url | provenance |  |
 | tainted-url-suffix-arguments.js:11:17:11:36 | window.location.href | tainted-url-suffix-arguments.js:11:11:11:36 | url | provenance |  |
-| tainted-url-suffix-arguments.js:12:17:12:19 | url | tainted-url-suffix-arguments.js:3:1:8:1 | 'arguments' object of function foo [1] | provenance |  |
 | tainted-url-suffix-arguments.js:12:17:12:19 | url | tainted-url-suffix-arguments.js:3:17:3:17 | y | provenance |  |
 | tooltip.jsx:6:11:6:30 | source | tooltip.jsx:10:25:10:30 | source | provenance |  |
 | tooltip.jsx:6:11:6:30 | source | tooltip.jsx:11:25:11:30 | source | provenance |  |
@@ -1078,11 +1066,7 @@ edges
 | tst.js:60:34:60:34 | s | tst.js:62:18:62:18 | s | provenance |  |
 | tst.js:64:25:64:48 | documen ... .search | tst.js:60:34:60:34 | s | provenance |  |
 | tst.js:65:25:65:48 | documen ... .search | tst.js:60:34:60:34 | s | provenance |  |
-| tst.js:70:1:70:27 | [,docum ... search] | tst.js:70:46:70:46 | x | provenance |  |
-| tst.js:70:1:70:27 | [,docum ... search] | tst.js:70:46:70:46 | x | provenance | Config |
 | tst.js:70:1:70:27 | [,docum ... search] [1] | tst.js:70:46:70:46 | x | provenance |  |
-| tst.js:70:1:70:27 | [,docum ... search] [1] | tst.js:70:46:70:46 | x | provenance | Config |
-| tst.js:70:3:70:26 | documen ... .search | tst.js:70:1:70:27 | [,docum ... search] | provenance | Config |
 | tst.js:70:3:70:26 | documen ... .search | tst.js:70:1:70:27 | [,docum ... search] [1] | provenance |  |
 | tst.js:70:46:70:46 | x | tst.js:73:20:73:20 | x | provenance |  |
 | tst.js:107:7:107:44 | v | tst.js:110:18:110:18 | v | provenance |  |

diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/tainted-url-suffix-arguments.js b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/tainted-url-suffix-arguments.js
@@ -2,9 +2,9 @@ import 'dummy';
 
 function foo(x, y, z) {
     arguments; // ensure 'arguments' are used
-    document.writeln(x); // OK [INCONSISTENCY]
+    document.writeln(x); // OK
     document.writeln(y); // NOT OK
-    document.writeln(z); // OK [INCONSISTENCY]
+    document.writeln(z); // OK
 }
 
 function bar() {