Basically, this would mean recognizing preg_* and similar functions as EXEC. What taint type, I don't know. I don't think there's a proper way to sanitize user input in regexps, so probably a new type that cannot be cleared. Low priority because this will likely have a ton of false positives.
Description
Description
Details
Details
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Implement ReDoS detection | mediawiki/tools/phan/SecurityCheckPlugin | master | +119 -21 |
Related Objects
Related Objects
- Mentioned In
- rMTPSb49dedfc6541: Implement ReDoS detection
- Mentioned Here
- T187669: Static PCRE ReDoS validator
T240884: RFC: How to evaluate user-provided regular expressions
T176312: Don’t check format constraint via SPARQL (safely evaluating user-provided regular expressions)
T214378: Check simple format constraints (no grouping) in PHP instead of SPARQL
Event Timeline
Comment Actions
I don't think there's a proper way to sanitize user input in regexps, so probably a new type that cannot be cleared.
From what I know, there are filtering and allow-lists which can be somewhat effective, but naive solutions like that quickly fail to accommodate for the entire regex grammar. Such solutions (and more elaborate ones) have been discussed for a few different projects from time-to-time (e.g. T214378, T176312).
Comment Actions
Yeah, I also remember some releated discussions at T187669 and T240884.
From what I know, there are filtering and allow-lists which can be somewhat effective, but naive solutions like that quickly fail to accommodate for the entire regex grammar.
Exactly. I'm not even sure where the regex grammar sits in the Chomsky hierarchy, as it also depends on the regex flavour. At any rate, it's certainly impossible to sanitize a regexp without tokenizing it, but I don't think many people do that...
So, this should have the MISC taint type, which is the same type used for eval, require, and similar constructs for which you usually cannot sanitize the input.
Comment Actions
In addition to some of the external research mentioned on these tasks, I feel like @Bawolff sent me this other paper on potential static analysis for ReDoS prevention, which I, um, haven't quite digested just yet.
Comment Actions
Change 644800 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Implement ReDoS detection
Comment Actions
Change 644800 merged by jenkins-bot:
[mediawiki/tools/phan/SecurityCheckPlugin@master] Implement ReDoS detection
Comment Actions
This might have a lot of false positives (haven't tested on mw-core), but if this is the case, I guess we can just disable it like we do for the SerializeInjection issue.