Skip to content

Commit 43174cf

Browse files
asgerfasgerf
authored
Merge pull request github#12668 from asgerf/js/jquery-callback-sinks
JS: fix handling of jQuery sinks involving callback
2 parents 7729a6b + 61a7ee9 commit 43174cf

File tree

5 files changed

+38
-0
lines changed

5 files changed

+38
-0
lines changed

‎javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,14 @@ class Configuration extends TaintTracking::Configuration {
122122
TaintedUrlSuffix::step(src, trg, TaintedUrlSuffix::label(), DataFlow::FlowLabel::taint()) and
123123
inlbl = TaintedUrlSuffix::label() and
124124
outlbl = prefixLabel()
125+
or
126+
exists(DataFlow::FunctionNode callback, DataFlow::Node arg |
127+
any(JQuery::MethodCall c).interpretsArgumentAsHtml(arg) and
128+
callback = arg.getABoundFunctionValue(_) and
129+
src = callback.getReturnNode() and
130+
trg = callback and
131+
inlbl = outlbl
132+
)
125133
}
126134
}
127135

‎javascript/ql/src/change-notes/2023-03-27-jquery-callback-sinks.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Improved the model of jQuery to account for XSS sinks where the HTML string
5+
is provided via a callback. This may lead to more results for the `js/xss` query.

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,11 @@ nodes
431431
| jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
432432
| jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
433433
| jquery.js:34:13:34:16 | hash |
434+
| jquery.js:36:25:36:31 | tainted |
435+
| jquery.js:36:25:36:31 | tainted |
436+
| jquery.js:37:25:37:37 | () => tainted |
437+
| jquery.js:37:25:37:37 | () => tainted |
438+
| jquery.js:37:31:37:37 | tainted |
434439
| json-stringify.jsx:5:9:5:36 | locale |
435440
| json-stringify.jsx:5:9:5:36 | locale |
436441
| json-stringify.jsx:5:18:5:36 | req.param("locale") |
@@ -1512,6 +1517,9 @@ edges
15121517
| express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") |
15131518
| jquery.js:2:7:2:40 | tainted | jquery.js:7:20:7:26 | tainted |
15141519
| jquery.js:2:7:2:40 | tainted | jquery.js:8:28:8:34 | tainted |
1520+
| jquery.js:2:7:2:40 | tainted | jquery.js:36:25:36:31 | tainted |
1521+
| jquery.js:2:7:2:40 | tainted | jquery.js:36:25:36:31 | tainted |
1522+
| jquery.js:2:7:2:40 | tainted | jquery.js:37:31:37:37 | tainted |
15151523
| jquery.js:2:17:2:40 | documen ... .search | jquery.js:2:7:2:40 | tainted |
15161524
| jquery.js:2:17:2:40 | documen ... .search | jquery.js:2:7:2:40 | tainted |
15171525
| jquery.js:7:20:7:26 | tainted | jquery.js:7:5:7:34 | "<div i ... + "\\">" |
@@ -1565,6 +1573,8 @@ edges
15651573
| jquery.js:28:5:28:26 | window. ... .search | jquery.js:28:5:28:43 | window. ... ?', '') |
15661574
| jquery.js:34:13:34:16 | hash | jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
15671575
| jquery.js:34:13:34:16 | hash | jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
1576+
| jquery.js:37:31:37:37 | tainted | jquery.js:37:25:37:37 | () => tainted |
1577+
| jquery.js:37:31:37:37 | tainted | jquery.js:37:25:37:37 | () => tainted |
15681578
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:11:51:11:56 | locale |
15691579
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:19:56:19:61 | locale |
15701580
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:31:55:31:60 | locale |
@@ -2355,6 +2365,8 @@ edges
23552365
| jquery.js:27:5:27:25 | hash.re ... #', '') | jquery.js:18:14:18:33 | window.location.hash | jquery.js:27:5:27:25 | hash.re ... #', '') | Cross-site scripting vulnerability due to $@. | jquery.js:18:14:18:33 | window.location.hash | user-provided value |
23562366
| jquery.js:28:5:28:43 | window. ... ?', '') | jquery.js:28:5:28:26 | window. ... .search | jquery.js:28:5:28:43 | window. ... ?', '') | Cross-site scripting vulnerability due to $@. | jquery.js:28:5:28:26 | window. ... .search | user-provided value |
23572367
| jquery.js:34:5:34:25 | '<b>' + ... '</b>' | jquery.js:18:14:18:33 | window.location.hash | jquery.js:34:5:34:25 | '<b>' + ... '</b>' | Cross-site scripting vulnerability due to $@. | jquery.js:18:14:18:33 | window.location.hash | user-provided value |
2368+
| jquery.js:36:25:36:31 | tainted | jquery.js:2:17:2:40 | documen ... .search | jquery.js:36:25:36:31 | tainted | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:40 | documen ... .search | user-provided value |
2369+
| jquery.js:37:25:37:37 | () => tainted | jquery.js:2:17:2:40 | documen ... .search | jquery.js:37:25:37:37 | () => tainted | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:40 | documen ... .search | user-provided value |
23582370
| json-stringify.jsx:31:40:31:61 | JSON.st ... locale) | json-stringify.jsx:5:18:5:36 | req.param("locale") | json-stringify.jsx:31:40:31:61 | JSON.st ... locale) | Cross-site scripting vulnerability due to $@. | json-stringify.jsx:5:18:5:36 | req.param("locale") | user-provided value |
23592371
| json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) | json-stringify.jsx:5:18:5:36 | req.param("locale") | json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) | Cross-site scripting vulnerability due to $@. | json-stringify.jsx:5:18:5:36 | req.param("locale") | user-provided value |
23602372
| jwt-server.js:11:19:11:29 | decoded.foo | jwt-server.js:7:17:7:35 | req.param("wobble") | jwt-server.js:11:19:11:29 | decoded.foo | Cross-site scripting vulnerability due to $@. | jwt-server.js:7:17:7:35 | req.param("wobble") | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,11 @@ nodes
431431
| jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
432432
| jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
433433
| jquery.js:34:13:34:16 | hash |
434+
| jquery.js:36:25:36:31 | tainted |
435+
| jquery.js:36:25:36:31 | tainted |
436+
| jquery.js:37:25:37:37 | () => tainted |
437+
| jquery.js:37:25:37:37 | () => tainted |
438+
| jquery.js:37:31:37:37 | tainted |
434439
| json-stringify.jsx:5:9:5:36 | locale |
435440
| json-stringify.jsx:5:9:5:36 | locale |
436441
| json-stringify.jsx:5:18:5:36 | req.param("locale") |
@@ -1562,6 +1567,9 @@ edges
15621567
| express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") |
15631568
| jquery.js:2:7:2:40 | tainted | jquery.js:7:20:7:26 | tainted |
15641569
| jquery.js:2:7:2:40 | tainted | jquery.js:8:28:8:34 | tainted |
1570+
| jquery.js:2:7:2:40 | tainted | jquery.js:36:25:36:31 | tainted |
1571+
| jquery.js:2:7:2:40 | tainted | jquery.js:36:25:36:31 | tainted |
1572+
| jquery.js:2:7:2:40 | tainted | jquery.js:37:31:37:37 | tainted |
15651573
| jquery.js:2:17:2:40 | documen ... .search | jquery.js:2:7:2:40 | tainted |
15661574
| jquery.js:2:17:2:40 | documen ... .search | jquery.js:2:7:2:40 | tainted |
15671575
| jquery.js:7:20:7:26 | tainted | jquery.js:7:5:7:34 | "<div i ... + "\\">" |
@@ -1615,6 +1623,8 @@ edges
16151623
| jquery.js:28:5:28:26 | window. ... .search | jquery.js:28:5:28:43 | window. ... ?', '') |
16161624
| jquery.js:34:13:34:16 | hash | jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
16171625
| jquery.js:34:13:34:16 | hash | jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
1626+
| jquery.js:37:31:37:37 | tainted | jquery.js:37:25:37:37 | () => tainted |
1627+
| jquery.js:37:31:37:37 | tainted | jquery.js:37:25:37:37 | () => tainted |
16181628
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:11:51:11:56 | locale |
16191629
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:19:56:19:61 | locale |
16201630
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:31:55:31:60 | locale |

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/jquery.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,4 +32,7 @@ function test() {
3232
$(hash + 'blah'); // OK
3333
$('blah' + hash); // OK - does not start with '<'
3434
$('<b>' + hash + '</b>'); // NOT OK
35+
36+
$('#foo').replaceWith(tainted); // NOT OK
37+
$('#foo').replaceWith(() => tainted); // NOT OK
3538
}

0 commit comments

Comments
 (0)