Fix CVE-2018-1000544 #371

bdewater · 2018-07-01T19:55:02Z

Attempt to fix #369

Not sure if there's a better way to handle Errno::ENOENT in the symlink case, open to suggstions.

coveralls · 2018-07-01T20:05:18Z

Coverage decreased (-2.3%) to 94.268% when pulling 7720b14 on bdewater:fix-cve-2018-1000544 into 8887b70 on rubyzip:master.

coveralls · 2018-07-01T20:05:18Z

Coverage decreased (-4.08%) to 94.718% when pulling 0586329 on bdewater:fix-cve-2018-1000544 into e89f6ac on rubyzip:master.

bdewater · 2018-07-01T20:26:07Z

CI failures are unrelated (JRuby installation 404s)

JackMc

A few comments, but this looks good to me

JackMc · 2018-07-01T20:01:33Z

lib/zip/entry.rb

-      if @name.squeeze('/') =~ /\.{2}(?:\/|\z)/
+    def extract(dest_path = nil, &block)
+      if dest_path.nil? && Pathname.new(@name).absolute?
+        puts "WARNING: skipped absolute path in #{@name}"


Is putsing a warning the best thing (most extensible) way to do things? I recognize that's what the previous code did, but it seems off to me. Maybe we should raise, or is there some kind of error reporting mechanism that we can surface to the user that's zipfile-wide?

Either way feel free to say that this is out of scope :)

JackMc · 2018-07-02T04:14:36Z

lib/zip/entry.rb

        puts "WARNING: skipped \"../\" path component(s) in #{@name}"
        return self
+      elsif symlink? && get_input_stream.read =~ %r{../..}


Could you add a test that this still works for non-malicious symlinks? It doesn't appear there is one.

The comment on get_input_stream's definition seems to also say that it doesn't work too great for symlinks. We should make sure we're not adding any undefined behaviour: https://github.com/rubyzip/rubyzip/blob/master/lib/zip/entry.rb#L484

Symlink could also links to absolute path
You can find examples of such zip files in https://github.com/jwilk/path-traversal-samples

zeha · 2018-08-12T15:59:57Z

Given this doesn't look too broken, can we please have that merged and a new release stamped with it? Thanks :-)

zeha · 2018-08-15T17:15:42Z

lib/zip/entry.rb

+      if dest_path.nil? && Pathname.new(@name).absolute?
+        puts "WARNING: skipped absolute path in #{@name}"
+        return self
+      elsif @name.squeeze('/') =~ /\.{2}(?:\/|\z)/


Should this not also gain the dest_path.nil? check?

zeha · 2018-08-15T17:33:58Z

Example in README calls .extract() with the name from the zipfile, leading to the same vulnerability in code that gets copy-pasted from the README.

drn · 2018-08-21T18:43:11Z

Our team would love to see this merged and released as well. This has started to show up in our automated vulnerability analysis (https://hakiri.io/technologies/rubyzip/issues/1e6c19ff65e713) and our builds are red as a result.

Happy to help get this over the line if any additional work is needed. Please let me know!

bdewater · 2018-08-21T18:50:12Z

I can address feedback to this PR - @simonoff have you had a chance to look at this?

Right now do not have the bandwidth available to look at/add tests for all the examples in https://github.com/jwilk/path-traversal-samples - so if somebody feels like taking a crack at it you're most welcome 🙂

simonoff · 2018-08-22T09:49:50Z

Tests is failed. Need to have a green build to make it merged.

bdewater · 2018-08-23T19:27:01Z

@simonoff like I mentioned the test failures seem unrelated to this change - can you try to rebuild please?

Edit: Jruby is failing on master still.

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

reedloden · 2018-08-25T02:09:36Z

This was just added to ruby-advisory-db, so would love for this PR to be finalized, landed, and released.

vipulnsward · 2018-08-26T02:40:34Z

I looked at why the jruby builds were failing. Its after an upgrade to 9.2 affecting the crc32 checksum in Encryption tests.
We can ignore failures on JRuby, since its a difference in MRI and JRuby implementation for now.

bdewater · 2018-08-26T04:06:33Z

Thanks @vipulnsward - I've pulled your commit from #375 in to get this PR green.

jdleesmiller · 2018-08-26T09:05:05Z

Based on #371 (comment) and #371 (comment) I added tests for https://github.com/jwilk/path-traversal-samples

Tests are in: jdleesmiller@9c468f3

Good news: most pass.

Bad news: two don't pass: test_directory_symlink and test_two_directory_symlinks_b:

  1) Failure:
PathTraversalTest#test_two_directory_symlinks_b [/Users/john/ex/rubyzip/test/path_traversal_test.rb:85]:
Expected false to be truthy.

  2) Failure:
PathTraversalTest#test_directory_symlink [/Users/john/ex/rubyzip/test/path_traversal_test.rb:65]:
Expected false to be truthy.

I have not yet attempted to fix the bugs, and I'm not sure I know enough about this library to do that, but I thought I should at least report the test results.

Edit: Tried out a (rather drastic) fix in #376

Ignore CVE-2018-1000544 (a vulnerability in rubyzip). It does not affect production. See: #1222 rubyzip/rubyzip#371 https://nvd.nist.gov/vuln/detail/CVE-2018-1000544 Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

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

bdewater force-pushed the fix-cve-2018-1000544 branch from 7720b14 to 2425c91 Compare July 1, 2018 20:03

bdewater force-pushed the fix-cve-2018-1000544 branch from 2425c91 to 472acab Compare July 1, 2018 20:05

bdewater force-pushed the fix-cve-2018-1000544 branch from 472acab to d68fbd2 Compare July 1, 2018 20:45

JackMc approved these changes Jul 2, 2018

View reviewed changes

zeha reviewed Aug 15, 2018

View reviewed changes

bdewater added 2 commits August 23, 2018 18:14

Fix CVE-2018-1000544 absolute path traversal

6e0d231

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

Fix CVE-2018-1000544 symlink path traversal

8e78311

Not sure if the exception is the right way to go

bdewater force-pushed the fix-cve-2018-1000544 branch from d68fbd2 to 8e78311 Compare August 23, 2018 22:15

This was referenced Aug 25, 2018

Ruby bindings: Security vulnerability in rubyzip 1.2.1 SeleniumHQ/selenium#6330

Closed

Security vulnerability in one of the deps of selenium-webdriver (rubyzip) forem/forem#481

Closed

This was referenced Aug 25, 2018

Update rubyzip when an update is available that fixes CVE-2018-1000544 coreinfrastructure/best-practices-badge#1222

Closed

Feature request: Allow group selection rubysec/bundler-audit#195

Open

ecbrodie mentioned this pull request Aug 26, 2018

allow passing Gemfile.lock paths to bundle audit rubysec/bundler-audit#178

Closed

Move jruby to allow failures matrix till crc uint 32 issues are resolved

cf71583

Trigger CI again

0586329

jdleesmiller mentioned this pull request Aug 26, 2018

Fix CVE-2018-1000544 and disable symlinks to avoid other security issues #376

Merged

gmcculloug mentioned this pull request Oct 24, 2018

Bump Rubyzip gem to 1.2.2 ManageIQ/manageiq#18129

Merged

jdleesmiller mentioned this pull request Mar 21, 2019

Move jruby to allow failures matrix #375

Merged

slonopotamus mentioned this pull request Dec 22, 2019

Fix unzipping of kindlegen archive on Windows tdtds/kindlegen#33

Merged

This was referenced Mar 18, 2021

Bump rubyzip from 1.2.1 to 1.3.0 cdhenry/rails-road-trip#1

Open

Bump rubyzip from 1.2.1 to 1.3.0 cdhenry/rails-road-trip-app#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CVE-2018-1000544 #371

Fix CVE-2018-1000544 #371

bdewater commented Jul 1, 2018

coveralls commented Jul 1, 2018 •

edited

coveralls commented Jul 1, 2018 •

edited

bdewater commented Jul 1, 2018

JackMc left a comment

JackMc Jul 1, 2018

JackMc Jul 1, 2018

JackMc Jul 2, 2018

tuzovakaoff Jul 2, 2018

zeha commented Aug 12, 2018

zeha Aug 15, 2018

zeha commented Aug 15, 2018

drn commented Aug 21, 2018

bdewater commented Aug 21, 2018

simonoff commented Aug 22, 2018

bdewater commented Aug 23, 2018 •

edited

reedloden commented Aug 25, 2018

vipulnsward commented Aug 26, 2018

bdewater commented Aug 26, 2018 •

edited

jdleesmiller commented Aug 26, 2018 •

edited

Fix CVE-2018-1000544 #371

Fix CVE-2018-1000544 #371

Conversation

bdewater commented Jul 1, 2018

coveralls commented Jul 1, 2018 • edited

coveralls commented Jul 1, 2018 • edited

bdewater commented Jul 1, 2018

JackMc left a comment

Choose a reason for hiding this comment

JackMc Jul 1, 2018

Choose a reason for hiding this comment

JackMc Jul 1, 2018

Choose a reason for hiding this comment

JackMc Jul 2, 2018

Choose a reason for hiding this comment

tuzovakaoff Jul 2, 2018

Choose a reason for hiding this comment

zeha commented Aug 12, 2018

zeha Aug 15, 2018

Choose a reason for hiding this comment

zeha commented Aug 15, 2018

drn commented Aug 21, 2018

bdewater commented Aug 21, 2018

simonoff commented Aug 22, 2018

bdewater commented Aug 23, 2018 • edited

reedloden commented Aug 25, 2018

vipulnsward commented Aug 26, 2018

bdewater commented Aug 26, 2018 • edited

jdleesmiller commented Aug 26, 2018 • edited

coveralls commented Jul 1, 2018 •

edited

coveralls commented Jul 1, 2018 •

edited

bdewater commented Aug 23, 2018 •

edited

bdewater commented Aug 26, 2018 •

edited

jdleesmiller commented Aug 26, 2018 •

edited