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

Validate entry sizes when extracting #403

Merged
merged 3 commits into from Sep 25, 2019

Conversation

@jdleesmiller
Copy link
Member

jdleesmiller commented Sep 12, 2019

Update (2019-09-25): Assigned CVE-2019-16892

To extract zip files safely:

  • If you upgrade to rubyzip >= 1.3.0 and < 2.0.0, you must:

    1. be sure to check entry.size as illustrated in the README before you call entry.extract, and
    2. set Zip.validate_entry_sizes = true to enable the validation added in this PR.
  • If you upgrade to rubyzip >= 2.0.0, you must:

    1. be sure to check entry.size as illustrated in the README before you call entry.extract.
    2. (there is no step 2)

If you are using a recent (not EOL) version of ruby, the upgrade to 2.0.0 should be smooth. See the Changelog for details.


Fix #193 ("Protection Against Zip Bombs")

Currently there is no way to safely check how much data the Zip::Entry#extract method will actually extract, because the uncompressed size reported in the zip can be spoofed, and the extraction happens in the method where the caller can't check on its progress.

This can lead to denial of service through disk exhaustion if the input is a zip bomb.

Approach

The approach taken here is based on the validateEntrySizes option for node's yauzl unzipping library: rubyzip can check that it doesn't extract (much) more than the expected amount of data, based on the uncompressed size reported in the zip file. That way the caller can check the entry's uncompressed size before extracting and trust that it is not misleading.

The "much" in "much more" above is because rubyzip writes the data in chunks (currently 32KiB), and it may write up to one chunk more than expected, but that should be negligible, and there will be an error that signals that rubyzip wrote more data than expected.

Impact

Zip files with incorrectly reported uncompressed sizes that worked before will now fail with a Zip::EntrySizeError. Like in yauzl, the safe behaviour is the default but a flag is provided to allow the caller to (dangerously) skip the checks:

Zip.validate_entry_sizes = false

EDIT: Following discussion below, false will be the default in the 1.3 release. We'll default to true in 2.0.

I have updated the README to say that the caller should be checking the size before extracting. I also reorganised some of the options, since there are now quite a few.

Review

I will aim to leave this open for 1 week for review. CC @simonoff @olleolleolle @hainesr who have been active recently.

It probably merits at least a minor version bump.

Credit

Thanks to the GE Digital Cyber Security Team for providing a proof of concept, which was the basis for the test case.

Thanks to @thejoshwolfe for providing (I think) a good example of how to handle this, in yauzl.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 12, 2019

Coverage Status

Coverage increased (+0.02%) to 95.769% when pulling 067aef2 on check-size into 0d85cb6 on master.

@simonoff

This comment has been minimized.

Copy link
Member

simonoff commented Sep 13, 2019

@jdleesmiller looks ok. The only thing that we need to show some notification on unsecured behavior.

@hainesr

This comment has been minimized.

Copy link
Contributor

hainesr commented Sep 13, 2019

I think this looks OK too.

Minor question though: There looks to be changes unrelated to this PR in the README. Headers and text moved around.

Also, it should definitely have a minor version bump, but maybe it should be a major bump. Do we have a feel for how much code out there might suddenly start failing unexpectedly? One strategy might be to release it as a minor bump with Zip.validate_entry_sizes set to false by default, so it's in there and people can turn it on in a controlled way, and then default to true at the next major bump.

@jdleesmiller

This comment has been minimized.

Copy link
Member Author

jdleesmiller commented Sep 13, 2019

Thanks both for the quick turnaround!

we need to show some notification on unsecured behavior

Could you elaborate on what you have in mind here? At the moment it will throw an error if the file's reported uncompressed size is incorrect.

There looks to be changes unrelated to this PR in the README

I reorganised the README for other options, because I thought the list was getting too long as I added yet another one. The content of the sections about the existing options should all be unchanged.

Do we have a feel for how much code out there might suddenly start failing unexpectedly?

Nothing very precise... but I have been using the yauzl node library, which validates the reported uncompressed sizes by default, for many years, and so far it has not been a problem. That suggests to me that non-malicious zip files with incorrect uncompressed sizes are not very common. However, I certainly can't rule out the possibility that it will break for someone.

Your suggestion of defaulting validate_entry_sizes to false for an initial minor release sounds good to me 👍 (but it should be 'secure by default' for the next major).

@jdleesmiller jdleesmiller force-pushed the check-size branch from 067aef2 to 7849f73 Sep 15, 2019
@jdleesmiller

This comment has been minimized.

Copy link
Member Author

jdleesmiller commented Sep 15, 2019

Rebased onto latest master and updated according to comments above (default false in 1.3).

@simonoff

This comment has been minimized.

Copy link
Member

simonoff commented Sep 15, 2019

@jdleesmiller notify into stdout need to show that there was such zip file. For cases when check is disabled - so instead of raising it will be a warning.

@jdleesmiller

This comment has been minimized.

Copy link
Member Author

jdleesmiller commented Sep 18, 2019

@simonoff OK, thanks for the clarification. That's OK by me. I've added 97cb6ae to print a warning when the validation error is disabled.

@hainesr

This comment has been minimized.

Copy link
Contributor

hainesr commented Sep 21, 2019

Sorry to be a pain. Could we use warn rather than puts for warning messages? warn prints to stderr so people can siphon off errors/warnings more easily in production.

(I realise that there are other parts of the library code that use puts, and I hope to fix them one day soon.)

@jdleesmiller

This comment has been minimized.

Copy link
Member Author

jdleesmiller commented Sep 25, 2019

Could we use warn rather than puts for warning messages?

Right... that would be better, but unfortunately I used puts in #376, so I thought it was better to be consistent. If you have a fix planned, I think it would be better to fix them all at once rather than having a mix. Sound OK?

@hainesr

This comment has been minimized.

Copy link
Contributor

hainesr commented Sep 25, 2019

👍 good for me! I'll work something up in the next couple of weeks...

@jdleesmiller

This comment has been minimized.

Copy link
Member Author

jdleesmiller commented Sep 25, 2019

OK, thanks! In that case I will get this merged and release 1.3.

@jdleesmiller jdleesmiller merged commit d65fe7b into master Sep 25, 2019
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on check-size at 99.674%
Details
@jdleesmiller jdleesmiller deleted the check-size branch Sep 25, 2019
@touhouota touhouota mentioned this pull request Sep 28, 2019
@annaswims annaswims mentioned this pull request Sep 30, 2019
0 of 5 tasks complete
zurbergram added a commit to department-of-veterans-affairs/gibct-data-service that referenced this pull request Sep 30, 2019
unning bundle-audit to check for insecure dependencies...
Updating ruby-advisory-db ...
From https://github.com/rubysec/ruby-advisory-db
 * branch            master     -> FETCH_HEAD
Already up to date.
Updated ruby-advisory-db
ruby-advisory-db: 406 advisories
Name: rubyzip
Version: 1.2.3
Advisory: CVE-2019-16892
Criticality: Unknown
URL: rubyzip/rubyzip#403
Title: Denial of Service in rubyzip ("zip bombs")
Solution: upgrade to >= 1.3.0

Vulnerabilities found!

Failed. Security vulnerabilities were found.
senid231 added a commit to senid231/yeti-web that referenced this pull request Oct 1, 2019
Name: rubyzip
Version: 1.2.2
Advisory: CVE-2019-16892
Criticality: Unknown
URL: rubyzip/rubyzip#403
Title: Denial of Service in rubyzip ("zip bombs")
Solution: upgrade to >= 1.3.0
senid231 added a commit to senid231/yeti-web that referenced this pull request Oct 1, 2019
Name: rubyzip
Version: 1.2.2
Advisory: CVE-2019-16892
Criticality: Unknown
URL: rubyzip/rubyzip#403
Title: Denial of Service in rubyzip ("zip bombs")
Solution: upgrade to >= 1.3.0
d-m-u pushed a commit to d-m-u/manageiq-automation_engine that referenced this pull request Oct 1, 2019
d-m-u
d-m-u pushed a commit to d-m-u/manageiq-automation_engine that referenced this pull request Oct 1, 2019
d-m-u
@toncid toncid mentioned this pull request Oct 1, 2019
@rokumatsumoto rokumatsumoto mentioned this pull request Oct 1, 2019
myGitHubehsan added a commit to myGitHubehsan/rubyzip that referenced this pull request Oct 2, 2019
myGitHubehsan added a commit to myGitHubehsan/ehsanasgari688 that referenced this pull request Oct 2, 2019
myGitHubehsan added a commit to myGitHubehsan/ehsanasgari688 that referenced this pull request Oct 2, 2019
d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Oct 3, 2019
d-m-u
d-m-u pushed a commit to d-m-u/manageiq-automation_engine that referenced this pull request Oct 3, 2019
d-m-u
This flag must be set in order to use the validation against zip bombs described in rubyzip/rubyzip#403.

See main repo PR ManageIQ/manageiq#19360
derilkusuma referenced this pull request Oct 12, 2019
@y-yagi y-yagi mentioned this pull request Oct 15, 2019
mysociety-pusher pushed a commit to mysociety/alaveteli that referenced this pull request Oct 15, 2019
Bumps [rubyzip](https://github.com/rubyzip/rubyzip) from 1.2.2 to 1.3.0. **This update includes a security fix.**
- [Release notes](https://github.com/rubyzip/rubyzip/releases/tag/v1.3.0)
- [Changelog](https://github.com/rubyzip/rubyzip/blob/master/Changelog.md)
- [Commits](rubyzip/rubyzip@v1.2.2...v1.3.0)

Security fix adds `validate_entry_sizes` option so that callers can
trust an entry's reported size when using `extract`.

We don't currently call `extract`, so I don't think we're affected by
this issue.

As per rubyzip/rubyzip#403, I've created an
initializer to set `validate_entry_sizes`, which can be removed once we
drop support for Ruby 2.3
(#5222).
jmromer added a commit to bikeindex/bike_index that referenced this pull request Oct 17, 2019
Name: rubyzip
Version: 1.2.3
Advisory: CVE-2019-16892
Criticality: Unknown
URL: rubyzip/rubyzip#403
Title: Denial of Service in rubyzip ("zip bombs")
Solution: upgrade to >= 1.3.0
jmromer added a commit to bikeindex/bike_index that referenced this pull request Oct 17, 2019
Name: rubyzip
Version: 1.2.3
Advisory: CVE-2019-16892
Criticality: Unknown
URL: rubyzip/rubyzip#403
Title: Denial of Service in rubyzip ("zip bombs")
Solution: upgrade to >= 1.3.0
juliancheal added a commit to juliancheal/manageiq that referenced this pull request Oct 17, 2019
david-a-wheeler added a commit to coreinfrastructure/best-practices-badge that referenced this pull request Oct 17, 2019
Update gem rubyzip; the version has a security vulnerability
CVE-2019-16892 reported against it.
It's not clear we're vulnerable, but best to fix it regardless.

This vulnerability was reported by bundle audit as follows:

* Name: rubyzip
* Version: 1.2.3
* Advisory: CVE-2019-16892
* Criticality: Unknown
* URL: rubyzip/rubyzip#403
* Title: Denial of Service in rubyzip ("zip bombs")
* Solution: upgrade to >= 1.3.0

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>
mysociety-pusher pushed a commit to mysociety/alaveteli that referenced this pull request Oct 18, 2019
Bumps [rubyzip](https://github.com/rubyzip/rubyzip) from 1.2.2 to 1.3.0. **This update includes a security fix.**
- [Release notes](https://github.com/rubyzip/rubyzip/releases/tag/v1.3.0)
- [Changelog](https://github.com/rubyzip/rubyzip/blob/master/Changelog.md)
- [Commits](rubyzip/rubyzip@v1.2.2...v1.3.0)

Security fix adds `validate_entry_sizes` option so that callers can
trust an entry's reported size when using `extract`.

We don't currently call `extract`, so I don't think we're affected by
this issue.

As per rubyzip/rubyzip#403, I've created an
initializer to set `validate_entry_sizes`, which can be removed once we
drop support for Ruby 2.3
(#5222).
mysociety-pusher pushed a commit to mysociety/alaveteli that referenced this pull request Oct 18, 2019
Bumps [rubyzip](https://github.com/rubyzip/rubyzip) from 1.2.2 to 1.3.0. **This update includes a security fix.**
- [Release notes](https://github.com/rubyzip/rubyzip/releases/tag/v1.3.0)
- [Changelog](https://github.com/rubyzip/rubyzip/blob/master/Changelog.md)
- [Commits](rubyzip/rubyzip@v1.2.2...v1.3.0)

Security fix adds `validate_entry_sizes` option so that callers can
trust an entry's reported size when using `extract`.

We don't currently call `extract`, so I don't think we're affected by
this issue.

As per rubyzip/rubyzip#403, I've created an
initializer to set `validate_entry_sizes`, which can be removed once we
drop support for Ruby 2.3
(#5222).
@touhouota touhouota mentioned this pull request Nov 7, 2019
@ghiculescu ghiculescu mentioned this pull request Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.