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 CVE-2018-1000544 and disable symlinks to avoid other security issues #376

Merged
merged 11 commits into from Aug 31, 2018

Conversation

@jdleesmiller
Copy link
Member

@jdleesmiller jdleesmiller commented Aug 26, 2018

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

gem 'rubyzip',
  require: 'zip',
  git: 'https://github.com/jdleesmiller/rubyzip.git',
  ref: 'fix-cve-2018-1000544'

in the Gemfile are 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_path to Entry#extract. Allowing the caller to specify dest_path does 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#realdirpath which 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 like filepath-securejoin, https://github.com/envygeeks/pathutil, which may be useful. However, with the current API, it's difficult for rubyzip to know what the safe 'root' directory should be.

bdewater and others added 7 commits Aug 23, 2018
@coveralls
Copy link

@coveralls coveralls commented Aug 26, 2018

Coverage Status

Coverage decreased (-3.4%) to 95.372% when pulling fd81bd5 on jdleesmiller:fix-cve-2018-1000544 into e89f6ac on rubyzip:master.

@jdleesmiller
Copy link
Member Author

@jdleesmiller jdleesmiller commented Aug 26, 2018

Should also have mentioned that this is another attempt to fix #369.

@klausbadelt
Copy link

@klausbadelt klausbadelt commented Aug 27, 2018

@jdleesmiller could you bump the version? 1.2.1 still fails bundle audit. We could then use your PR branch until your PR is pulled in.

(I don't know how to add a PR to your PR...)

@@ -1,3 +1,3 @@
module Zip
VERSION = '1.2.1'
VERSION = '2.0.0'
Copy link

@rhymes rhymes Aug 27, 2018

@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

Copy link
Member Author

@jdleesmiller jdleesmiller Aug 27, 2018

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.

@jdleesmiller
Copy link
Member Author

@jdleesmiller jdleesmiller commented Aug 27, 2018

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.)

@thorsteneckel
Copy link

@thorsteneckel thorsteneckel commented Aug 27, 2018

JFI: Version bump didn't help resolve the bundle-audit issue. You have to exclude the CVE (after you applied the PR) via bundle-audit check --update --ignore CVE-2018-1000544

@everydayruby
Copy link

@everydayruby everydayruby commented Aug 27, 2018

@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 patched_versions?

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?

@thorsteneckel
Copy link

@thorsteneckel thorsteneckel commented Aug 27, 2018

@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 rubyzip as a dependency (e.g. trough selenium-webdriver for the test suite as we do) we needed a way to get on. Otherwise bundle-audit is blocking our build pipeline ☹️

@rhymes
Copy link

@rhymes rhymes commented Aug 27, 2018

@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"

@thorsteneckel
Copy link

@thorsteneckel thorsteneckel commented Aug 27, 2018

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 bundle-audit-db update 2 days ago. Let's move together in one direction 🚀 💪

@rhymes
Copy link

@rhymes rhymes commented Aug 27, 2018

@thorsteneckel oh sorry, I didn't know that. I just noticed 2 days ago when the builds stopped working :D

zammad-sync pushed a commit to zammad/zammad that referenced this issue Aug 27, 2018
…ubyzip/rubyzip#369) which only affects test env (selenium-webdriver dependency) until it's resolved (rubyzip/rubyzip#376).
zammad-sync pushed a commit to zammad/zammad that referenced this issue Aug 27, 2018
…ubyzip/rubyzip#369) which only affects test env (selenium-webdriver dependency) until it's resolved (rubyzip/rubyzip#376).
@eliasjpr
Copy link

@eliasjpr eliasjpr commented Aug 27, 2018

Whats the ETA of having this patch published?

@PoslinskiNet
Copy link

@PoslinskiNet PoslinskiNet commented Aug 29, 2018

Any news on that?

@bdewater
Copy link
Contributor

@bdewater bdewater commented Aug 29, 2018

@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!

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