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.

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

fix!: Correct mishandled escaped path separators #34

Merged
merged 1 commit into from May 3, 2021
Merged

Conversation

@Trott
Copy link
Contributor

@Trott Trott commented Feb 3, 2021

This change fixes the regular expression denial of service
vulnerability.

Refs: https://app.snyk.io/vuln/SNYK-JS-GLOBPARENT-1016905

@Trott
Copy link
Contributor Author

@Trott Trott commented Feb 3, 2021

With this change, at least locally for me:

  • All tests continue to pass.
  • The following proof-of-concept runs quickly (on the order of milliseconds) whereas with the current regular
    expression, it freezes up for a long time. (I hit control C after maybe five or ten seconds. Not sure how long it
    would run to finish. Don't care, honestly.)
    var globParent = require('./index.js');
    
    globParent('{' + '/'.repeat(5000));

@Trott
Copy link
Contributor Author

@Trott Trott commented Feb 3, 2021

And, because regular expressions can be confusing, here's how the refactor went.

Original regexp: /[\{\[].*[\/]*.*[\}\]]$/

The part that was changed is this: [\/]*.*

So that's basically two things next to each other: [\/]* followed by .*.

That first one is "zero or more forward slashes and/or back slashes". That second one is "zero or more of anything".

So it amounts to "Zero or more forward slashes and/or back slashes, followed by zero or more of anything".

"Zero or more of anything" already includes any possible "zero or more forward slashes and/or back slashes" (which is why the regular expression engine ties itself in knots if there are a ton of slashes in a row) and so I replaced [\/]*.* with just .*. Should do the exact same thing, but with a lot less work on the part of the regular expression engine.

@Trott
Copy link
Contributor Author

@Trott Trott commented Feb 3, 2021

Seems like a test case can be added that will time out on current main but pass with this change. So I've added that.

@phated
Copy link
Member

@phated phated commented Feb 3, 2021

@Trott Awesome! And thank you for the thorough explanation 😄 I believe your solution fixes that, but I think our tests are lacking around this regexp because it is only supposed to match if an enclosure { } or [ ] contains a slash (as mentioned in #32)

I think that needs to be fixed and more tests added to make sure we are correctly handling that case.

@Trott
Copy link
Contributor Author

@Trott Trott commented Feb 3, 2021

@phated Would it be possible to separate the two issues?:

  • Fix the ReDoS
  • Make the regular expression and tests more robust/correct

If this lands, it addresses the first bullet point and there is a test so that the ReDoS issue doesn't return as y'all work through the second bullet point.

@phated
Copy link
Member

@phated phated commented Feb 3, 2021

No, because that further obscures the meaning of this code branch. As you've likely noticed in the other issue thread, no one knows what is going on.

@Trott
Copy link
Contributor Author

@Trott Trott commented Feb 4, 2021

OK, i've updated this based on the conversation in #32.

New commit message:

fix: eliminate ReDoS

This change fixes the regular expression denial of service
vulnerability.

This also fixes some incorrect tests that concealed a bug.

Fixes: https://github.com/gulpjs/glob-parent/issues/32
Refs: https://app.snyk.io/vuln/SNYK-JS-GLOBPARENT-1016905

@Trott
Copy link
Contributor Author

@Trott Trott commented Feb 4, 2021

So, instead of removing the detection for / in the reg exp, this now changes the "zero or more slashes" to be "one slash surrounded by other stuff".

So, before, the relevant segment was .*[\/]*.*. Now, it's .*\/.*. This has two effects. First, it gets rid of the ReDoS. Second, it fixes this segment of the regular expression so it detects a / rather than matching regardless of whether or not there is a slash.

This made four tests fail, but based on the conversation in the issue, those four tests were wrong. They now instead return the results per expectations. See #32 (comment) and subsequent comments.

@Trott
Copy link
Contributor Author

@Trott Trott commented Feb 10, 2021

I know this is open source and you don't owe it to me or anyone else, but is there anything more I should do to get this reviewed? Based on the conversation in the related issue, I believe this addresses all the outstanding concerns.

@phated
Copy link
Member

@phated phated commented Feb 10, 2021

@Trott Sorry for the delay, I don't mean to make you wait. I've been thinking about your comment about making a breaking release that changes this due to the test suite changing. Even though/if I'm very confident this was a bug with our test suite, it still probably needs to be released in a major.

I'm considering landing your original patch in the current major (since it makes the suite pass) and then doing a major that updates scaffold, fixes this incorrect behavior, and drops older node support.

@andyedwardsibm
Copy link

@andyedwardsibm andyedwardsibm commented Feb 10, 2021

@phated is there any plan to back-port the core fix to previous versions of glob-parent? I couldn't find a definitive statement on whether previous major versions are supported for some time, or whether only the latest major version is supported

@Trott
Copy link
Contributor Author

@Trott Trott commented Feb 10, 2021

I'm considering landing your original patch in the current major (since it makes the suite pass) and then doing a major that updates scaffold, fixes this incorrect behavior, and drops older node support.

@phated I've opened #36 which is the original version of this that fixes the ReDoS but does not change any behavior. If you land that and publish it in a patch release, I'll rebase this against that change and then this can be landed as a breaking change.

@Trott Trott mentioned this pull request Feb 19, 2021
@phated
Copy link
Member

@phated phated commented Mar 6, 2021

@Trott I updated the testing patterns with the new scaffold, so there are some conflicts.

@Trott Trott force-pushed the snyk-fix branch 2 times, most recently from 82fd0db to 9a725b0 Mar 7, 2021
@Trott
Copy link
Contributor Author

@Trott Trott commented Mar 7, 2021

@Trott I updated the testing patterns with the new scaffold, so there are some conflicts.

All conflicts resolved.

I guess the commit message should be updated, since this doesn't fix any ReDoS anymore. That happened in #36. This now updates the behavior to be correct, but may also be a breaking change if anyone is relying on the old behavior. I'll try to come up with a reasonable commit message and force push.

@Trott Trott changed the title fix: eliminate ReDoS fix: fix mishandled escaped path separators Mar 7, 2021
@Trott
Copy link
Contributor Author

@Trott Trott commented Mar 7, 2021

I changed the commit message to "fix: fix mishandled escaped path separators" but didn't include any details because it's all in #32 which is referred to in the commit metadata. Hopefully that's good enough, but if not, hey, I'm please edit (you should be able to force push to my branch) or tell me what it should say and I'll edit it.

I also didn't include "breaking change" anywhere in the commit message. Not sure if you plan to treat this as a bug fix or a breaking change. I can add that too, of course, or you can go for it.

@Trott
Copy link
Contributor Author

@Trott Trott commented Mar 7, 2021

I also didn't include "breaking change" anywhere in the commit message. Not sure if you plan to treat this as a bug fix or a breaking change. I can add that too, of course, or you can go for it.

I see in your comment over in #36 that you do intend to release this as a major, so I've changed fix: to fix!: per conventional commits, although you don't use conventional commits on this project. I'm not sure why I feel the need to do this and leave a comment, but here we are. Oh well. Change it any way you want of course. Thanks.

@phated
Copy link
Member

@phated phated commented Mar 17, 2021

Thanks @Trott! I'm trying to finish up #39 which would also be a breaking change so it'll be good to batch them together.

@Trott
Copy link
Contributor Author

@Trott Trott commented Apr 28, 2021

Thanks @Trott! I'm trying to finish up #39 which would also be a breaking change so it'll be good to batch them together.

It looks like #39 may have stalled. If there's anything I can do to nudge this PR forward, let me know!

@phated phated changed the title fix: fix mishandled escaped path separators fix!: Correct mishandled escaped path separators May 3, 2021
@phated
Copy link
Member

@phated phated commented May 3, 2021

@Trott Thanks for the ping, I closed out the other issue since everyone was talking past each other. I'll merge this now.

@phated phated merged commit 32f6d52 into gulpjs:main May 3, 2021
15 of 21 checks passed
@github-actions github-actions bot mentioned this pull request May 3, 2021
@phated
Copy link
Member

@phated phated commented May 3, 2021

Published as 6.0.0 - thanks for all your hard work and patience @Trott!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants