From b6ffc461509858434ee0a32efb597bcd8dba1bd2 Mon Sep 17 00:00:00 2001 From: Napalys Date: Wed, 27 Nov 2024 12:12:08 +0100 Subject: [PATCH] JS: incomplete sanitization now also works with RegExp objects --- .../Security/CWE-116/IncompleteSanitization.ql | 6 +++--- .../IncompleteSanitization.expected | 9 +++++++++ .../CWE-116/IncompleteSanitization/tst.js | 16 ++++++++-------- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql b/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql index dc92b24abef41..9a00cabc83b28 100644 --- a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql +++ b/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql @@ -23,7 +23,7 @@ string metachar() { result = "'\"\\&<>\n\r\t*|{}[]%$".charAt(_) } /** Gets a string matched by `e` in a `replace` call. */ string getAMatchedString(DataFlow::Node e) { - result = e.(DataFlow::RegExpLiteralNode).getRoot().getAMatchedString() + result = e.(DataFlow::RegExpCreationNode).getRoot().getAMatchedString() or result = e.getStringValue() } @@ -52,7 +52,7 @@ predicate isSimpleAlt(RegExpAlt t) { forall(RegExpTerm ch | ch = t.getAChild() | * Holds if `mce` is of the form `x.replace(re, new)`, where `re` is a global * regular expression and `new` prefixes the matched string with a backslash. */ -predicate isBackslashEscape(StringReplaceCall mce, DataFlow::RegExpLiteralNode re) { +predicate isBackslashEscape(StringReplaceCall mce, DataFlow::RegExpCreationNode re) { mce.isGlobal() and re = mce.getRegExp() and ( @@ -72,7 +72,7 @@ predicate allBackslashesEscaped(DataFlow::Node nd) { nd instanceof JsonStringifyCall or // check whether `nd` itself escapes backslashes - exists(DataFlow::RegExpLiteralNode rel | isBackslashEscape(nd, rel) | + exists(DataFlow::RegExpCreationNode rel | isBackslashEscape(nd, rel) | // if it's a complex regexp, we conservatively assume that it probably escapes backslashes not isSimple(rel.getRoot()) or getAMatchedString(rel) = "\\" diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected index cf6105d8c6fe4..7e7327d599ead 100644 --- a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected @@ -29,3 +29,12 @@ | tst.js:149:2:149:24 | x.repla ... replace | This replaces only the first occurrence of "\\n". | | tst.js:193:9:193:17 | s.replace | This replaces only the first occurrence of /'/. | | tst.js:202:10:202:18 | p.replace | This replaces only the first occurrence of "/../". | +| tst.js:337:2:337:12 | s().replace | This replaces only the first occurrence of new Reg ... nown()). | +| tst.js:341:9:341:17 | p.replace | This replaces only the first occurrence of new Reg ... .\\\\./"). | +| tst.js:345:9:345:17 | s.replace | This does not escape backslash characters in the input. | +| tst.js:349:9:349:17 | s.replace | This replaces only the first occurrence of new RegExp("\\'"). | +| tst.js:353:9:353:17 | s.replace | This replaces only the first occurrence of new Reg ... lags()). | +| tst.js:362:2:362:10 | x.replace | This replaces only the first occurrence of new RegExp("\\n"). | +| tst.js:363:2:363:24 | x.repla ... replace | This replaces only the first occurrence of new RegExp("\\n"). | +| tst.js:365:2:365:10 | x.replace | This replaces only the first occurrence of new Reg ... lags()). | +| tst.js:366:2:366:24 | x.repla ... replace | This replaces only the first occurrence of new Reg ... lags()). | diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js index 994c7c5182d1e..bdac925e6f737 100644 --- a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js @@ -338,19 +338,19 @@ function typicalBadHtmlSanitizers(s) { } function bad18NewRegExp(p) { - return p.replace(new RegExp("\\.\\./"), ""); // NOT OK -- should be flagged, but currently checking only for literals + return p.replace(new RegExp("\\.\\./"), ""); // NOT OK } function bad4NewRegExpG(s) { - return s.replace(new RegExp("\'","g"), "\\$&"); // NOT OK -- should be flagged, but currently checking only for literals + return s.replace(new RegExp("\'","g"), "\\$&"); // NOT OK } function bad4NewRegExp(s) { - return s.replace(new RegExp("\'"), "\\$&"); // NOT OK -- should be flagged, but currently checking only for literals + return s.replace(new RegExp("\'"), "\\$&"); // NOT OK } function bad4NewRegExpUnknown(s) { - return s.replace(new RegExp("\'", unknownFlags()), "\\$&"); // NOT OK -- should be flagged, but currently checking only for literals + return s.replace(new RegExp("\'", unknownFlags()), "\\$&"); // NOT OK } function newlinesNewReGexp(s) { @@ -359,9 +359,9 @@ function newlinesNewReGexp(s) { x.replace(new RegExp("\n", "g"), "").replace(x, y); // OK x.replace(x, y).replace(new RegExp("\n", "g"), ""); // OK - x.replace(new RegExp("\n"), "").replace(x, y); // NOT OK -- should be flagged, but currently checking only for literals - x.replace(x, y).replace(new RegExp("\n"), ""); // NOT OK -- should be flagged, but currently checking only for literals + x.replace(new RegExp("\n"), "").replace(x, y); // NOT OK + x.replace(x, y).replace(new RegExp("\n"), ""); // NOT OK - x.replace(new RegExp("\n", unknownFlags()), "").replace(x, y); // OK - x.replace(x, y).replace(new RegExp("\n", unknownFlags()), ""); // OK + x.replace(new RegExp("\n", unknownFlags()), "").replace(x, y); // OK -- Should not be flagged but now it is + x.replace(x, y).replace(new RegExp("\n", unknownFlags()), ""); // OK -- Should not be flagged but now it is }