-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixed regexes to avoid ReDoS attacks #10
fixed regexes to avoid ReDoS attacks #10
Conversation
// 'root' is just a slash, or nothing. | ||
var splitPathRe = | ||
/^(\/?|)([\s\S]*?)((?:\.{1,2}|[^\/]+?|)(\.[^.\/]*|))(?:[\/]*)$/; | ||
/^((\/?)(?:[^\/]*\/)*)((\.{1,2}|[^\/]+?|)(\.[^.\/]*|))[\/]*$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look like breaking changes to me, and I am not entirely sure if this fixes the DoS. I did a little work on this issue. It is tempting to fix the groups, the whitespace (\s\S), etc., but I found that the existing unit tests were incomplete (even if some are unrealistic).
I found that these tests needed to be added:
var regexDosFileNameLength = 50000;
var regexDosFileName = Buffer.alloc(regexDosFileNameLength, '/').toString() + 'file.txt';
[{ root: '/', dir: '/home/user/dir', base: 'file.txt', ext: '.txt', name: 'file' }, '/home/user/dir/file.txt/'],
[{ root: '/', dir: '/\n\thome/user/dir', base: 'file.txt', ext: '.txt', name: 'file' }, '/\n\thome/user/dir/file.txt/'],
[{ root: '/', dir: '/////////', base: 'file.txt', ext: '.txt', name: 'file' }, '//////////file.txt'],
[{ root: '/', dir: '/', base: '', ext: '', name: '' }, '//////////'],
[{ root: '/', dir: Buffer.alloc(regexDosFileNameLength - 1, '/').toString(), base: 'file.txt', ext: '.txt', name: 'file' }, regexDosFileName],
];
If you want these to be breaking changes, then fine. The regexDosFileNameLength
is specifically to test the regex DoS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can pass those tests with
var splitPathRe =
/^((\/?)(?:[^\/]*\/)*)((\.{1,2}|[^\/]+?|)(\.[^.\/]*|))(?:(?<=^\/)|(?<!\/))[\/]*$/;`
return {
// ...
dir: allParts[0] === '/' ? allparts[0] : allParts[0].slice(0, -1),
// ...
}
But as thrilling as it is to have a single regex handling the parsing, it'd be much wiser to clean up the parameter (e.g. by stripping trailing slashes, while retaining a leading slash) and keeping the regex simple.
And as for avoiding the ReDoS, my testing shows a significant improvement:
attack_str.length: 1970001: 332 ms
attack_str.length: 1980001: 359 ms
attack_str.length: 1990001: 385 ms
attack_str.length: 2000001: 295 ms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.npmignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.npmignore
Replaces
[\s\S]*?
with less explosive alternatives.