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: update 'decompress' #7319

Closed
vince-fugnitto opened this issue Mar 11, 2020 · 8 comments · Fixed by #8294
Closed

plugin-ext: update 'decompress' #7319

vince-fugnitto opened this issue Mar 11, 2020 · 8 comments · Fixed by #8294
Assignees
Labels
dependencies pull requests that update a dependency file quality issues related to code and application quality security issues related to security

Comments

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Mar 11, 2020

Description

The decompress dependency (part of plugin-ext) has a known security vulnerability which has been reported by the security-audit.

The issue is that decompress is not well-maintained (3 years since the last release) and it is unlikely that the flaw will be resolved. We should instead opt for a license compatible alternative which is maintained.


Update

decompress has been updated and no longer has a security advisory so there is no need to find an alternative, we can instead update the dependency to decompress >= 4.2.1

@vince-fugnitto vince-fugnitto added dependencies pull requests that update a dependency file quality issues related to code and application quality security issues related to security labels Mar 11, 2020
@vince-fugnitto
Copy link
Member Author

A suitable alternative might be adm-zip. @benoitf for the decompression (unzipping), are we to support different archive types? (ex: targz, zip,...). We will need to find an alternative that meets our requirements.

@benoitf
Copy link
Contributor

benoitf commented Mar 11, 2020

I think we're decompressing with zip for .theiaand .vsix but from npm registry, it's tarball/tgz

@vince-fugnitto
Copy link
Member Author

I think we're decompressing with zip for .theiaand .vsix but from npm registry, it's tarball/tgz

Thank you, we should find a suitable alternative that handles them all.

@akosyakov
Copy link
Member

@vince-fugnitto @marcdumais-work @spoenemann maybe it is time to publish next versions as preview extensions into our registry and drop npm support

paul-marechal added a commit that referenced this issue Mar 11, 2020
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

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
paul-marechal added a commit that referenced this issue Mar 11, 2020
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

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
paul-marechal added a commit that referenced this issue Mar 11, 2020
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

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
paul-marechal added a commit that referenced this issue Mar 11, 2020
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
kevva/decompress#71 (comment)

Fixes #7319

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
paul-marechal added a commit that referenced this issue Mar 12, 2020
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
kevva/decompress#71 (comment)

Fixes #7319

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
@marcdumais-work
Copy link
Contributor

maybe it is time to publish next versions as preview extensions into our registry and drop npm support

Does this refer to the fact that we initially published the VS Code builtins on npm? Are there other VS Code extensions published on npm that we know about? Anyway, will not npm/yarn take care of unzipping the archive downloaded from npm? Can we drop tar decompressing support for plugins and only support zip?

Note: .vsix look to be in zip format:

$ file rust-lang.rust-0.6.1.vsix
rust-lang.rust-0.6.1.vsix: Zip archive data, at least v2.0 to extract

@akosyakov
Copy link
Member

Does this refer to the fact that we initially published the VS Code builtins on npm? Are there other VS Code extensions published on npm that we know about?

Yes, it is only about built-ins.

Anyway, will not npm/yarn take care of unzipping the archive downloaded from npm? Can we drop tar decompressing support for plugins and only support zip?

We don't use npm/yarn to download anything.

@spoenemann Is there a way to publish a new version to Open VSX Registry but not like latest? What we need is to have for exactly the same unique id to versions one latest for tested version and one next to nightly build.

paul-marechal added a commit that referenced this issue Mar 13, 2020
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 <paul.marechal@ericsson.com>
@spoenemann
Copy link
Contributor

package.json has a preview field, should we use that to mark next versions?

@akosyakov
Copy link
Member

@spoenemann It sounds fine. I'm more concerned whether current registry api can provide a stable URI for latest and next versions? i.e. something like open-vsx.org/api/ms-vscode/bat for latest open-vsx.org/api/ms-vscode/bat?preview for next?

@vince-fugnitto vince-fugnitto changed the title plugin-ext: find an alternative dependency instead of 'decompress' plugin-ext: update 'decompress' Jul 28, 2020
@vince-fugnitto vince-fugnitto self-assigned this Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies pull requests that update a dependency file quality issues related to code and application quality security issues related to security
Projects
None yet
5 participants