Skip to content

Commit 9bbbece

Browse files
asgerfasgerf
authored
Merge pull request github#10670 from tyage/property-stringify
JS: Improve detection of XSS when JSON.stringify()
2 parents 5756a33 + b1a165e commit 9bbbece

File tree

5 files changed

+135
-1
lines changed

5 files changed

+135
-1
lines changed

‎javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -711,13 +711,31 @@ module TaintTracking {
711711
}
712712
}
713713

714+
/**
715+
* Gets a local source of any part of the input to the given stringification `call`.
716+
*/
717+
pragma[nomagic]
718+
private DataFlow::Node getAJsonLocalInput(JsonStringifyCall call) {
719+
result = call.getInput()
720+
or
721+
exists(DataFlow::SourceNode source |
722+
source = pragma[only_bind_out](getAJsonLocalInput(call)).getALocalSource()
723+
|
724+
result = source.getAPropertyWrite().getRhs()
725+
or
726+
result = source.(DataFlow::ObjectLiteralNode).getASpreadProperty()
727+
or
728+
result = source.(DataFlow::ArrayCreationNode).getASpreadArgument()
729+
)
730+
}
731+
714732
/**
715733
* A taint propagating data flow edge arising from JSON unparsing.
716734
*/
717735
private class JsonStringifyTaintStep extends SharedTaintStep {
718736
override predicate serializeStep(DataFlow::Node pred, DataFlow::Node succ) {
719737
exists(JsonStringifyCall call |
720-
pred = call.getArgument(0) and
738+
pred = getAJsonLocalInput(call) and
721739
succ = call
722740
)
723741
}

‎javascript/ql/src/change-notes/2022-10-04-json-stringify-improvement.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Improved taint tracking through `JSON.stringify` in cases where a tainted value is stored somewhere in the input object.

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,22 @@ 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+
| json-stringify.jsx:5:9:5:36 | locale |
435+
| json-stringify.jsx:5:9:5:36 | locale |
436+
| json-stringify.jsx:5:18:5:36 | req.param("locale") |
437+
| json-stringify.jsx:5:18:5:36 | req.param("locale") |
438+
| json-stringify.jsx:5:18:5:36 | req.param("locale") |
439+
| json-stringify.jsx:11:16:11:58 | `https: ... ocale}` |
440+
| json-stringify.jsx:11:51:11:56 | locale |
441+
| json-stringify.jsx:19:16:19:63 | `https: ... ocale}` |
442+
| json-stringify.jsx:19:56:19:61 | locale |
443+
| json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
444+
| json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
445+
| json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
446+
| json-stringify.jsx:31:55:31:60 | locale |
447+
| json-stringify.jsx:31:55:31:60 | locale |
448+
| json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
449+
| json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
434450
| jwt-server.js:7:9:7:35 | taint |
435451
| jwt-server.js:7:9:7:35 | taint |
436452
| jwt-server.js:7:17:7:35 | req.param("wobble") |
@@ -1509,6 +1525,24 @@ edges
15091525
| jquery.js:28:5:28:26 | window. ... .search | jquery.js:28:5:28:43 | window. ... ?', '') |
15101526
| jquery.js:34:13:34:16 | hash | jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
15111527
| jquery.js:34:13:34:16 | hash | jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
1528+
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:11:51:11:56 | locale |
1529+
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:19:56:19:61 | locale |
1530+
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:31:55:31:60 | locale |
1531+
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:31:55:31:60 | locale |
1532+
| json-stringify.jsx:5:18:5:36 | req.param("locale") | json-stringify.jsx:5:9:5:36 | locale |
1533+
| json-stringify.jsx:5:18:5:36 | req.param("locale") | json-stringify.jsx:5:9:5:36 | locale |
1534+
| json-stringify.jsx:5:18:5:36 | req.param("locale") | json-stringify.jsx:5:9:5:36 | locale |
1535+
| json-stringify.jsx:5:18:5:36 | req.param("locale") | json-stringify.jsx:5:9:5:36 | locale |
1536+
| json-stringify.jsx:11:16:11:58 | `https: ... ocale}` | json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
1537+
| json-stringify.jsx:11:16:11:58 | `https: ... ocale}` | json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
1538+
| json-stringify.jsx:11:51:11:56 | locale | json-stringify.jsx:11:16:11:58 | `https: ... ocale}` |
1539+
| json-stringify.jsx:19:16:19:63 | `https: ... ocale}` | json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
1540+
| json-stringify.jsx:19:16:19:63 | `https: ... ocale}` | json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
1541+
| json-stringify.jsx:19:56:19:61 | locale | json-stringify.jsx:19:16:19:63 | `https: ... ocale}` |
1542+
| json-stringify.jsx:31:55:31:60 | locale | json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
1543+
| json-stringify.jsx:31:55:31:60 | locale | json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
1544+
| json-stringify.jsx:31:55:31:60 | locale | json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
1545+
| json-stringify.jsx:31:55:31:60 | locale | json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
15121546
| jwt-server.js:7:9:7:35 | taint | jwt-server.js:9:16:9:20 | taint |
15131547
| jwt-server.js:7:9:7:35 | taint | jwt-server.js:9:16:9:20 | taint |
15141548
| jwt-server.js:7:17:7:35 | req.param("wobble") | jwt-server.js:7:9:7:35 | taint |
@@ -2241,6 +2275,8 @@ edges
22412275
| 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 |
22422276
| 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 |
22432277
| 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 |
2278+
| 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 |
2279+
| 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 |
22442280
| 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 |
22452281
| nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` | nodemailer.js:13:50:13:66 | req.query.message | nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` | HTML injection vulnerability due to $@. | nodemailer.js:13:50:13:66 | req.query.message | user-provided value |
22462282
| optionalSanitizer.js:6:18:6:23 | target | optionalSanitizer.js:2:16:2:39 | documen ... .search | optionalSanitizer.js:6:18:6:23 | target | Cross-site scripting vulnerability due to $@. | optionalSanitizer.js:2:16:2:39 | documen ... .search | user-provided value |

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,22 @@ 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+
| json-stringify.jsx:5:9:5:36 | locale |
435+
| json-stringify.jsx:5:9:5:36 | locale |
436+
| json-stringify.jsx:5:18:5:36 | req.param("locale") |
437+
| json-stringify.jsx:5:18:5:36 | req.param("locale") |
438+
| json-stringify.jsx:5:18:5:36 | req.param("locale") |
439+
| json-stringify.jsx:11:16:11:58 | `https: ... ocale}` |
440+
| json-stringify.jsx:11:51:11:56 | locale |
441+
| json-stringify.jsx:19:16:19:63 | `https: ... ocale}` |
442+
| json-stringify.jsx:19:56:19:61 | locale |
443+
| json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
444+
| json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
445+
| json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
446+
| json-stringify.jsx:31:55:31:60 | locale |
447+
| json-stringify.jsx:31:55:31:60 | locale |
448+
| json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
449+
| json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
434450
| jwt-server.js:7:9:7:35 | taint |
435451
| jwt-server.js:7:9:7:35 | taint |
436452
| jwt-server.js:7:17:7:35 | req.param("wobble") |
@@ -1559,6 +1575,24 @@ edges
15591575
| jquery.js:28:5:28:26 | window. ... .search | jquery.js:28:5:28:43 | window. ... ?', '') |
15601576
| jquery.js:34:13:34:16 | hash | jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
15611577
| jquery.js:34:13:34:16 | hash | jquery.js:34:5:34:25 | '<b>' + ... '</b>' |
1578+
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:11:51:11:56 | locale |
1579+
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:19:56:19:61 | locale |
1580+
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:31:55:31:60 | locale |
1581+
| json-stringify.jsx:5:9:5:36 | locale | json-stringify.jsx:31:55:31:60 | locale |
1582+
| json-stringify.jsx:5:18:5:36 | req.param("locale") | json-stringify.jsx:5:9:5:36 | locale |
1583+
| json-stringify.jsx:5:18:5:36 | req.param("locale") | json-stringify.jsx:5:9:5:36 | locale |
1584+
| json-stringify.jsx:5:18:5:36 | req.param("locale") | json-stringify.jsx:5:9:5:36 | locale |
1585+
| json-stringify.jsx:5:18:5:36 | req.param("locale") | json-stringify.jsx:5:9:5:36 | locale |
1586+
| json-stringify.jsx:11:16:11:58 | `https: ... ocale}` | json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
1587+
| json-stringify.jsx:11:16:11:58 | `https: ... ocale}` | json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
1588+
| json-stringify.jsx:11:51:11:56 | locale | json-stringify.jsx:11:16:11:58 | `https: ... ocale}` |
1589+
| json-stringify.jsx:19:16:19:63 | `https: ... ocale}` | json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
1590+
| json-stringify.jsx:19:16:19:63 | `https: ... ocale}` | json-stringify.jsx:35:40:35:61 | JSON.st ... jsonLD) |
1591+
| json-stringify.jsx:19:56:19:61 | locale | json-stringify.jsx:19:16:19:63 | `https: ... ocale}` |
1592+
| json-stringify.jsx:31:55:31:60 | locale | json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
1593+
| json-stringify.jsx:31:55:31:60 | locale | json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
1594+
| json-stringify.jsx:31:55:31:60 | locale | json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
1595+
| json-stringify.jsx:31:55:31:60 | locale | json-stringify.jsx:31:40:31:61 | JSON.st ... locale) |
15621596
| jwt-server.js:7:9:7:35 | taint | jwt-server.js:9:16:9:20 | taint |
15631597
| jwt-server.js:7:9:7:35 | taint | jwt-server.js:9:16:9:20 | taint |
15641598
| jwt-server.js:7:17:7:35 | req.param("wobble") | jwt-server.js:7:9:7:35 | taint |

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/json-stringify.jsx

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
var express = require("express");
2+
var app = express();
3+
4+
app.get("/some/path", function (req, res) {
5+
const locale = req.param("locale");
6+
const breadcrumbList = [
7+
{
8+
"@type": "ListItem",
9+
position: 1,
10+
item: {
11+
"@id": `https://example.com/some?locale=${locale}`,
12+
name: "Some",
13+
},
14+
},
15+
{
16+
"@type": "ListItem",
17+
position: 2,
18+
item: {
19+
"@id": `https://example.com/some/path?locale=${locale}`,
20+
name: "Path",
21+
},
22+
},
23+
];
24+
const jsonLD = {
25+
"@context": "https://schema.org",
26+
"@type": "BreadcrumbList",
27+
itemListElement: breadcrumbList,
28+
};
29+
<script
30+
type="application/ld+json"
31+
dangerouslySetInnerHTML={{ __html: JSON.stringify(locale) }} // NOT OK
32+
/>;
33+
<script
34+
type="application/ld+json"
35+
dangerouslySetInnerHTML={{ __html: JSON.stringify(jsonLD) }} // NOT OK
36+
/>;
37+
<script
38+
type="application/ld+json"
39+
dangerouslySetInnerHTML={{ __html: JSON.stringify({}) }} // OK
40+
/>;
41+
<script type="application/ld+json">{ JSON.stringify(jsonLD) }</script> // OK
42+
});

0 commit comments

Comments
 (0)