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

Detect G.zip64 based on local file header and not central directory file header #9

Closed
wants to merge 1 commit into from

Conversation

janchochol
Copy link

Both file headers (central directory and local) can contain Zip64 extra
field.
It has different semantics base on header.

In central directory file header it allows specifying uncompressed size,
compressed size, local file header offset as 64 bit numbers and disk
number as 32 bit number.

In local file header it allows specifying uncompressed size and
compressed size as 64 bit numbers.

There can be valid situation, where central directory file header use
Zip64 extra field, but local file header does not use it.
It is typical for files smaller than 4 GB, where local file header
offset is larger than 4 GB (e.g. for large archives).

Such file can be created e.g. by:

dd if=/dev/urandom of=file_larger_than_4g bs=1024 count=4194305
dd if=/dev/zero of=file_smaller_than_4g bs=1024 count=1
dd if=/dev/zero of=another_file_smaller_than_4g bs=1024 count=1

zip -1 -fd bomb.zip file_larger_than_4g file_smaller_than_4g another_file_smaller_than_4g

Note: -fd is used to simplify scenario - similar error can be achieved
with more complicated input even without -fd.

Problem is current ZIP bomb detection, which guess size of data descriptor
based on presence of Zip64 extra field in central directory file header.
For given example file_smaller_than_4g has Zip64 extra field in central
directory file header, but has no Zip64 extra field in local file header
and also it has small (containing 32 bit numbers) data descriptor.

Which leads to incorrect ZIP bomb detection:

unzip -t bomb.zip file_smaller_than_4g another_file_smaller_than_4g
Archive:  bomb.zip
    testing: file_smaller_than_4g     OK
error: invalid zip file with overlapped components (possible zip bomb)

Purpose of this fix is to correctly detect size of data descriptor
based on presence of Zip64 extra field in local file header (and not in central
directory file header).
It looks, that it should be correct when consulted with zip source
code (there is very long comment, that situation is not ideal, and size
must be guessed).

With this change:

unzip -t bomb.zip file_smaller_than_4g another_file_smaller_than_4g
Archive:  bomb.zip
    testing: file_smaller_than_4g     OK
    testing: another_file_smaller_than_4g   OK
No errors detected in bomb.zip for the 2 files tested.

…ile header

Both file headers (central directory and local) can contain Zip64 extra
field.
It has different semantics base on header.

In central directory file header it allows specifying uncompressed size,
compressed size, local file header offset as 64 bit numbers and disk
number as 32 bit number.

In local file header it allows specifying uncompressed size and
compressed size as 64 bit numbers.

There can be valid situation, where central directory file header use
Zip64 extra field, but local file header does not use it.
It is typical for files smaller than 4 GB, where local file header
offset is larger than 4 GB (e.g. for large archives).

Such file can be created e.g. by:
```
dd if=/dev/urandom of=file_larger_than_4g bs=1024 count=4194305
dd if=/dev/zero of=file_smaller_than_4g bs=1024 count=1
dd if=/dev/zero of=another_file_smaller_than_4g bs=1024 count=1

zip -1 -fd bomb.zip file_larger_than_4g file_smaller_than_4g another_file_smaller_than_4g
```

Note: `-fd` is used to simplify scenario - similar error can be achieved
with more complicated input even without `-fd`.

Problem is current ZIP bomb detection, which guess size of data descriptor
based on presence of Zip64 extra field in central directory file header.
For given example `file_smaller_than_4g` has Zip64 extra field in central
directory file header, but has no Zip64 extra field in local file header
and also it has small (containing 32 bit numbers) data descriptor.

Which leads to incorrect ZIP bomb detection:
```
unzip -t bomb.zip file_smaller_than_4g another_file_smaller_than_4g
Archive:  bomb.zip
    testing: file_smaller_than_4g     OK
error: invalid zip file with overlapped components (possible zip bomb)
```

Purpose of this fix is to correctly detect size of data descriptor
based on presence of Zip64 extra field in local file header (and not in central
directory file header).
It looks, that it should be correct when consulted with `zip` source
code (there is very long comment, that situation is not ideal, and size
must be guessed).

With this change:
```
unzip -t bomb.zip file_smaller_than_4g another_file_smaller_than_4g
Archive:  bomb.zip
    testing: file_smaller_than_4g     OK
    testing: another_file_smaller_than_4g   OK
No errors detected in bomb.zip for the 2 files tested.
```
@janchochol
Copy link
Author

Hi there, is there any update regarding this PR?

1 similar comment
@janchochol
Copy link
Author

Hi there, is there any update regarding this PR?

@madler
Copy link
Owner

madler commented Oct 7, 2022

af0d07f fixes this by ignoring the extra fields, looking for all four data descriptor possibilities. The comments describe why this works.

@madler madler closed this Oct 7, 2022
@janchochol
Copy link
Author

Thanks for update. I check our case and it behaves correctly.

@madler
Copy link
Owner

madler commented Oct 7, 2022

Good. Can you provide a download link to your case?

@janchochol
Copy link
Author

Unfortunately it contains private data, so I can not provide it.
But test case (which has same behavior in regards to this problem) can be created as described in PR:

dd if=/dev/urandom of=file_larger_than_4g bs=1024 count=4194305
dd if=/dev/zero of=file_smaller_than_4g bs=1024 count=1
dd if=/dev/zero of=another_file_smaller_than_4g bs=1024 count=1

zip -1 -fd bomb.zip file_larger_than_4g file_smaller_than_4g another_file_smaller_than_4g

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants