From 13c701948a8875f675d2fc78b52b78da13110caf Mon Sep 17 00:00:00 2001 From: Napalys Date: Mon, 10 Mar 2025 19:27:36 +0100 Subject: [PATCH 1/3] Refactor Markdown taint steps and update expected results for reflected XSS tests --- javascript/ql/lib/ext/markdown-table.model.yml | 6 ++++++ .../lib/semmle/javascript/frameworks/Markdown.qll | 13 ------------- .../CWE-079/ReflectedXss/ReflectedXss.expected | 8 ++++++++ .../Security/CWE-079/ReflectedXss/ReflectedXss.js | 4 ++-- .../ReflectedXssWithCustomSanitizer.expected | 1 + 5 files changed, 17 insertions(+), 15 deletions(-) create mode 100644 javascript/ql/lib/ext/markdown-table.model.yml diff --git a/javascript/ql/lib/ext/markdown-table.model.yml b/javascript/ql/lib/ext/markdown-table.model.yml new file mode 100644 index 000000000000..4dcf36164c99 --- /dev/null +++ b/javascript/ql/lib/ext/markdown-table.model.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/javascript-all + extensible: summaryModel + data: + - ["markdown-table", "", "Argument[0].ArrayElement.ArrayElement", "ReturnValue", "taint"] diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Markdown.qll b/javascript/ql/lib/semmle/javascript/frameworks/Markdown.qll index 04f3c9f7db78..344bdc2a40fc 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Markdown.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Markdown.qll @@ -46,19 +46,6 @@ module Markdown { } } - /** - * A taint step for the `markdown-table` library. - */ - private class MarkdownTableStep extends MarkdownStep { - override predicate step(DataFlow::Node pred, DataFlow::Node succ) { - exists(DataFlow::CallNode call | call = DataFlow::moduleImport("markdown-table").getACall() | - // TODO: needs a flow summary to ensure ArrayElement content is unfolded - succ = call and - pred = call.getArgument(0) - ) - } - } - /** * A taint step for the `showdown` library. */ 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 9e842ab516ed..675403f5ab9d 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 @@ -2,6 +2,9 @@ edges | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | provenance | | | ReflectedXss.js:17:31:17:39 | params.id | ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id | provenance | | | ReflectedXss.js:23:19:23:26 | req.body | ReflectedXss.js:23:12:23:27 | marked(req.body) | provenance | | +| ReflectedXss.js:30:7:33:4 | mytable | ReflectedXss.js:34:12:34:18 | mytable | provenance | | +| ReflectedXss.js:30:17:33:4 | table([ ... y]\\n ]) | ReflectedXss.js:30:7:33:4 | mytable | provenance | | +| ReflectedXss.js:32:14:32:21 | req.body | ReflectedXss.js:30:17:33:4 | table([ ... y]\\n ]) | provenance | | | ReflectedXss.js:42:31:42:38 | req.body | ReflectedXss.js:42:12:42:39 | convert ... q.body) | provenance | | | ReflectedXss.js:64:14:64:21 | req.body | ReflectedXss.js:64:39:64:42 | file | provenance | | | ReflectedXss.js:64:39:64:42 | file | ReflectedXss.js:65:16:65:19 | file | provenance | | @@ -152,6 +155,10 @@ nodes | ReflectedXss.js:23:12:23:27 | marked(req.body) | semmle.label | marked(req.body) | | ReflectedXss.js:23:19:23:26 | req.body | semmle.label | req.body | | ReflectedXss.js:29:12:29:19 | req.body | semmle.label | req.body | +| ReflectedXss.js:30:7:33:4 | mytable | semmle.label | mytable | +| ReflectedXss.js:30:17:33:4 | table([ ... y]\\n ]) | semmle.label | table([ ... y]\\n ]) | +| ReflectedXss.js:32:14:32:21 | req.body | semmle.label | req.body | +| ReflectedXss.js:34:12:34:18 | mytable | semmle.label | mytable | | ReflectedXss.js:41:12:41:19 | req.body | semmle.label | req.body | | ReflectedXss.js:42:12:42:39 | convert ... q.body) | semmle.label | convert ... q.body) | | ReflectedXss.js:42:31:42:38 | req.body | semmle.label | req.body | @@ -340,6 +347,7 @@ subpaths | ReflectedXss.js:22:12:22:19 | req.body | ReflectedXss.js:22:12:22:19 | req.body | ReflectedXss.js:22:12:22:19 | req.body | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:22:12:22:19 | req.body | user-provided value | | ReflectedXss.js:23:12:23:27 | marked(req.body) | ReflectedXss.js:23:19:23:26 | req.body | ReflectedXss.js:23:12:23:27 | marked(req.body) | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:23:19:23:26 | req.body | user-provided value | | ReflectedXss.js:29:12:29:19 | req.body | ReflectedXss.js:29:12:29:19 | req.body | ReflectedXss.js:29:12:29:19 | req.body | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:29:12:29:19 | req.body | user-provided value | +| ReflectedXss.js:34:12:34:18 | mytable | ReflectedXss.js:32:14:32:21 | req.body | ReflectedXss.js:34:12:34:18 | mytable | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:32:14:32:21 | req.body | user-provided value | | ReflectedXss.js:41:12:41:19 | req.body | ReflectedXss.js:41:12:41:19 | req.body | ReflectedXss.js:41:12:41:19 | req.body | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:41:12:41:19 | req.body | user-provided value | | ReflectedXss.js:42:12:42:39 | convert ... q.body) | ReflectedXss.js:42:31:42:38 | req.body | ReflectedXss.js:42:12:42:39 | convert ... q.body) | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:42:31:42:38 | req.body | user-provided value | | ReflectedXss.js:56:12:56:19 | req.body | ReflectedXss.js:56:12:56:19 | req.body | ReflectedXss.js:56:12:56:19 | req.body | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:56:12:56:19 | req.body | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.js b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.js index c3b1cbc2da8a..dee88dd177bc 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.js +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.js @@ -31,7 +31,7 @@ app.get('/user/:id', function(req, res) { ['Name', 'Content'], ['body', req.body] ]); - res.send(mytable); // NOT OK - FIXME: only works in OLD dataflow, add implicit reads before library-contributed taint steps + res.send(mytable); // NOT OK }); var showdown = require('showdown'); @@ -122,4 +122,4 @@ app.get("invalid/keys/:id", async (req, res) => { res.status(400).send(`${invalidKeys.join(', ')} not in whitelist`); return; } -}); \ No newline at end of file +}); 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 d29b35203b80..a367f07307a5 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 @@ -3,6 +3,7 @@ | ReflectedXss.js:22:12:22:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:22:12:22:19 | req.body | user-provided value | | ReflectedXss.js:23:12:23:27 | marked(req.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:23:19:23:26 | req.body | user-provided value | | ReflectedXss.js:29:12:29:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:29:12:29:19 | req.body | user-provided value | +| ReflectedXss.js:34:12:34:18 | mytable | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:32:14:32:21 | req.body | user-provided value | | ReflectedXss.js:41:12:41:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:41:12:41:19 | req.body | user-provided value | | ReflectedXss.js:42:12:42:39 | convert ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:42:31:42:38 | req.body | user-provided value | | ReflectedXss.js:56:12:56:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:56:12:56:19 | req.body | user-provided value | From 4a365857f1879c207480e1bb918e4f7674c808e2 Mon Sep 17 00:00:00 2001 From: Napalys Date: Mon, 10 Mar 2025 19:40:41 +0100 Subject: [PATCH 2/3] Added change note. --- .../lib/change-notes/2025-03-10-js-refactor-markdown-table.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 javascript/ql/lib/change-notes/2025-03-10-js-refactor-markdown-table.md diff --git a/javascript/ql/lib/change-notes/2025-03-10-js-refactor-markdown-table.md b/javascript/ql/lib/change-notes/2025-03-10-js-refactor-markdown-table.md new file mode 100644 index 000000000000..29e767c0d561 --- /dev/null +++ b/javascript/ql/lib/change-notes/2025-03-10-js-refactor-markdown-table.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Improved the modeling of the `markdown-table` package by replacing the QL implementation with a data model. From 1ad8b4677db139dc4c194a040d6b46f663fe9111 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Tue, 11 Mar 2025 08:07:49 +0100 Subject: [PATCH 3/3] Update javascript/ql/lib/change-notes/2025-03-10-js-refactor-markdown-table.md Co-authored-by: Asger F --- .../lib/change-notes/2025-03-10-js-refactor-markdown-table.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/change-notes/2025-03-10-js-refactor-markdown-table.md b/javascript/ql/lib/change-notes/2025-03-10-js-refactor-markdown-table.md index 29e767c0d561..8dd3c17404c7 100644 --- a/javascript/ql/lib/change-notes/2025-03-10-js-refactor-markdown-table.md +++ b/javascript/ql/lib/change-notes/2025-03-10-js-refactor-markdown-table.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* Improved the modeling of the `markdown-table` package by replacing the QL implementation with a data model. +* Improved the modeling of the `markdown-table` package to ensure it handles nested arrays properly.