Fixed uncompressing files with wrong uncompressed size set.

Fixed uncompressing files with wrong uncompressed size set.

A zip file carries some metadata for each archived file, including the total
uncompressed size. If that size was incorrect, therefore the compressed file
being different in size when unpacking, the minizip code would fail with a
CRC error. Every other zip utility handles these files, so should the minizip
code for safety sake.

BUG=359516
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268940

[João Eiras, 2014-04-02 15:04:48]

So, first fix here

(for reference, original review  https://codereview.chromium.org/179963002/ )

[satorux1, 2014-04-03 04:59:03]

Could you file a bug on crbug.com and add BUG=<bugid> ?

A bug fix should have a BUG= line in the patch description.


buildbots may not handle binary files nicely. You may need to submit the test
zip file separately before this patch.

https://codereview.chromium.org/222243003/diff/1/third_party/zlib/google/zip_...
File third_party/zlib/google/zip_unittest.cc (right):

https://codereview.chromium.org/222243003/diff/1/third_party/zlib/google/zip_...
third_party/zlib/google/zip_unittest.cc:325: for (int k = 0; k < 8; k++) {
k -> i

for consistency with other code in Chromium

https://codereview.chromium.org/222243003/diff/1/third_party/zlib/google/zip_...
third_party/zlib/google/zip_unittest.cc:331: EXPECT_EQ(file_size,
static_cast<int64>(k));
please swap the arguments. Per gtest.h, the expected value should come first.
The right order should give you a better error message in case of a failure

[João Eiras, 2014-04-03 11:19:39]

Hi agl and gavinp.

I hope this commit message is enough content for you to understand what the
change in minizip is. You might ask for me to upstream the change to the minizip
project. I've tried to contact the owner
(http://www.winimage.com/zLibDll/minizip.html) but got no reply. I guess the
project is dead. Meanwhile someone forked it into github and that person is
actively maintaining the code (https://github.com/nmoinvaz/minizip). I gave him
my changes. I think chromium should be using that updated version.

[agl, 2014-04-03 14:34:56]

https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/...
File third_party/zlib/google/zip_unittest.cc (right):

https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/...
third_party/zlib/google/zip_unittest.cc:326:
SCOPED_TRACE(base::StringPrintf("<loop:%d>", static_cast<int>(i)));
|i| is an int, but you're static_casting it to int in a couple of places.

https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/...
third_party/zlib/google/zip_unittest.cc:328:
base::StringPrintf(FILE_PATH_LITERAL("%d.txt"), static_cast<int>(i)));
I don't think you want FILE_PATH_LITERAL here.

[satorux1, 2014-04-04 05:31:30]

I just recalled that allowing wrong uncompressed size was dangerous. As
described in crbug.com/268243 a malicious zip file can declare the uncompressed
file size to be zero, but have giga-bytes of super-compressed content, that'll
crash the browser if uncompressed in-memory.

Is it really important to support zip files with broken metadata? I guess
popular zipping tools won't create such zip files.

[João Eiras, 2014-04-04 11:51:58]

On 2014/04/04 05:31:30, satorux1 wrote:
> I just recalled that allowing wrong uncompressed size was dangerous. As
> described in crbug.com/268243 a malicious zip file can declare the
uncompressed
> file size to be zero, but have giga-bytes of super-compressed content, that'll
> crash the browser if uncompressed in-memory.
> 
> Is it really important to support zip files with broken metadata? I guess
> popular zipping tools won't create such zip files.

The API I introduced in https://codereview.chromium.org/179963002/
(ZipReader::ExtractCurrentEntryToString has a max_read_bytes field)

But about the issue, you can also have the reverse: a 1kb zip file with the
metadata telling it has a 2GB file. If you rely on
current_entry()->original_size() to allocate a buffer it is not going to go
well.
But who said you need a corrupt file ? Even a perfectly valid zip file can have
large uncompressed data inside (e.g.: 2GB file with just zero bytes compresses
to a 2MB zip file). If you're doing a unbounded read, then the problem lies
right there.

The most important part here is that a ) the size info cannot be trusted b) if
we're to make it easy to read to memory, make sure it is bounded. The problem
oyu mentioned is is no way aggravated by this change.

I did this fix really because I became aware of the problem you mentioned, so I
tested in other software and it works. So instead of having an unexpected
failure in our code (a programmer would have to debug to see which error code
the zip code is returning), why not actually be a bit more lenient (there is no
loss of data really, if the uncompressed size is wrong) and make sure that there
are checks and bounds to make sure the programmer doesn't bite his/her tongue.

[satorux1, 2014-04-30 00:18:19]

https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/...
File third_party/zlib/google/zip_unittest.cc (right):

https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/...
third_party/zlib/google/zip_unittest.cc:326:
SCOPED_TRACE(base::StringPrintf("<loop:%d>", static_cast<int>(i)));
On 2014/04/03 14:34:56, agl wrote:
> |i| is an int, but you're static_casting it to int in a couple of places.

Yes. Please remove static_cast from here and elsewhere.

Maybe "Processing %d.txt" may be more informative than <loop:%d>

https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/...
third_party/zlib/google/zip_unittest.cc:328:
base::StringPrintf(FILE_PATH_LITERAL("%d.txt"), static_cast<int>(i)));
On 2014/04/03 14:34:56, agl wrote:
> I don't think you want FILE_PATH_LITERAL here.

ditto. please address agl's comment.

[satorux1, 2014-04-30 00:20:09]

On 2014/04/04 11:51:58, João Eiras wrote:
> On 2014/04/04 05:31:30, satorux1 wrote:
> > I just recalled that allowing wrong uncompressed size was dangerous. As
> > described in crbug.com/268243 a malicious zip file can declare the
> uncompressed
> > file size to be zero, but have giga-bytes of super-compressed content,
that'll
> > crash the browser if uncompressed in-memory.
> > 
> > Is it really important to support zip files with broken metadata? I guess
> > popular zipping tools won't create such zip files.
> 
> The API I introduced in https://codereview.chromium.org/179963002/
> (ZipReader::ExtractCurrentEntryToString has a max_read_bytes field)

That makes a lot of sense. 


> But about the issue, you can also have the reverse: a 1kb zip file with the
> metadata telling it has a 2GB file. If you rely on
> current_entry()->original_size() to allocate a buffer it is not going to go
> well.
> But who said you need a corrupt file ? Even a perfectly valid zip file can
have
> large uncompressed data inside (e.g.: 2GB file with just zero bytes compresses
> to a 2MB zip file). If you're doing a unbounded read, then the problem lies
> right there.
> 
> The most important part here is that a ) the size info cannot be trusted b) if
> we're to make it easy to read to memory, make sure it is bounded. The problem
> oyu mentioned is is no way aggravated by this change.
> 
> I did this fix really because I became aware of the problem you mentioned, so
I
> tested in other software and it works. So instead of having an unexpected
> failure in our code (a programmer would have to debug to see which error code
> the zip code is returning), why not actually be a bit more lenient (there is
no
> loss of data really, if the uncompressed size is wrong) and make sure that
there
> are checks and bounds to make sure the programmer doesn't bite his/her tongue.

[João Eiras, 2014-05-07 12:03:50]

https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/...
File third_party/zlib/google/zip_unittest.cc (right):

https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/...
third_party/zlib/google/zip_unittest.cc:326:
SCOPED_TRACE(base::StringPrintf("<loop:%d>", static_cast<int>(i)));
On 2014/04/30 00:18:19, satorux1 wrote:
> On 2014/04/03 14:34:56, agl wrote:
> > |i| is an int, but you're static_casting it to int in a couple of places.
> 
> Yes. Please remove static_cast from here and elsewhere.
> 
> Maybe "Processing %d.txt" may be more informative than <loop:%d>

done

https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/...
third_party/zlib/google/zip_unittest.cc:328:
base::StringPrintf(FILE_PATH_LITERAL("%d.txt"), static_cast<int>(i)));
On 2014/04/30 00:18:19, satorux1 wrote:
> On 2014/04/03 14:34:56, agl wrote:
> > I don't think you want FILE_PATH_LITERAL here.
> 
> ditto. please address agl's comment.

done

[gavinp, 2014-05-07 14:01:00]

Overall this lgtm.

This bug exists in zlib 1.2.8, the currently released version. Shouldn't we
upstream it? Just post a patch to zlib-devel and follow the discussion afaik
should do it for you.

https://codereview.chromium.org/222243003/diff/40001/third_party/zlib/google/...
File third_party/zlib/google/zip_unittest.cc (right):

https://codereview.chromium.org/222243003/diff/40001/third_party/zlib/google/...
third_party/zlib/google/zip_unittest.cc:319: const base::FilePath& temp_dir =
scoped_temp_dir.path();
This freaks me out. However, it is safe. But it freaks me out.

https://codereview.chromium.org/222243003/diff/40001/third_party/zlib/google/...
third_party/zlib/google/zip_unittest.cc:322: 
nit: lose this blank line.

[João Eiras, 2014-05-07 15:31:10]

On 2014/05/07 14:01:00, gavinp wrote:
> Overall this lgtm.
> 
> This bug exists in zlib 1.2.8, the currently released version. Shouldn't we
> upstream it? Just post a patch to zlib-devel and follow the discussion afaik
> should do it for you.
> 

The change is in minizip, not zlib. The minizip project seems dead: site without
updates, the maintainer not responding to mail nor other means to contact
however is working on it (if anyone).

[João Eiras, 2014-05-07 15:31:20]

https://codereview.chromium.org/222243003/diff/40001/third_party/zlib/google/...
File third_party/zlib/google/zip_unittest.cc (right):

https://codereview.chromium.org/222243003/diff/40001/third_party/zlib/google/...
third_party/zlib/google/zip_unittest.cc:319: const base::FilePath& temp_dir =
scoped_temp_dir.path();
On 2014/05/07 14:01:01, gavinp wrote:
> This freaks me out. However, it is safe. But it freaks me out.

Why ? The folder needs to be used somehow.

[João Eiras, 2014-05-07 15:36:17]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-05-07 15:41:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/joaoe@opera.com/222243003/60001

[commit-bot: I haz the power, 2014-05-07 20:53:07]

Change committed as 268940