From a96ea182c7cf72c9209a34e95826ce1c4d3969c4 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Mon, 16 Jun 2025 09:30:40 +0200 Subject: [PATCH 1/5] JS: add test cases for `serialize-javascript` with tainted object properties --- .../Security/CWE-079/ReflectedXss/tst2.js | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js index 660743338848..20149bea1e76 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js @@ -87,4 +87,28 @@ app.get('/baz', function(req, res) { res.send(p); // $ Alert res.send(other.p); // $ Alert -}); \ No newline at end of file +}); + +app.get('/baz', function(req, res) { + let { p } = req.params; // $ MISSING: Source + + var serialized = serializeJavaScript(p); + + res.send(serialized); + + var unsafe = serializeJavaScript({someProperty: p}, {unsafe: true}); + + res.send(unsafe); // $ MISSING: Alert +}); + +app.get('/baz', function(req, res) { + let { p } = req.params; // $ MISSING: Source + + var serialized = serializeJavaScript(p); + + res.send(serialized); + let obj = {someProperty: p}; + var unsafe = serializeJavaScript(obj, {unsafe: true}); + + res.send(unsafe); // $ MISSING: Alert +}); From 5a107ec33b45788dbf70e4c383d30bd9e311cc47 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Mon, 16 Jun 2025 09:32:55 +0200 Subject: [PATCH 2/5] JS: track taint through `serialize-javascript` calls with object arguments --- .../semmle/javascript/JsonStringifiers.qll | 2 ++ .../ReflectedXss/ReflectedXss.expected | 34 +++++++++++++++++++ .../ReflectedXssWithCustomSanitizer.expected | 2 ++ .../Security/CWE-079/ReflectedXss/tst2.js | 8 ++--- 4 files changed, 42 insertions(+), 4 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/JsonStringifiers.qll b/javascript/ql/lib/semmle/javascript/JsonStringifiers.qll index f573c1364378..bb1b23610c94 100644 --- a/javascript/ql/lib/semmle/javascript/JsonStringifiers.qll +++ b/javascript/ql/lib/semmle/javascript/JsonStringifiers.qll @@ -27,6 +27,8 @@ class JsonStringifyCall extends DataFlow::CallNode { ) or this = Templating::getAPipeCall(["json", "dump"]) + or + this = DataFlow::moduleImport("serialize-javascript").getACall() } /** diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.expected index 09874ecef104..c1e626a688a9 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.expected @@ -70,6 +70,8 @@ | 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 | | 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 | | 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 | +| 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 | +| 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 | | 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 | | 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 | edges @@ -239,6 +241,22 @@ edges | tst2.js:86:15:86:27 | sortKeys(obj) [p] | tst2.js:86:7:86:27 | other [p] | provenance | | | tst2.js:86:24:86:26 | obj [p] | tst2.js:86:15:86:27 | sortKeys(obj) [p] | provenance | | | tst2.js:89:12:89:16 | other [p] | tst2.js:89:12:89:18 | other.p | provenance | | +| tst2.js:93:7:93:24 | p | tst2.js:99:51:99:51 | p | provenance | | +| tst2.js:93:9:93:9 | p | tst2.js:93:7:93:24 | p | provenance | | +| tst2.js:99:7:99:69 | unsafe | tst2.js:101:12:101:17 | unsafe | provenance | | +| tst2.js:99:16:99:69 | seriali ... true}) | tst2.js:99:7:99:69 | unsafe | provenance | | +| tst2.js:99:36:99:52 | {someProperty: p} [someProperty] | tst2.js:99:16:99:69 | seriali ... true}) | provenance | | +| tst2.js:99:51:99:51 | p | tst2.js:99:16:99:69 | seriali ... true}) | provenance | | +| tst2.js:99:51:99:51 | p | tst2.js:99:36:99:52 | {someProperty: p} [someProperty] | provenance | | +| tst2.js:105:7:105:24 | p | tst2.js:110:28:110:28 | p | provenance | | +| tst2.js:105:9:105:9 | p | tst2.js:105:7:105:24 | p | provenance | | +| tst2.js:110:7:110:29 | obj [someProperty] | tst2.js:111:36:111:38 | obj [someProperty] | provenance | | +| tst2.js:110:13:110:29 | {someProperty: p} [someProperty] | tst2.js:110:7:110:29 | obj [someProperty] | provenance | | +| tst2.js:110:28:110:28 | p | tst2.js:110:13:110:29 | {someProperty: p} [someProperty] | provenance | | +| tst2.js:110:28:110:28 | p | tst2.js:111:16:111:55 | seriali ... true}) | provenance | | +| tst2.js:111:7:111:55 | unsafe | tst2.js:113:12:113:17 | unsafe | provenance | | +| tst2.js:111:16:111:55 | seriali ... true}) | tst2.js:111:7:111:55 | unsafe | provenance | | +| tst2.js:111:36:111:38 | obj [someProperty] | tst2.js:111:16:111:55 | seriali ... true}) | provenance | | | tst3.js:5:7:5:24 | p | tst3.js:6:12:6:12 | p | provenance | | | tst3.js:5:9:5:9 | p | tst3.js:5:7:5:24 | p | provenance | | | tst3.js:11:9:11:74 | code | tst3.js:12:12:12:15 | code | provenance | | @@ -457,6 +475,22 @@ nodes | tst2.js:88:12:88:12 | p | semmle.label | p | | tst2.js:89:12:89:16 | other [p] | semmle.label | other [p] | | tst2.js:89:12:89:18 | other.p | semmle.label | other.p | +| tst2.js:93:7:93:24 | p | semmle.label | p | +| tst2.js:93:9:93:9 | p | semmle.label | p | +| tst2.js:99:7:99:69 | unsafe | semmle.label | unsafe | +| tst2.js:99:16:99:69 | seriali ... true}) | semmle.label | seriali ... true}) | +| tst2.js:99:36:99:52 | {someProperty: p} [someProperty] | semmle.label | {someProperty: p} [someProperty] | +| tst2.js:99:51:99:51 | p | semmle.label | p | +| tst2.js:101:12:101:17 | unsafe | semmle.label | unsafe | +| tst2.js:105:7:105:24 | p | semmle.label | p | +| tst2.js:105:9:105:9 | p | semmle.label | p | +| tst2.js:110:7:110:29 | obj [someProperty] | semmle.label | obj [someProperty] | +| tst2.js:110:13:110:29 | {someProperty: p} [someProperty] | semmle.label | {someProperty: p} [someProperty] | +| tst2.js:110:28:110:28 | p | semmle.label | p | +| tst2.js:111:7:111:55 | unsafe | semmle.label | unsafe | +| tst2.js:111:16:111:55 | seriali ... true}) | semmle.label | seriali ... true}) | +| tst2.js:111:36:111:38 | obj [someProperty] | semmle.label | obj [someProperty] | +| tst2.js:113:12:113:17 | unsafe | semmle.label | unsafe | | tst3.js:5:7:5:24 | p | semmle.label | p | | tst3.js:5:9:5:9 | p | semmle.label | p | | tst3.js:6:12:6:12 | p | semmle.label | p | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXssWithCustomSanitizer.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXssWithCustomSanitizer.expected index b57d294c7a7f..a4b02fa07491 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXssWithCustomSanitizer.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXssWithCustomSanitizer.expected @@ -68,5 +68,7 @@ | tst2.js:76:12:76:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:69:9:69:9 | p | user-provided value | | tst2.js:88:12:88:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:82:9:82:9 | p | user-provided value | | tst2.js:89:12:89:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:82:9:82:9 | p | user-provided value | +| tst2.js:101:12:101:17 | unsafe | Cross-site scripting vulnerability due to $@. | tst2.js:93:9:93:9 | p | user-provided value | +| tst2.js:113:12:113:17 | unsafe | Cross-site scripting vulnerability due to $@. | tst2.js:105:9:105:9 | p | user-provided value | | tst3.js:6:12:6:12 | p | Cross-site scripting vulnerability due to $@. | tst3.js:5:9:5:9 | p | user-provided value | | tst3.js:12:12:12:15 | code | Cross-site scripting vulnerability due to $@. | tst3.js:11:32:11:39 | reg.body | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js index 20149bea1e76..fff9c2250972 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js @@ -90,7 +90,7 @@ app.get('/baz', function(req, res) { }); app.get('/baz', function(req, res) { - let { p } = req.params; // $ MISSING: Source + let { p } = req.params; // $ Source var serialized = serializeJavaScript(p); @@ -98,11 +98,11 @@ app.get('/baz', function(req, res) { var unsafe = serializeJavaScript({someProperty: p}, {unsafe: true}); - res.send(unsafe); // $ MISSING: Alert + res.send(unsafe); // $ Alert }); app.get('/baz', function(req, res) { - let { p } = req.params; // $ MISSING: Source + let { p } = req.params; // $ Source var serialized = serializeJavaScript(p); @@ -110,5 +110,5 @@ app.get('/baz', function(req, res) { let obj = {someProperty: p}; var unsafe = serializeJavaScript(obj, {unsafe: true}); - res.send(unsafe); // $ MISSING: Alert + res.send(unsafe); // $ Alert }); From fffbc0c0bc89f4973f46f6b18aee7471a2a954ab Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Mon, 16 Jun 2025 09:34:31 +0200 Subject: [PATCH 3/5] JS: add change note --- javascript/ql/lib/change-notes/2025-06-16-serialize-js.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 javascript/ql/lib/change-notes/2025-06-16-serialize-js.md diff --git a/javascript/ql/lib/change-notes/2025-06-16-serialize-js.md b/javascript/ql/lib/change-notes/2025-06-16-serialize-js.md new file mode 100644 index 000000000000..e9afcfaf42f4 --- /dev/null +++ b/javascript/ql/lib/change-notes/2025-06-16-serialize-js.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Improved XSS detection for `serialize-javascript` calls with tainted object properties. From eca69e1654ada174cebf3d287fd83303a8e39cfb Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Mon, 16 Jun 2025 12:59:36 +0200 Subject: [PATCH 4/5] JS: remove `serialize-javascript` from `JsonParsers.qll` as it is not a parser --- javascript/ql/lib/semmle/javascript/JsonParsers.qll | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/JsonParsers.qll b/javascript/ql/lib/semmle/javascript/JsonParsers.qll index e3f1f285a212..f447f0e41c9e 100644 --- a/javascript/ql/lib/semmle/javascript/JsonParsers.qll +++ b/javascript/ql/lib/semmle/javascript/JsonParsers.qll @@ -33,8 +33,7 @@ private class PlainJsonParserCall extends JsonParserCall { callee = DataFlow::moduleImport("parse-json") or callee = DataFlow::moduleImport("json-parse-better-errors") or callee = DataFlow::moduleImport("json-safe-parse") or - callee = AngularJS::angular().getAPropertyRead("fromJson") or - callee = DataFlow::moduleImport("serialize-javascript") + callee = AngularJS::angular().getAPropertyRead("fromJson") ) } From fc0c8a8f5a975ffb470943372471090bc1d1e2b5 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Tue, 17 Jun 2025 08:20:35 +0200 Subject: [PATCH 5/5] JS: update change note --- javascript/ql/lib/change-notes/2025-06-16-serialize-js.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/change-notes/2025-06-16-serialize-js.md b/javascript/ql/lib/change-notes/2025-06-16-serialize-js.md index e9afcfaf42f4..a89e0e19b6f4 100644 --- a/javascript/ql/lib/change-notes/2025-06-16-serialize-js.md +++ b/javascript/ql/lib/change-notes/2025-06-16-serialize-js.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* Improved XSS detection for `serialize-javascript` calls with tainted object properties. +* Improved taint tracking through calls to `serialize-javascript`.