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

Add missing gzip footer check in ActiveSupport::Gzip.decompress #28158

Merged
merged 1 commit into from Feb 25, 2017

Conversation

Projects
None yet
3 participants
@dylanahsmith
Contributor

dylanahsmith commented Feb 24, 2017

Problem

I noticed that gzip data decompressed with ActiveSupport::Gzip.decompress didn't check the crc in the footer for the compressed data. After digging into why this wasn't happening I found the following in the ruby documentation for Zlib::GzipReader

Be careful of the footer of the gzip file. A gzip file has the checksum of pre-compressed data in its footer. GzipReader checks all uncompressed data against that checksum at the following cases, and if it fails, raises Zlib::GzipFile::NoFooter, Zlib::GzipFile::CRCError, or Zlib::GzipFile::LengthError exception.

  • When an reading request is received beyond the end of file (the end of compressed data). That is, when #read, #gets, or some other methods for reading returns nil.
  • When Zlib::GzipFile#close method is called after the object reaches the end of file.
  • When #unused method is called after the object reaches the end of file.

So calling Zlib::GzipReader#read isn't enough, the Zlib::GzipReader also needs to be closed to make sure the footer is checked.

Solution

I added a failing test that showed that the footer wasn't being checked by flipping some bits in the CRC32 field of the footer. Details on the gzip file format can be found in RFC 1952 which documents the end of the file as having a 4 byte CRC32 field and a 4 byte input size field.

  0   1   2   3   4   5   6   7
+---+---+---+---+---+---+---+---+
|     CRC32     |     ISIZE     |
+---+---+---+---+---+---+---+---+

To fix this bug I used Zlib::GzipReader.wrap which closes the Zlib::GzipReader after the given block is called which reads the compressed data.

Add missing gzip footer check in ActiveSupport::Gzip.decompress
A gzip file has a checksum and length for the decompressed data in its
footer which isn't checked by just calling Zlib::GzipReader#read.
Calling Zlib::GzipReader#close must be called after reading to the end
of the file causes this check to be done, which is done by
Zlib::GzipReader.wrap after its block is called.

@rafaelfranca rafaelfranca merged commit 389c617 into rails:master Feb 25, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Feb 25, 2017

Merge pull request #28158 from dylanahsmith/gzip-crc-check
Add missing gzip footer check in ActiveSupport::Gzip.decompress
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment