Skip to content

Commit ac533ea

Browse files
NapalysNapalys
authored
Merge pull request #19771 from Napalys/js/sanitizer_serialize
JS: Improve XSS detection for `serialize-javascript` with tainted objects
2 parents 909e95f + fc0c8a8 commit ac533ea

File tree

6 files changed

+68
-3
lines changed

6 files changed

+68
-3
lines changed

‎javascript/ql/lib/change-notes/2025-06-16-serialize-js.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 calls to `serialize-javascript`.

‎javascript/ql/lib/semmle/javascript/JsonParsers.qll

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,7 @@ private class PlainJsonParserCall extends JsonParserCall {
3333
callee = DataFlow::moduleImport("parse-json") or
3434
callee = DataFlow::moduleImport("json-parse-better-errors") or
3535
callee = DataFlow::moduleImport("json-safe-parse") or
36-
callee = AngularJS::angular().getAPropertyRead("fromJson") or
37-
callee = DataFlow::moduleImport("serialize-javascript")
36+
callee = AngularJS::angular().getAPropertyRead("fromJson")
3837
)
3938
}
4039

‎javascript/ql/lib/semmle/javascript/JsonStringifiers.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ class JsonStringifyCall extends DataFlow::CallNode {
2727
)
2828
or
2929
this = Templating::getAPipeCall(["json", "dump"])
30+
or
31+
this = DataFlow::moduleImport("serialize-javascript").getACall()
3032
}
3133

3234
/**

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,8 @@
7070
| tst2.js:76:12:76:18 | other.p | tst2.js:69:9:69:9 | p | tst2.js:76:12:76:18 | other.p | Cross-site scripting vulnerability due to a $@. | tst2.js:69:9:69:9 | p | user-provided value |
7171
| tst2.js:88:12:88:12 | p | tst2.js:82:9:82:9 | p | tst2.js:88:12:88:12 | p | Cross-site scripting vulnerability due to a $@. | tst2.js:82:9:82:9 | p | user-provided value |
7272
| tst2.js:89:12:89:18 | other.p | tst2.js:82:9:82:9 | p | tst2.js:89:12:89:18 | other.p | Cross-site scripting vulnerability due to a $@. | tst2.js:82:9:82:9 | p | user-provided value |
73+
| tst2.js:101:12:101:17 | unsafe | tst2.js:93:9:93:9 | p | tst2.js:101:12:101:17 | unsafe | Cross-site scripting vulnerability due to a $@. | tst2.js:93:9:93:9 | p | user-provided value |
74+
| tst2.js:113:12:113:17 | unsafe | tst2.js:105:9:105:9 | p | tst2.js:113:12:113:17 | unsafe | Cross-site scripting vulnerability due to a $@. | tst2.js:105:9:105:9 | p | user-provided value |
7375
| tst3.js:6:12:6:12 | p | tst3.js:5:9:5:9 | p | tst3.js:6:12:6:12 | p | Cross-site scripting vulnerability due to a $@. | tst3.js:5:9:5:9 | p | user-provided value |
7476
| tst3.js:12:12:12:15 | code | tst3.js:11:32:11:39 | reg.body | tst3.js:12:12:12:15 | code | Cross-site scripting vulnerability due to a $@. | tst3.js:11:32:11:39 | reg.body | user-provided value |
7577
edges
@@ -239,6 +241,22 @@ edges
239241
| tst2.js:86:15:86:27 | sortKeys(obj) [p] | tst2.js:86:7:86:27 | other [p] | provenance | |
240242
| tst2.js:86:24:86:26 | obj [p] | tst2.js:86:15:86:27 | sortKeys(obj) [p] | provenance | |
241243
| tst2.js:89:12:89:16 | other [p] | tst2.js:89:12:89:18 | other.p | provenance | |
244+
| tst2.js:93:7:93:24 | p | tst2.js:99:51:99:51 | p | provenance | |
245+
| tst2.js:93:9:93:9 | p | tst2.js:93:7:93:24 | p | provenance | |
246+
| tst2.js:99:7:99:69 | unsafe | tst2.js:101:12:101:17 | unsafe | provenance | |
247+
| tst2.js:99:16:99:69 | seriali ... true}) | tst2.js:99:7:99:69 | unsafe | provenance | |
248+
| tst2.js:99:36:99:52 | {someProperty: p} [someProperty] | tst2.js:99:16:99:69 | seriali ... true}) | provenance | |
249+
| tst2.js:99:51:99:51 | p | tst2.js:99:16:99:69 | seriali ... true}) | provenance | |
250+
| tst2.js:99:51:99:51 | p | tst2.js:99:36:99:52 | {someProperty: p} [someProperty] | provenance | |
251+
| tst2.js:105:7:105:24 | p | tst2.js:110:28:110:28 | p | provenance | |
252+
| tst2.js:105:9:105:9 | p | tst2.js:105:7:105:24 | p | provenance | |
253+
| tst2.js:110:7:110:29 | obj [someProperty] | tst2.js:111:36:111:38 | obj [someProperty] | provenance | |
254+
| tst2.js:110:13:110:29 | {someProperty: p} [someProperty] | tst2.js:110:7:110:29 | obj [someProperty] | provenance | |
255+
| tst2.js:110:28:110:28 | p | tst2.js:110:13:110:29 | {someProperty: p} [someProperty] | provenance | |
256+
| tst2.js:110:28:110:28 | p | tst2.js:111:16:111:55 | seriali ... true}) | provenance | |
257+
| tst2.js:111:7:111:55 | unsafe | tst2.js:113:12:113:17 | unsafe | provenance | |
258+
| tst2.js:111:16:111:55 | seriali ... true}) | tst2.js:111:7:111:55 | unsafe | provenance | |
259+
| tst2.js:111:36:111:38 | obj [someProperty] | tst2.js:111:16:111:55 | seriali ... true}) | provenance | |
242260
| tst3.js:5:7:5:24 | p | tst3.js:6:12:6:12 | p | provenance | |
243261
| tst3.js:5:9:5:9 | p | tst3.js:5:7:5:24 | p | provenance | |
244262
| tst3.js:11:9:11:74 | code | tst3.js:12:12:12:15 | code | provenance | |
@@ -457,6 +475,22 @@ nodes
457475
| tst2.js:88:12:88:12 | p | semmle.label | p |
458476
| tst2.js:89:12:89:16 | other [p] | semmle.label | other [p] |
459477
| tst2.js:89:12:89:18 | other.p | semmle.label | other.p |
478+
| tst2.js:93:7:93:24 | p | semmle.label | p |
479+
| tst2.js:93:9:93:9 | p | semmle.label | p |
480+
| tst2.js:99:7:99:69 | unsafe | semmle.label | unsafe |
481+
| tst2.js:99:16:99:69 | seriali ... true}) | semmle.label | seriali ... true}) |
482+
| tst2.js:99:36:99:52 | {someProperty: p} [someProperty] | semmle.label | {someProperty: p} [someProperty] |
483+
| tst2.js:99:51:99:51 | p | semmle.label | p |
484+
| tst2.js:101:12:101:17 | unsafe | semmle.label | unsafe |
485+
| tst2.js:105:7:105:24 | p | semmle.label | p |
486+
| tst2.js:105:9:105:9 | p | semmle.label | p |
487+
| tst2.js:110:7:110:29 | obj [someProperty] | semmle.label | obj [someProperty] |
488+
| tst2.js:110:13:110:29 | {someProperty: p} [someProperty] | semmle.label | {someProperty: p} [someProperty] |
489+
| tst2.js:110:28:110:28 | p | semmle.label | p |
490+
| tst2.js:111:7:111:55 | unsafe | semmle.label | unsafe |
491+
| tst2.js:111:16:111:55 | seriali ... true}) | semmle.label | seriali ... true}) |
492+
| tst2.js:111:36:111:38 | obj [someProperty] | semmle.label | obj [someProperty] |
493+
| tst2.js:113:12:113:17 | unsafe | semmle.label | unsafe |
460494
| tst3.js:5:7:5:24 | p | semmle.label | p |
461495
| tst3.js:5:9:5:9 | p | semmle.label | p |
462496
| tst3.js:6:12:6:12 | p | semmle.label | p |

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXssWithCustomSanitizer.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,5 +68,7 @@
6868
| tst2.js:76:12:76:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:69:9:69:9 | p | user-provided value |
6969
| tst2.js:88:12:88:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:82:9:82:9 | p | user-provided value |
7070
| tst2.js:89:12:89:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:82:9:82:9 | p | user-provided value |
71+
| tst2.js:101:12:101:17 | unsafe | Cross-site scripting vulnerability due to $@. | tst2.js:93:9:93:9 | p | user-provided value |
72+
| tst2.js:113:12:113:17 | unsafe | Cross-site scripting vulnerability due to $@. | tst2.js:105:9:105:9 | p | user-provided value |
7173
| tst3.js:6:12:6:12 | p | Cross-site scripting vulnerability due to $@. | tst3.js:5:9:5:9 | p | user-provided value |
7274
| tst3.js:12:12:12:15 | code | Cross-site scripting vulnerability due to $@. | tst3.js:11:32:11:39 | reg.body | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,4 +87,28 @@ app.get('/baz', function(req, res) {
8787

8888
res.send(p); // $ Alert
8989
res.send(other.p); // $ Alert
90-
});
90+
});
91+
92+
app.get('/baz', function(req, res) {
93+
let { p } = req.params; // $ Source
94+
95+
var serialized = serializeJavaScript(p);
96+
97+
res.send(serialized);
98+
99+
var unsafe = serializeJavaScript({someProperty: p}, {unsafe: true});
100+
101+
res.send(unsafe); // $ Alert
102+
});
103+
104+
app.get('/baz', function(req, res) {
105+
let { p } = req.params; // $ Source
106+
107+
var serialized = serializeJavaScript(p);
108+
109+
res.send(serialized);
110+
let obj = {someProperty: p};
111+
var unsafe = serializeJavaScript(obj, {unsafe: true});
112+
113+
res.send(unsafe); // $ Alert
114+
});

0 commit comments

Comments
 (0)