Add eslint plugin for detecting redos vulnerabilities and tweak regex ip regex to prevent backtracks #2849
Conversation
and use a function for validating ipv6 instead of trying to do it in a single regex
✅ Deploy Preview for guileless-rolypoly-866f8a ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
tjenkinson
changed the title
|
|
||
| const ipv4Regex = | ||
| /^(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([0-9]{1,2}))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([0-9]{1,2}))$/; | ||
| /^(?:(?:(?=(25[0-5]))\1|(?=(2[0-4][0-9]))\2|(?=(1[0-9]{2}))\3|(?=([0-9]{1,2}))\4)\.){3}(?:(?=(25[0-5]))\5|(?=(2[0-4][0-9]))\6|(?=(1[0-9]{2}))\7|(?=([0-9]{1,2}))\8)$/; |
There was a problem hiding this comment.
The behaviour should be identical but this uses lookaheads and references make the matches atomic, which prevents backtracks
- Before: Graph, Redos detector
- After: Graph, Redos detector
Without this the lint plugin flags this as a potential issue
There was a problem hiding this comment.
It looks like only redos detector flags this regex. I can't produce any exploit string. Can you?
(I tried Rescue and regexploit)
from manually looking into it, I also can't see how infinite backtracing would be produced, as one part is capped to 3 groups, and the last part is limited to one (with 2 subgroups having at max 2 backtraces), so any explanation would be appreciated
There was a problem hiding this comment.
As mentioned on fabian-hiller/valibot#202 (comment), there are more backtracks possible without the atomic group polyfll here. Probably not a concerning number though. So the question is whether or not having the pattern like this where the redos checker can be confident there is no issue is worth it looking slightly more complicated, vs disabling the rule and having a simpler pattern :)
| const dateTimeRegexPrecision0Offset = | ||
| /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(([+-]\d{2}(:?\d{2})?)|Z)$/; | ||
| const dateTimeRegexPrecision0NoOffset = | ||
| /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$/; | ||
| const dateTimeNoPrecisionOffset = | ||
| /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?(([+-]\d{2}(:?\d{2})?)|Z)$/; | ||
| const dateTimeNoPrecisionNoOffset = | ||
| /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?Z$/; | ||
|
|
There was a problem hiding this comment.
I moved these to regex literals instead of the RegExp instance because the lint plugin doesn't support RegExp and these could be literals. Also only creating them once now which should be more efficient
| } | ||
| } | ||
| }; | ||
|
|
||
| function isValidIpv6(ip: string): boolean { |
There was a problem hiding this comment.
I moved this to a function because the regex was very complicated and the linter was flagging it as a potential problem
| [ | ||
| "1e5e:e6c8:daac:514b:114b:e360:d8c0:682c", | ||
| "9d4:c956:420f:5788:4339:9b3b:2418:75c3", | ||
| "9d4:c956:420f:5788:4339:9b3b:2418:75C3", |
There was a problem hiding this comment.
previously it only allowed lowercase, but uppercase is still a valid ip
|
Thanks for the great work and I appreciate this PR, but Zod's actually moving away from eslint imminently, so I'm going to close this. Zod also now relies on a fair number of dynamically generated regexes, so I think the best approach to addressing this issue is adding runtime tests using https://www.npmjs.com/package/redos-detector or similar, which I'll do shortly in the v4 branch. |
|
Sounds good. Let me know if you hit any issues :) |
|
Runtime might not be good for performance though |
This adds and configures
eslint-plugin-redos-detector(fyi I'm the author of that), and then updates the ipv4 regex so that it's no longer flagged as a potential problem and moves the ipv6 check to a function which I think is a bit easier to follow, and doesn't trigger the linter.This would have caught the email redos (2ba00fe), but better late than never?
Let me know what you think :)