Skip to content

Commit 26b5f26

Browse files
owen-mcowen-mc
authored
Merge pull request #22966 from owen-mc/owen-mc/22845-multiline-regexp-sanitizer
JS: Fix multiline regexp sanitization
2 parents d3c4a3b + a986a00 commit 26b5f26

7 files changed

Lines changed: 245 additions & 8 deletions

File tree

‎javascript/ql/lib/change-notes/2026-10-22-multiline-regexp-sanitizer.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+
* Regular expression checks using multiline (`m`) or unknown flags are no longer treated as generic taint sanitizers just because the pattern is anchored with `^` and `$`. This can reveal previously missed alerts, including command injection and reflected cross-site scripting.

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

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1275,16 +1275,26 @@ module RegExp {
12751275
bindingset[flags]
12761276
predicate maybeDotAll(string flags) { flags = unknownFlag() or isDotAll(flags) }
12771277

1278-
/** Holds if `term` and all of its disjuncts are anchored on both ends. */
1278+
/**
1279+
* Holds if `term` and all of its disjuncts are anchored to both ends of the input.
1280+
*
1281+
* Does not hold if a regular expression using the term has multiline or unknown flags.
1282+
*/
12791283
predicate isFullyAnchoredTerm(RegExpTerm term) {
1280-
exists(RegExpSequence seq | term = seq |
1281-
seq.getChild(0) instanceof RegExpCaret and
1282-
seq.getLastChild() instanceof RegExpDollar
1284+
not exists(DataFlow::RegExpCreationNode regexp |
1285+
regexp.getRoot() = term.getRootTerm() and
1286+
maybeMultiline(regexp.tryGetFlags())
1287+
) and
1288+
(
1289+
exists(RegExpSequence seq | term = seq |
1290+
seq.getChild(0) instanceof RegExpCaret and
1291+
seq.getLastChild() instanceof RegExpDollar
1292+
)
1293+
or
1294+
isFullyAnchoredTerm(term.(RegExpGroup).getAChild())
1295+
or
1296+
isFullyAnchoredAlt(term, term.getNumChild())
12831297
)
1284-
or
1285-
isFullyAnchoredTerm(term.(RegExpGroup).getAChild())
1286-
or
1287-
isFullyAnchoredAlt(term, term.getNumChild())
12881298
}
12891299

12901300
/** Holds if the first `i` disjuncts of `term` are fully anchored. */
@@ -1330,6 +1340,8 @@ module RegExp {
13301340
*
13311341
* Specifically, whitelisting regexps such as `^(foo|bar)$` sanitize matches in the true case.
13321342
* Inverted character classes such as `[^a-z]` or `\W` sanitize matches in the false case.
1343+
* Anchored terms do not sanitize matches if a regular expression using the term has
1344+
* multiline or unknown flags.
13331345
*/
13341346
predicate isGenericRegExpSanitizer(RegExpTerm term, boolean outcome) {
13351347
term.isRootTerm() and

‎javascript/ql/test/library-tests/RegExpSanitizer/decorators.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import { Matches } from "class-validator";
2+
3+
class Validated {
4+
@Matches(/^\w+$/) // $ sanitizer
5+
singleLine: string;
6+
7+
@Matches(/^\w+$/m)
8+
multiline: string;
9+
10+
@Matches(new RegExp("^\\w+$")) // $ sanitizer
11+
constructedSingleLine: string;
12+
13+
@Matches(new RegExp("^\\w+$", "m"))
14+
constructedMultiline: string;
15+
16+
@Matches("^\\w+$", "i") // $ sanitizer
17+
stringSingleLine: string;
18+
19+
// Separate flags for string patterns are not currently covered by the sanitizer model.
20+
@Matches("^\\w+$", "m") // $ SPURIOUS: sanitizer
21+
stringMultiline: string;
22+
}

‎javascript/ql/test/library-tests/RegExpSanitizer/guards.js‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
function literals() {
2+
const value = source();
3+
sink(value); // $ flow
4+
5+
if (/^[\w.-]+$/m.test(value)) {
6+
sink(value); // $ flow
7+
}
8+
if (/^[\w.-]+$/.test(value)) {
9+
sink(value);
10+
} else {
11+
sink(value); // $ flow
12+
}
13+
if (/^[\w.-]+$/i.test(value)) {
14+
sink(value);
15+
}
16+
if (/^[\w.-]+$/s.test(value)) {
17+
sink(value);
18+
}
19+
if (/^[\w.-]+$/m.exec(value)) {
20+
sink(value); // $ flow
21+
}
22+
if (/^[\w.-]+$/.exec(value)) {
23+
sink(value);
24+
}
25+
if (value.match(/^[\w.-]+$/m)) {
26+
sink(value); // $ flow
27+
}
28+
if (value.match(/^[\w.-]+$/)) {
29+
sink(value);
30+
}
31+
if (value.match("^[\\w.-]+$")) {
32+
sink(value);
33+
}
34+
if (value.matchAll(/^[\w.-]+$/gm)) {
35+
sink(value); // $ flow
36+
}
37+
}
38+
39+
function references() {
40+
const value = source();
41+
const regexp = /^[\w.-]+$/m;
42+
if (regexp.test(value)) {
43+
sink(value); // $ flow
44+
}
45+
let match;
46+
if (match = regexp.exec(value)) {
47+
sink(value); // $ flow
48+
}
49+
}
50+
51+
function constructors(flags) {
52+
const value = source();
53+
if (new RegExp("^[\\w.-]+$", "m").test(value)) {
54+
sink(value); // $ flow
55+
}
56+
if (RegExp("^[\\w.-]+$", "im").test(value)) {
57+
sink(value); // $ flow
58+
}
59+
const multiline = "m";
60+
if (new RegExp("^[\\w.-]+$", multiline).test(value)) {
61+
sink(value); // $ flow
62+
}
63+
if (new RegExp("^[\\w.-]+$", flags).test(value)) {
64+
sink(value); // $ flow
65+
}
66+
if (new RegExp("^[\\w.-]+$").test(value)) {
67+
sink(value);
68+
}
69+
if (new RegExp("^[\\w.-]+$", "").test(value)) {
70+
sink(value);
71+
}
72+
if (new RegExp("^[\\w.-]+$", "i").test(value)) {
73+
sink(value);
74+
}
75+
if (new RegExp(/^[\w.-]+$/m).test(value)) {
76+
sink(value); // $ flow
77+
}
78+
if (new RegExp(/^[\w.-]+$/, "m").test(value)) {
79+
sink(value); // $ flow
80+
}
81+
}
82+
83+
function overriddenFlags() {
84+
const value = source();
85+
// Explicit overrides removing a literal's `m` flag are not currently covered by the sanitizer model.
86+
if (new RegExp(/^[\w.-]+$/m, "").test(value)) {
87+
sink(value); // $ SPURIOUS: flow
88+
}
89+
if (new RegExp(/^[\w.-]+$/m, "i").test(value)) {
90+
sink(value); // $ SPURIOUS: flow
91+
}
92+
}
93+
94+
function groupsAndAlternatives() {
95+
const value = source();
96+
if (/(^[\w.-]+$)/m.test(value)) {
97+
sink(value); // $ flow
98+
}
99+
if (/(^[\w.-]+$)/.test(value)) {
100+
sink(value);
101+
}
102+
if (/^foo$|^bar$/m.test(value)) {
103+
sink(value); // $ flow
104+
}
105+
if (/^foo$|^bar$/.test(value)) {
106+
sink(value);
107+
}
108+
}
109+
110+
function negativeChecks(flags) {
111+
const value = source();
112+
if (/[^a-z]/m.test(value)) {
113+
sink(value); // $ flow
114+
} else {
115+
sink(value);
116+
}
117+
if (!/\W/m.test(value)) {
118+
sink(value);
119+
}
120+
if (!new RegExp("[^a-z]", flags).test(value)) {
121+
sink(value);
122+
}
123+
}

‎javascript/ql/test/library-tests/RegExpSanitizer/routes.js‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
const cp = require("child_process");
2+
const app = require("express")();
3+
4+
app.get("/a", (req, res) => {
5+
const dir = req.query.dir;
6+
if (/^[\w.-]+$/m.test(dir)) {
7+
cp.exec("ls " + dir); // $ command-injection
8+
}
9+
res.send("ok");
10+
});
11+
12+
app.get("/b", (req, res) => {
13+
const dir = req.query.dir;
14+
if (/^[\w.-]+$/.test(dir)) {
15+
cp.exec("ls " + dir);
16+
}
17+
res.send("ok");
18+
});
19+
20+
app.get("/c", (req, res) => {
21+
cp.exec("ls " + req.query.dir); // $ command-injection
22+
res.send("ok");
23+
});
24+
25+
app.get("/d", (req, res) => {
26+
const name = req.query.name;
27+
if (/^\w+$/m.test(name)) {
28+
res.send("<p>Hello " + name + "</p>"); // $ reflected-xss
29+
} else {
30+
res.send("bad name");
31+
}
32+
});
33+
34+
app.get("/e", (req, res) => {
35+
res.send("<p>Hello " + req.query.name + "</p>"); // $ reflected-xss
36+
});

‎javascript/ql/test/library-tests/RegExpSanitizer/test.expected‎

Whitespace-only changes.

‎javascript/ql/test/library-tests/RegExpSanitizer/test.ql‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import javascript
2+
import utils.test.InlineExpectationsTest
3+
import semmle.javascript.frameworks.ClassValidator
4+
import semmle.javascript.security.dataflow.CommandInjectionQuery as CommandInjection
5+
import semmle.javascript.security.dataflow.ReflectedXssQuery as ReflectedXss
6+
7+
module TestConfig implements DataFlow::ConfigSig {
8+
predicate isSource(DataFlow::Node source) {
9+
source.(DataFlow::CallNode).getCalleeName() = "source"
10+
}
11+
12+
predicate isSink(DataFlow::Node sink) {
13+
sink = any(DataFlow::CallNode call | call.getCalleeName() = "sink").getArgument(0)
14+
}
15+
}
16+
17+
module TestFlow = TaintTracking::Global<TestConfig>;
18+
19+
module InlineTest implements TestSig {
20+
string getARelevantTag() { result = ["flow", "command-injection", "reflected-xss", "sanitizer"] }
21+
22+
predicate hasActualResult(Location location, string element, string tag, string value) {
23+
element = "" and
24+
value = "" and
25+
exists(DataFlow::Node node |
26+
location = node.getLocation() and
27+
(
28+
tag = "flow" and TestFlow::flowTo(node)
29+
or
30+
tag = "command-injection" and CommandInjection::CommandInjectionFlow::flowTo(node)
31+
or
32+
tag = "reflected-xss" and ReflectedXss::ReflectedXssFlow::flowTo(node)
33+
or
34+
tag = "sanitizer" and node = ClassValidator::sanitizingDecorator()
35+
)
36+
)
37+
}
38+
}
39+
40+
import MakeTest<InlineTest>

0 commit comments

Comments
 (0)