Fix CVE-2018-1000544 and disable symlinks to avoid other security issues #376
Conversation
Small refactor along the way to centralize destination handling when no explicit path is given and a potential malicious one from the zipfile is used
Not sure if the exception is the right way to go
|
Coverage decreased (-3.4%) to 95.372% when pulling fd81bd5 on jdleesmiller:fix-cve-2018-1000544 into e89f6ac on rubyzip:master. |
|
Should also have mentioned that this is another attempt to fix #369. |
|
@jdleesmiller could you bump the version? 1.2.1 still fails (I don't know how to add a PR to your PR...) |
lib/zip/version.rb
Outdated
| @@ -1,3 +1,3 @@ | |||
| module Zip | |||
| VERSION = '1.2.1' | |||
| VERSION = '2.0.0' | |||
There was a problem hiding this comment.
@jdleesmiller can you please revert this? Or at least release a patch version of 1.2 as well?
The thing is all the libraries we have in Gemfile.lock either explicitly declare < 2.0.0 or ~> 1.2
There was a problem hiding this comment.
Hmm. That is a good point about version constraints.
My rationale was that dropping support for symlinks completely is a breaking change, and I'd be unhappy if semver minor or patch release broke my app.
If you have ~> 1.2, I guess 1.3.0 should do it. It's not very semantic, but it does seem like the pragmatic solution. Happy to try it out.
|
OK, I've bumped the version to 1.2.2. I think this should really be a semver major version bump, because dropping support for symlinks completely is a breaking change. However, as @rhymes pointed out, that will probably cause more problems than it's worth at this point for people looking to apply this as a hotfix. (And I'm not sure what's going on with coveralls. Changing a version number shouldn't affect the coverage numbers.) |
|
JFI: Version bump didn't help resolve the |
|
@thorsteneckel Thanks for the information. I'm not that familiar with bundler-audit/ruby-advisory-db but is that because https://github.com/rubysec/ruby-advisory-db/blob/master/gems/rubyzip/CVE-2018-1000544.yml doesn't include Does ruby-advisory-db need to adjust patched_versions after this PR is pulled and version 1.2.2 is released, in order for bundle-audit to pass without ignoring the CVE? |
|
@everydayruby - yes. You are absolutely right. But since there is not much traction regarding this CVE in this repository and some of us rely heavily on |
|
@thorsteneckel it happened on a weekend and this PR is from 18 hours ago, I don't think it's fair to say there's "not much traction in this repository" |
|
Sorry, it wasn't meant to sound rude. I was referring to the CVE not the PR: The CVE is public now for more than a month. However, the audience grew with the |
|
@thorsteneckel oh sorry, I didn't know that. I just noticed 2 days ago when the builds stopped working :D |
…ubyzip/rubyzip#369) which only affects test env (selenium-webdriver dependency) until it's resolved (rubyzip/rubyzip#376).
…ubyzip/rubyzip#369) which only affects test env (selenium-webdriver dependency) until it's resolved (rubyzip/rubyzip#376).
|
Whats the ETA of having this patch published? |
|
Any news on that? |
|
@jdleesmiller thanks for picking up the work 🙌 @simonoff you seem to be the sole maintainer of this gem and are undoubtedly busy with other things 🙂but with automated security advisories out a lot of people are eagerly awaiting a new release with a fix. Any feedback on this PR to get it over the finish line would be much appreciated! |
unzipping was broken due to fixes for CVE-2018-1000544 in rubyzip: [1], [2]. Also, see [3]. Fixes tdtds#32. [1]: rubyzip/rubyzip#371 [2]: rubyzip/rubyzip#376 [3]: rubyzip/rubyzip#354
Unzipping was broken due to fixes for CVE-2018-1000544 in rubyzip: [1], [2]. Also, see [3]. [1]: rubyzip/rubyzip#371 [2]: rubyzip/rubyzip#376 [3]: rubyzip/rubyzip#354
With apologies for opening another PR... this PR builds on #371.
As mentioned in #371 (comment) , there are still security problems with symlinks in #371 .
So, I've had a go at fixing the symlink issues... by just disabling symlink extraction altogether. I don't need symlink support, so this is an OK solution for me. I'm not sure whether it will be in general; feel free to close / cherry pick bits of this PR if not. I think this would require a major version bump. (It's also notable that, as mentioned in #371 (comment), there are no tests for non-malicious symlinks at present.)
Tests on my apps with
in the
Gemfileare green.Compared to #371, this PR also includes some test reorganisation and changes the name validation code to use some built-in ruby methods, rather than regular expressions. Hopefully that is more portable.
Notes
As noted in #371 (comment), the name validation still does nothing to protect the caller if they pass in
dest_pathtoEntry#extract. Allowing the caller to specifydest_pathdoes give it a lot of flexibility, but it is also a bit of a foot-gun, unfortunately.I think the approach in #371 for checking that symlink targets are relative and do not contain
..constructions is probably workable, and would be an improvement vs banning symlinks altogether, but it is still likely to reject some valid inputs, where symlinks contain..but still resolve inside the archive.A general solution (e.g. like the one in progress on mholt/archiver#70) seems like it probably has to involve something like https://github.com/cyphar/filepath-securejoin (in Go), which can tell you whether a path resolves within a particular 'root' directory, even in the presence of symlinks. Apparently doing it in a cross-platform way is hard (at least in Go): golang/go#20126 .
In ruby, there is
Pathname#realdirpathwhich is almost capable of doing this, but it errors on paths that don't exist, which may be a problem when we're trying to create paths and files from a zip. There is a utility library that looks like it implements something more likefilepath-securejoin, https://github.com/envygeeks/pathutil, which may be useful. However, with the current API, it's difficult forrubyzipto know what the safe 'root' directory should be.