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

plugin-ext: validate path when unpacking archives #7322

Closed
wants to merge 1 commit into from

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Mar 11, 2020

What it does

Fix zip-slip by validating where a given file will be unpacked. If the
expected path is outside of the destination folder: log a warning and
ignore the file.

Fixes #7319

How to test

Tests should fail if you comment lines 32 to 44 in plugin-deployer-file-handler-context-impl.ts.

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added the quality issues related to code and application quality label Mar 11, 2020
@paul-marechal paul-marechal force-pushed the mp/decompress branch 3 times, most recently from 19491bf to 94fbe5b Compare March 11, 2020 23:06
@akosyakov akosyakov added plug-in system issues related to the plug-in system security issues related to security and removed quality issues related to code and application quality labels Mar 12, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

While I'm fine with having safety measures for zip-slip (arbitrary file-write) including the additional tests, I think we should still look for a replacement to decompress. The project is not actively maintained and will likely cause more issues going forward. In addition, since the flaw is not patched in decompress, the security vulnerability will still show up for @theia/plugin-ext. Also, if someone still happens to use decompress in another area of the code, it will not benefit from the same patching.

It looks like we only need a dependency capable of handling zip archives so it should be easy to find a maintained alternative.

cc @marcdumais-work

@paul-marechal
Copy link
Member Author

paul-marechal commented Mar 12, 2020

TBH I don't really care. I like decompress because it handles several extensions at the time. The vulnerability only happens when you let the library unpack without validating anything on the user side, which this PR adds.

I'll still update this PR based on Anton's comment, feel free to open a separate PR that would replace the dependency by something else if this is an hard requirement.

@marcdumais-work
Copy link
Contributor

Also, if someone still happens to use decompress in another area of the code, it will not benefit from the same patching.

I find this to be a compelling argument. If we keep decompress it will always show-up as a known vulnerability for the platform and applications that use plugin-ext. We'll need to explain that we took steps so that the vulnerability is not triggered for our use-case, and that it can be safely ignored. After a little while we will just ignore it, knowing that "it's ok in this case", which can lead to problems if the library gets used in other places without taking these protective steps.

Fix zip-slip by validating where a given file will be unpacked. If the
expected path is outside of the destination folder: log a warning and
ignore the file.

This commit includes an archive that will trigger the exploit by writing
a file to `/tmp/slipped.txt`. This comes from magicOz, who reported the
vulnerability on kevva's decompress repository (issue 71).

Fixes #7319

Signed-off-by: Paul Maréchal <[email protected]>
@paul-marechal
Copy link
Member Author

Preferred way is to change the dependency.

@paul-marechal paul-marechal deleted the mp/decompress branch February 25, 2021 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system security issues related to security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plugin-ext: update 'decompress'
4 participants