Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Fix ReDoS vulnerability CVE-2021-35065 #49

Merged
merged 4 commits into from Jul 20, 2021
Merged

fix: Fix ReDoS vulnerability CVE-2021-35065 #49

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d16d629
fix: Fix ReDoS vulnerability CVE-2021-35065
sttk Jun 24, 2021
723eed9
test: Add a test case for ReDoS vulnerability
sttk Jun 24, 2021
9bc351f
fix: Modify regex to accept newlines and to avoid ReDoS by '{'.repeat(n)
sttk Jun 26, 2021
468dde3
fix: Modify to get rid of RegExp `enclosure`
sttk Jul 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.js
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@ var isWin32 = require('os').platform() === 'win32';

var slash = '/';
var backslash = /\\/g;
var enclosure = /[{[].*\/.*[}\]]$/;
var enclosure = /[{[][^/\r\n\u2028\u2029]*\/.*[}\]]$/;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this suffice (and be a fair bit easier to read as well)?

Suggested change
var enclosure = /[{[][^/\r\n\u2028\u2029]*\/.*[}\]]$/;
var enclosure = /[{[][^/]*\/.*[}\]]$/;

If not, we should probably add a test case that shows it not working.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott The direct solution of this issue is the way as you suggested. But the reporter suggested this solution and we agree with it because we can't accept newlines in a filepath.

Copy link
Contributor

@Trott Trott Jun 24, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the decision is "we won't accept newlines in a path", then OK. But if the belief is "newlines cannot legitimately appear in a path", then I do not believe that is correct. See https://superuser.com/a/129532/81159. Or try this (which is basically the same thing as in that linked answer):

$ touch 'foo
quote> bar'
$ ls -lbd foo*
-rw-r--r--  1 trott  staff  0 Jun 24 07:04 foo\nbar
$

(The quote> thing is zsh console output. Here's basically the same thing in Bourne shell instead.)

sh-3.2$ touch 'foo
> bar'
sh-3.2$ ls foo*
foo?bar
sh-3.2$ ls -ldb foo*
-rw-r--r--  1 trott  staff  0 Jun 24 07:08 foo\nbar
sh-3.2$

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a onetime file system tester, I'm with @Trott here: newlines are valid path elements in many file systems.

Copy link

@davisjam davisjam Jun 24, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex is much improved in its performance. However. I did not check the context in which it is used, but if one of the "match a substring within this string" APIs is used then the performance can still be problematic.

Try for example:

let match = '{'.repeat(10*10*10*10*5);

console.log('matching');
var res = /[[{[][^/\r\n\u2028\u2029]*\/.*[}\]]$/.exec(match);
console.log('done matching')
console.log(res)

(This is slow in a Node v12 REPL I found on the web).

The slow performance is because:

  1. The "match anywhere" will basically try starting from each of n open-braces
  2. There is a reachable quantifier that also matches a { (the first *'d group).
  3. So the regex engine will match O(n) of them before rejecting each time -- that makes for O(n^2) behavior.

So, food for thought:

  1. How long are the strings that this regex processes? (In the example above, I'm using a string with 50K characters in it. For shorter strings, the performance is "not great", but perhaps not catastrophic).
    2.Are there any additional constraints on the input we could add in? (e.g. enforcing a maximum length, or additional properties that the regex can check) I think there is a possibly-illegible refactoring that would do the trick, but it would be nicer if there is a more maintainable option.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the project again and it seems like the regex /(^|[^\\])([{[]|\([^)]+$)/ has the same problem for all inputs "(".repeat(n).

Whatever solution is chosen for /[[{[][^/\r\n\u2028\u2029]*\/.*[}\]]$/ should also be applied to /(^|[^\\])([{[]|\([^)]+$)/.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine that ideally, this wouldn't be doing regex matching at all but would instead use some module that does proper parsing for globs. Although I guess there really isn't a single globbing standard to adhere to. Anyway, if no one's looked yet, it might be worth looking at what's out there (to make sure they just don't do regexp matching under the hood).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to use a linear-time regex engine. For example, you could add re2 as a (somewhat heavy, alas) dependency. It supports all of the features used in these regexes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Please @ me if you want further opinions)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RunDevelopment I think that regex has no problem.

% cat test.js
var performance = require('perf_hooks').performance;
var globby = /(^|[^\\])([{[]|\([^)]+$)/;
var st = performance.now();
globby.test("{" + "/".repeat(500000));
var ed = performance.now();                                                     
console.log(ed - st);
% node test.js
0.7820440530776978
%

var globby = /(^|[^\\])([{[]|\([^)]+$)/;
var escaped = /\\([!*?|[\](){}])/g;

Expand Down
22 changes: 22 additions & 0 deletions test/index.test.js
View file
Open in desktop
Expand Up @@ -4,6 +4,8 @@ var gp = require('../');
var expect = require('expect');
var isWin32 = require('os').platform() === 'win32';

var performance = require('perf_hooks').performance;

describe('glob-parent', function () {
it('should strip glob magic to return parent path', function (done) {
expect(gp('.')).toEqual('.');
Expand Down Expand Up @@ -224,6 +226,26 @@ describe('glob2base test patterns', function () {

done();
});

it('should not increase calc. time exponentially by \'/\' count [CVE-2021-35065]', function (done) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment says "exponential". This regex will exhibit polynomial growth, not exponential growth.

Copy link
Contributor Author

@sttk sttk Jun 26, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I'll modify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this case and add a new case to check absolute time.

var measure = function(n) {
var input = "{" + "/".repeat(n);
var st = performance.now();
gp(input);
var ed = performance.now();
return (ed - st) / (n * n);
};

var result0 = measure(5000);

[50000, 500000].forEach(function(n) {
var result1 = measure(n);
expect(result1 / result0).toBeLessThan(0.9);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly you should also check for a "reasonable" absolute matching time, instead of just checking for a low-enough relative increase. For example, you could also assert that the 500K string finishes in under 1 second.

Copy link
Contributor Author

@sttk sttk Jun 26, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I'll add that assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this case and add a new case to check absolute time.

result0 = result1;
});

done();
});
});

if (isWin32) {
Expand Down