Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

JS: Improve handling of spread arguments and rest parameters [shared data flow branch] #17213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 37 commits into from
Sep 9, 2024
Merged

JS: Improve handling of spread arguments and rest parameters [shared data flow branch] #17213

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
4cdaccd
JS: Add InlineFlowTest
asgerf Aug 7, 2024
5d77c33
Test case for spread and rest args/params
asgerf Aug 8, 2024
6c7d745
JS: Add nodes for static/dynamic argument/parameter arrays
asgerf Aug 9, 2024
a72f795
JS: Add corresponding argument positions
asgerf Aug 9, 2024
623dbda
Do not pass regular positional args into the rest parameter
asgerf Aug 12, 2024
fa7ad03
JS: Add store/load steps for the new argument arrays
asgerf Aug 9, 2024
ed33a6e
JS: Add explicit model of .join()
asgerf Aug 12, 2024
bbb1c8c
Remove old arguments-array position
asgerf Aug 12, 2024
60c3d07
Update DataFlowImplConsistency.qll
asgerf Aug 12, 2024
c04f0be
Update DataFlowConsistency.expected
asgerf Aug 12, 2024
5c7e623
JS: Add some tests for missing handling of dynamic args in flow summa…
asgerf Aug 12, 2024
53a2a66
Add new nodes to early stage
asgerf Aug 12, 2024
acdc896
JS: Support for dynamic args to flow summaries
asgerf Aug 12, 2024
6a08313
JS: Hide some nodes
asgerf Aug 12, 2024
079a622
JS: Add tests showing missing taint flow
asgerf Aug 14, 2024
895cb87
JS: Add taint into dynamic argument array
asgerf Aug 14, 2024
5084d02
Update tests.expected
asgerf Aug 14, 2024
ac1dd18
JS: Remove taint step from array element to whole array
asgerf Aug 14, 2024
34e6864
JS: Note issue with .apply() calls
asgerf Aug 14, 2024
4389b5c
JS: Fix issue for .apply() calls
asgerf Aug 14, 2024
4e7bd9d
JS: Update Arrays test now that array elements do not taint the whole…
asgerf Aug 19, 2024
df42e7c
JS: Add test showing lack of implicit reads for ArrayElement
asgerf Aug 19, 2024
371f7ef
JS: Add implicit taint read of array elements
asgerf Aug 19, 2024
aa8bd33
JS: Add a few more tests
asgerf Aug 22, 2024
3e196f8
JS: Update Promises/flow2 test
asgerf Aug 23, 2024
2e2181b
JS: Update test output that only affects nodes/edges/subpaths
asgerf Aug 27, 2024
837a8be
JS: Update test output and add related TODO in 'markdown-table' model
asgerf Aug 27, 2024
a2d53c2
JS: Update test output and add related TODO in model of 'async'
asgerf Aug 26, 2024
cb5dbb9
JS: Update test to reflect implicit read flow has been fixed
asgerf Aug 27, 2024
f65879e
JS: Update a test that no longer fails
asgerf Aug 27, 2024
65a36b0
JS: Add regression test for argument position confusion
asgerf Aug 29, 2024
4568967
JS: Do not use legacy taint steps in TaintedUrlSuffix
asgerf Aug 29, 2024
92bb4b3
JS: Address some comments from hvitved
asgerf Sep 5, 2024
379c7ef
JS: Add test to show lack of unknown array element being propagated
asgerf Sep 5, 2024
a9a8351
JS: Fix one case of missing handling of unknown array index
asgerf Sep 5, 2024
1da68aa
JS: Benign test output change
asgerf Sep 5, 2024
fb9732a
JS: Add another test and TODO about an issue with constant array indices
asgerf Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
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.
  • Loading branch information
@asgerf
asgerf committed Aug 29, 2024
commit 4568967a76ca7effed60693b001b2eff0ace89aa
2 changes: 1 addition & 1 deletion 2 javascript/ql/lib/semmle/javascript/security/TaintedUrlSuffix.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
18 changes: 0 additions & 18 deletions 18 javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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 | |
Expand Down Expand Up @@ -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 | |
Expand Down Expand Up @@ -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 |
Expand Down
16 changes: 0 additions & 16 deletions 16 ...script/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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 | |
Expand Down Expand Up @@ -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 | |
Expand Down
4 changes: 2 additions & 2 deletions 4 javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/tainted-url-suffix-arguments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Loading
Morty Proxy This is a proxified and sanitized view of the page, visit original site.