From 162de332eb5590e9537cd7eefcc872222c6de655 Mon Sep 17 00:00:00 2001 From: Sam Robson Date: Thu, 12 Nov 2026 17:26:35 +0000 Subject: [PATCH 1/2] fix(js): stop treating parseInt and parseFloat as whole-string number guards --- .../2026-11-12-isnumberguard-parseint.md | 4 ++++ .../javascript/dataflow/TaintTracking.qll | 7 ------- .../UnsafeShellCommandConstruction.expected | 13 ++++++++++++ .../UnsafeShellCommandConstruction/lib/lib.js | 21 +++++++++++++++---- 4 files changed, 34 insertions(+), 11 deletions(-) create mode 100644 javascript/ql/lib/change-notes/2026-11-12-isnumberguard-parseint.md diff --git a/javascript/ql/lib/change-notes/2026-11-12-isnumberguard-parseint.md b/javascript/ql/lib/change-notes/2026-11-12-isnumberguard-parseint.md new file mode 100644 index 000000000000..ad7ce053a846 --- /dev/null +++ b/javascript/ql/lib/change-notes/2026-11-12-isnumberguard-parseint.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Checks of the form `isNaN(parseInt(x))` and `isNaN(parseFloat(x))` are no longer treated as checking that `x` is a number, since `parseInt` and `parseFloat` only validate a numeric prefix of `x`. This can reveal previously missed alerts, for example from `js/shell-command-constructed-from-input` and `js/unsafe-jquery-plugin`. diff --git a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll index 5d7895b53dbc..a8e3658a5e58 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll @@ -989,13 +989,6 @@ module TaintTracking { | operand = isNaN.getArgument(0).asExpr() or - exists(DataFlow::CallNode parse | - parse = DataFlow::globalVarRef(["parseInt", "parseFloat"]).getACall() - | - parse = isNaN.getArgument(0) and - operand = parse.getArgument(0).asExpr() - ) - or exists(UnaryExpr unary | unary.getOperator() = ["+", "-"] | unary = isNaN.getArgument(0).asExpr() and operand = unary.getOperand() diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected index 6fb03c27bb06..6717207be3b0 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected @@ -76,8 +76,10 @@ | lib/lib.js:510:10:510:25 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:510:2:510:26 | cp.exec ... + name) | shell command | | lib/lib.js:513:11:513:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:513:3:513:27 | cp.exec ... + name) | shell command | | lib/lib.js:519:11:519:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:519:3:519:27 | cp.exec ... + name) | shell command | +| lib/lib.js:521:11:521:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:521:23:521:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:521:3:521:27 | cp.exec ... + name) | shell command | | lib/lib.js:525:11:525:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:525:23:525:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:525:3:525:27 | cp.exec ... + name) | shell command | | lib/lib.js:531:11:531:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:531:23:531:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:531:3:531:27 | cp.exec ... + name) | shell command | +| lib/lib.js:533:11:533:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:533:23:533:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:533:3:533:27 | cp.exec ... + name) | shell command | | lib/lib.js:537:11:537:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:537:23:537:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:537:3:537:27 | cp.exec ... + name) | shell command | | lib/lib.js:543:11:543:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:543:23:543:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:543:3:543:27 | cp.exec ... + name) | shell command | | lib/lib.js:545:11:545:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:545:3:545:27 | cp.exec ... + name) | shell command | @@ -117,6 +119,8 @@ | lib/lib.js:729:26:729:29 | args | lib/lib.js:643:48:643:51 | name | lib/lib.js:729:26:729:29 | args | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:643:48:643:51 | name | library input | lib/lib.js:729:9:729:47 | cp.spaw ... true }) | shell command | | lib/lib.js:730:29:730:32 | args | lib/lib.js:643:48:643:51 | name | lib/lib.js:730:29:730:32 | args | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:643:48:643:51 | name | library input | lib/lib.js:730:9:730:55 | cp.exec ... /sh" }) | shell command | | lib/lib.js:736:26:736:31 | [name] | lib/lib.js:643:48:643:51 | name | lib/lib.js:736:26:736:31 | [name] | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:643:48:643:51 | name | library input | lib/lib.js:736:9:736:41 | cp.spaw ... ptions) | shell command | +| lib/lib.js:752:11:752:26 | "rm -rf " + name | lib/lib.js:750:48:750:51 | name | lib/lib.js:752:23:752:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:750:48:750:51 | name | library input | lib/lib.js:752:3:752:27 | cp.exec ... + name) | shell command | +| lib/lib.js:754:11:754:26 | "rm -rf " + name | lib/lib.js:750:48:750:51 | name | lib/lib.js:754:23:754:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:750:48:750:51 | name | library input | lib/lib.js:754:3:754:27 | cp.exec ... + name) | shell command | | lib/subLib2/compiled-file.ts:4:13:4:28 | "rm -rf " + name | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib2/compiled-file.ts:3:26:3:29 | name | library input | lib/subLib2/compiled-file.ts:4:5:4:29 | cp.exec ... + name) | shell command | | lib/subLib2/special-file.js:4:10:4:25 | "rm -rf " + name | lib/subLib2/special-file.js:3:28:3:31 | name | lib/subLib2/special-file.js:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib2/special-file.js:3:28:3:31 | name | library input | lib/subLib2/special-file.js:4:2:4:26 | cp.exec ... + name) | shell command | | lib/subLib3/my-file.ts:4:10:4:25 | "rm -rf " + name | lib/subLib3/my-file.ts:3:28:3:31 | name | lib/subLib3/my-file.ts:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib3/my-file.ts:3:28:3:31 | name | library input | lib/subLib3/my-file.ts:4:2:4:26 | cp.exec ... + name) | shell command | @@ -237,8 +241,10 @@ edges | lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name | provenance | | | lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name | provenance | | | lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name | provenance | | +| lib/lib.js:509:39:509:42 | name | lib/lib.js:521:23:521:26 | name | provenance | | | lib/lib.js:509:39:509:42 | name | lib/lib.js:525:23:525:26 | name | provenance | | | lib/lib.js:509:39:509:42 | name | lib/lib.js:531:23:531:26 | name | provenance | | +| lib/lib.js:509:39:509:42 | name | lib/lib.js:533:23:533:26 | name | provenance | | | lib/lib.js:509:39:509:42 | name | lib/lib.js:537:23:537:26 | name | provenance | | | lib/lib.js:509:39:509:42 | name | lib/lib.js:543:23:543:26 | name | provenance | | | lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name | provenance | | @@ -375,6 +381,8 @@ edges | lib/lib.js:734:14:734:27 | runWithOptions [name] | lib/lib.js:738:5:738:18 | runWithOptions [name] | provenance | | | lib/lib.js:736:27:736:30 | name | lib/lib.js:736:26:736:31 | [name] | provenance | | | lib/lib.js:738:5:738:18 | runWithOptions [name] | lib/lib.js:736:27:736:30 | name | provenance | | +| lib/lib.js:750:48:750:51 | name | lib/lib.js:752:23:752:26 | name | provenance | | +| lib/lib.js:750:48:750:51 | name | lib/lib.js:754:23:754:26 | name | provenance | | | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | provenance | | | lib/subLib2/special-file.js:3:28:3:31 | name | lib/subLib2/special-file.js:4:22:4:25 | name | provenance | | | lib/subLib3/my-file.ts:3:28:3:31 | name | lib/subLib3/my-file.ts:4:22:4:25 | name | provenance | | @@ -542,8 +550,10 @@ nodes | lib/lib.js:510:22:510:25 | name | semmle.label | name | | lib/lib.js:513:23:513:26 | name | semmle.label | name | | lib/lib.js:519:23:519:26 | name | semmle.label | name | +| lib/lib.js:521:23:521:26 | name | semmle.label | name | | lib/lib.js:525:23:525:26 | name | semmle.label | name | | lib/lib.js:531:23:531:26 | name | semmle.label | name | +| lib/lib.js:533:23:533:26 | name | semmle.label | name | | lib/lib.js:537:23:537:26 | name | semmle.label | name | | lib/lib.js:543:23:543:26 | name | semmle.label | name | | lib/lib.js:545:23:545:26 | name | semmle.label | name | @@ -646,6 +656,9 @@ nodes | lib/lib.js:736:26:736:31 | [name] | semmle.label | [name] | | lib/lib.js:736:27:736:30 | name | semmle.label | name | | lib/lib.js:738:5:738:18 | runWithOptions [name] | semmle.label | runWithOptions [name] | +| lib/lib.js:750:48:750:51 | name | semmle.label | name | +| lib/lib.js:752:23:752:26 | name | semmle.label | name | +| lib/lib.js:754:23:754:26 | name | semmle.label | name | | lib/subLib2/compiled-file.ts:3:26:3:29 | name | semmle.label | name | | lib/subLib2/compiled-file.ts:4:25:4:28 | name | semmle.label | name | | lib/subLib2/special-file.js:3:28:3:31 | name | semmle.label | name | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js index 86db944c2fe1..15122b75b45e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js @@ -515,10 +515,10 @@ module.exports.sanitizer4 = function (name) { // $ Source cp.exec("rm -rf " + name); } - if (isNaN(parseInt(name))) { + if (isNaN(parseInt(name))) { // <- only checks a numeric prefix cp.exec("rm -rf " + name); // $ Alert } else { - cp.exec("rm -rf " + name); + cp.exec("rm -rf " + name); // $ Alert } if (isNaN(+name)) { @@ -527,10 +527,10 @@ module.exports.sanitizer4 = function (name) { // $ Source cp.exec("rm -rf " + name); } - if (isNaN(parseInt(name, 10))) { + if (isNaN(parseInt(name, 10))) { // <- only checks a numeric prefix cp.exec("rm -rf " + name); // $ Alert } else { - cp.exec("rm -rf " + name); + cp.exec("rm -rf " + name); // $ Alert } if (isNaN(name - 0)) { @@ -746,3 +746,16 @@ module.exports.shellOptionsOnly = function (options) { cp.execFile("echo", options, () => {}); cp.execFileSync("echo", options); }; + +module.exports.sanitizerParseFloat = function (name) { // $ Source + if (isNaN(parseFloat(name))) { // <- only checks a numeric prefix + cp.exec("rm -rf " + name); // $ Alert + } else { + cp.exec("rm -rf " + name); // $ Alert + } + + const parsed = parseInt(name, 10); + if (!isNaN(parsed)) { + cp.exec("rm -rf " + parsed); + } +} From 1317435d2f1579172b10d68e0389c246390f9e64 Mon Sep 17 00:00:00 2001 From: Sam Robson Date: Fri, 13 Nov 2026 11:04:15 +0000 Subject: [PATCH 2/2] fix(js): preserve parseInt and parseFloat guard for prototype pollution --- .../PrototypePollutingAssignmentQuery.qll | 17 +++++++++++++++-- .../PrototypePollutingAssignment.expected | 15 +++++++++++++++ .../CWE-915/PrototypePollutingAssignment/tst.js | 17 +++++++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll index 96eed4cadc2b..88da1412de14 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll @@ -312,12 +312,25 @@ private class TypeofCheck extends BarrierGuard, DataFlow::ValueNode { } } -/** A guard that checks whether `x` is a number. */ +/** + * A guard that checks whether `x` is a number, or starts with a numeric prefix + * that rules out `"__proto__"`. + */ class NumberGuard extends BarrierGuard instanceof DataFlow::CallNode { Expr x; boolean polarity; - NumberGuard() { TaintTracking::isNumberGuard(this, x, polarity) } + NumberGuard() { + TaintTracking::isNumberGuard(this, x, polarity) + or + this = DataFlow::globalVarRef("isNaN").getACall() and + polarity = false and + exists(DataFlow::CallNode parse | + parse = DataFlow::globalVarRef(["parseInt", "parseFloat"]).getACall() and + parse = super.getArgument(0) and + x = parse.getArgument(0).asExpr() + ) + } override predicate blocksExpr(boolean outcome, Expr e) { e = x and outcome = polarity } } diff --git a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected index c2fddac0ce35..6c766c6462ca 100644 --- a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected +++ b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected @@ -26,6 +26,8 @@ | tst.js:97:5:97:46 | obj[req ... g, '')] | tst.js:97:9:97:19 | req.query.x | tst.js:97:5:97:46 | obj[req ... g, '')] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:97:9:97:19 | req.query.x | user controlled input | | tst.js:105:5:105:17 | object[taint] | tst.js:102:24:102:37 | req.query.data | tst.js:105:5:105:17 | object[taint] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:102:24:102:37 | req.query.data | user controlled input | | tst.js:130:5:130:53 | obj[req ... ), '')] | tst.js:130:9:130:19 | req.query.x | tst.js:130:5:130:53 | obj[req ... ), '')] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:130:9:130:19 | req.query.x | user controlled input | +| tst.js:139:9:139:16 | obj[key] | tst.js:135:22:135:34 | req.query.key | tst.js:139:9:139:16 | obj[key] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:135:22:135:34 | req.query.key | user controlled input | +| tst.js:145:9:145:16 | obj[key] | tst.js:135:22:135:34 | req.query.key | tst.js:145:9:145:16 | obj[key] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:135:22:135:34 | req.query.key | user controlled input | edges | lib.js:1:38:1:40 | obj | lib.js:6:7:6:9 | obj | provenance | | | lib.js:1:43:1:46 | path | lib.js:2:21:2:24 | path | provenance | | @@ -136,6 +138,12 @@ edges | tst.js:105:12:105:16 | taint | tst.js:105:5:105:17 | object[taint] | provenance | Config | | tst.js:130:9:130:19 | req.query.x | tst.js:130:9:130:52 | req.que ... '), '') | provenance | Config | | tst.js:130:9:130:52 | req.que ... '), '') | tst.js:130:5:130:53 | obj[req ... ), '')] | provenance | Config | +| tst.js:135:9:135:11 | key | tst.js:139:13:139:15 | key | provenance | | +| tst.js:135:9:135:11 | key | tst.js:145:13:145:15 | key | provenance | | +| tst.js:135:15:135:35 | String( ... ry.key) | tst.js:135:9:135:11 | key | provenance | | +| tst.js:135:22:135:34 | req.query.key | tst.js:135:15:135:35 | String( ... ry.key) | provenance | Config | +| tst.js:139:13:139:15 | key | tst.js:139:9:139:16 | obj[key] | provenance | Config | +| tst.js:145:13:145:15 | key | tst.js:145:9:145:16 | obj[key] | provenance | Config | nodes | lib.js:1:38:1:40 | obj | semmle.label | obj | | lib.js:1:43:1:46 | path | semmle.label | path | @@ -265,5 +273,12 @@ nodes | tst.js:130:5:130:53 | obj[req ... ), '')] | semmle.label | obj[req ... ), '')] | | tst.js:130:9:130:19 | req.query.x | semmle.label | req.query.x | | tst.js:130:9:130:52 | req.que ... '), '') | semmle.label | req.que ... '), '') | +| tst.js:135:9:135:11 | key | semmle.label | key | +| tst.js:135:15:135:35 | String( ... ry.key) | semmle.label | String( ... ry.key) | +| tst.js:135:22:135:34 | req.query.key | semmle.label | req.query.key | +| tst.js:139:9:139:16 | obj[key] | semmle.label | obj[key] | +| tst.js:139:13:139:15 | key | semmle.label | key | +| tst.js:145:9:145:16 | obj[key] | semmle.label | obj[key] | +| tst.js:145:13:145:15 | key | semmle.label | key | subpaths | tst.js:14:27:14:31 | taint | tst.js:55:29:55:32 | prop | tst.js:56:12:56:33 | obj ? o ... : null | tst.js:14:5:14:32 | unsafeG ... taint) | diff --git a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/tst.js b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/tst.js index 91fb71941b36..6bd492660448 100644 --- a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/tst.js @@ -130,3 +130,20 @@ app.get('/foo', (req, res) => { obj[req.query.x.replace(new RegExp('_', ''), '')].x = 'foo'; // $ Alert obj[req.query.x.replace(new RegExp('_', unknownFlags()), '')].x = 'foo'; }); + +app.get('/number-guard', (req, res) => { + let key = String(req.query.key); // $ Source + let obj = {}; + + if (isNaN(parseInt(key))) { + obj[key].x = 'foo'; // $ Alert + } else { + obj[key].x = 'foo'; + } + + if (isNaN(parseFloat(key))) { + obj[key].x = 'foo'; // $ Alert + } else { + obj[key].x = 'foo'; + } +});