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

Zip entry size unset now honors user requested compression level #1598

Merged

Conversation

JiaT75
Copy link
Contributor

@JiaT75 JiaT75 commented Oct 19, 2021

Fixed an issue where not setting the size of an entry in a zip archive would force the user to use the deflate algorithm even if they specified no compression. I added a test to verify the change correctly addresses this problem.

closes #1547

@mmatuska mmatuska merged commit 09dcd24 into libarchive:master Nov 15, 2021
@gamer191
Copy link

Has anyone checked this commit? I don't understand it, but adding low-level C code seems suspicious, and it was PRed by a bad actor known for hiding malware in tests

@emaste emaste mentioned this pull request Mar 30, 2024
15 tasks
@and-sanford
Copy link

While I don’t know this code base very well, I have some concerns about the changes made to libarchive/archive_write_set_format_zip.c that I at least wanted to share. In context of JiaT75’s other work, I can see those changes introducing the following types of vulnerabilities:

Removal of length-at-end
(1) Without the end-of-data marker, the algorithm (which could also be user specified) could struggle to identify the true end (eg, the user-specified algorithm lacks a clear marker). This could lead to a number of issues with data integrity or unexpected behavior, such as memory leaks, overflows, execution of malicious code, etc.

Via a user-specified algorithm
(2) Code Injection
A threat actor could specify an algorithm that allows custom code execution or manipulation of data structures.

(3) Weaknesses in other compression algorithms.
See the Zip64 section, below, as an example.

(4) Data corruption
The specified algorithm may cause bugs that render the data unreadable.

(5) DoS
The specified algorithm could be computationally expensive and/or have bugs that consume an excessive amount of resources.

via Zip64
(6) While I have no direct evidence, given JiaT75’s skill level, I’m concerned there may be an unknown vulnerability present in Zip64 itself and/or via an interaction between Zip64 and libarchive (eg, an odd string passed through the user-specified command or in the compressed data).

Lastly, thanks for all you’re doing to review JiaT75’s changes and maintain this code base overall. I know you’re doing this voluntarily and really appreciate the great care you have in keeping people safe.

@kientzle
Copy link
Contributor

Thank you, @and-sanford for the moral support!

@kientzle
Copy link
Contributor

I actually do know this area of the code well, since I've been working around here just recently. I'll take a close look at this PR today and let you know what I find.

@and-sanford
Copy link

@kientzle I saw the parent PR was closed, so it looks like this change was benign. I’m curious what your thoughts were when you reviewed?

@kientzle
Copy link
Contributor

kientzle commented Apr 4, 2024

Libarchive's zip writer allows the user to request a specific compression be used. Currently, the two supported options are "deflate" (the compression method invented specifically for the original PK-ZIP implementation) and "store" (uncompressed). This PR fixes a bug where even when the user specifically requested "store" compression, that would be ignored by libarchive in certain cases. Instead, libarchive would force the use of "deflate" compression.

I wrote the original logic here with the idea that "store" compression was incompatible with certain uses, but I've since spent a lot more time working with the corresponding Zip reading code in libarchive and agree with the author of this PR that I was being overly conservative and that there's no really strong reason not to use the compression that was specifically requested by the user.

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.

Writing zip archives with compression-level=0 uses high compression if no file size is set.
7 participants