From 4568967a76ca7effed60693b001b2eff0ace89aa Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 29 Aug 2024 13:36:44 +0200 Subject: [PATCH] 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. --- .../javascript/security/TaintedUrlSuffix.qll | 2 +- .../Security/CWE-079/DomBasedXss/Xss.expected | 18 ------------------ .../XssWithAdditionalSources.expected | 16 ---------------- .../tainted-url-suffix-arguments.js | 4 ++-- 4 files changed, 3 insertions(+), 37 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/TaintedUrlSuffix.qll b/javascript/ql/lib/semmle/javascript/security/TaintedUrlSuffix.qll index 68a4f1c8d8ea..683c5e19f4d4 100644 --- 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 index 96aa177e4c22..69a9515da11b 100644 --- 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/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected index 4f908c6fe101..2523b19237ff 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected +++ b/javascript/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 index c853b1ebfbcf..a1feef0267a0 100644 --- 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() {