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.

Sign up for GitHub

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

Jump to bottom

[WIP] unzip: CVE-2019-13232 (release-19.03) #64910

Closed
wants to merge 1 commit into from
Closed

[WIP] unzip: CVE-2019-13232 (release-19.03) #64910

wants to merge 1 commit into from

Conversation

mmahut
Copy link
Member

@mmahut mmahut commented Jul 16, 2019

Motivation for this change

Fixes #64663.
(cherry picked from commit 2868c75)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@mmahut
unzip: CVE-2019-13232
3e34c69 
(cherry picked from commit 2868c75)
@mmahut mmahut changed the title unzip: CVE-2019-13232 unzip: CVE-2019-13232 (release-19.03) Jul 16, 2019
@FRidh
Copy link
Member

FRidh commented Jul 17, 2019

Please perform the cherry pick after the original changeset is accepted. The motivation is that we/I typically pull requests instead of merge them.

@FRidh FRidh self-assigned this Jul 17, 2019
@FRidh FRidh mentioned this pull request Jul 17, 2019
Closed
10 tasks
@mmahut
Copy link
Member Author

mmahut commented Jul 17, 2019

@FRidh, ack I will, thank you.

@risicle
Copy link
Contributor

risicle commented Jul 17, 2019

Again, would this be better aimed at staging-19.03 given the hugeness of the rebuild?

@vcunat
Copy link
Member

vcunat commented Jul 20, 2019

I wouldn't hurry with backporting this, as I see it causes multiple regressions.

@FRidh
Copy link
Member

FRidh commented Jul 20, 2019

the cherry-pick -x reference is not correct

@mmahut
Copy link
Member Author

mmahut commented Jul 20, 2019

I wouldn't hurry with backporting this, as I see it causes multiple regressions.

I agree, setting up as [WIP] until we fix the regression. At the moment I know of only one, haskell.zip-archiver is using a zip archive in their tests that this patch detects as a zip bomb, reported upstream and being discussed if we disable tests for this package until this is fixed upstream.

@mmahut mmahut changed the title unzip: CVE-2019-13232 (release-19.03) [WIP] unzip: CVE-2019-13232 (release-19.03) Jul 20, 2019
@vcunat
Copy link
Member

vcunat commented Jul 20, 2019

Here's the other I've seen: 0238946#commitcomment-34378254

@vcunat vcunat added the 2.status: wait-for-upstream Waiting for upstream fix (or their other action). label Jul 20, 2019
@mmahut
Copy link
Member Author

mmahut commented Jul 20, 2019

Thanks to @vcunat the docbook5 regression has been fixed by upgrading the package from 5.0 to 5.0.1.

The question is, how we want to approach this in release-19.03 and if this CVE severity outweighs updating docbook5 and haskellPackages.zip-archive (regressions found so far) in the stable release branch? My option would be probably not, but it's up to the security team to make this call I think.

@vcunat
Copy link
Member

vcunat commented Jul 20, 2019 •
edited

For unzipping sources we could have an unpatched variant that's explicitly chosen in the problematic cases. EDIT: but in general this CVE doesn't seem serious to me.

@mmahut
Copy link
Member Author

mmahut commented Jul 21, 2019

For unzipping sources we could have an unpatched variant that's explicitly chosen in the problematic cases. EDIT: but in general this CVE doesn't seem serious to me.

That would work for packages with fetchzip but would not fix haskellPackages.zip-archive which uses unzip for its tests. I have a commit in to disable the tests for that package, but just need someone to review/approve it. mmahut@d6059ae

@mmahut
Copy link
Member Author

mmahut commented Jul 23, 2019

After considering the severity of this CVE along with the fixes needed to 19.03 to fix the regressions I think it's not worth backporting this CVE.

@mmahut mmahut closed this Jul 23, 2019
@vcunat vcunat mentioned this pull request Jul 23, 2019
Closed
1 task
@vcunat
Copy link
Member

vcunat commented Jul 25, 2019

I see many lua's *.rock files fail due to this. In our builds it shows as

Error: Failed unpacking rock file: /build/compat53-0.7-1.src.rock: failed extracting /build/compat53-0.7-1.src.rock

vcunat referenced this pull request in madler/unzip Jul 25, 2019
@madler
Detect and reject a zip bomb using overlapped entries.
47b3cea 
This detects an invalid zip file that has at least one entry that
overlaps with another entry or with the central directory to the
end of the file. A Fifield zip bomb uses overlapped local entries
to vastly increase the potential inflation ratio. Such an invalid
zip file is rejected.

See https://www.bamsoftware.com/hacks/zipbomb/ for David Fifield's
analysis, construction, and examples of such zip bombs.

The detection maintains a list of covered spans of the zip files
so far, where the central directory to the end of the file and any
bytes preceding the first entry at zip file offset zero are
considered covered initially. Then as each entry is decompressed
or tested, it is considered covered. When a new entry is about to
be processed, its initial offset is checked to see if it is
contained by a covered span. If so, the zip file is rejected as
invalid.

This commit depends on a preceding commit: "Fix bug in
undefer_input() that misplaced the input state."
@adisbladis adisbladis mentioned this pull request Jul 25, 2019
Merged
10 tasks
@madler
Copy link

madler commented Jul 25, 2019

@vcnuat Can you provide a link to some of these "rock" files that are failing?

@vcunat
Copy link
Member

vcunat commented Jul 26, 2019

@madler: I'm sorry; it seems likely that the vast majority of these are our mistake of not reading your commit message properly. This particular one does succeed when using both of the patches.

@madler
Copy link

madler commented Jul 26, 2019

Does "vast majority" mean that there are still some that indicate overlapped entries that you think is incorrect?

@vcunat
Copy link
Member

vcunat commented Jul 26, 2019

I'm not aware of any at this moment. When I see some, I'll mention you. I expect we will soon try a full rebuild with a complete set of patches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@FRidh FRidh

Labels
2.status: wait-for-upstream Waiting for upstream fix (or their other action). 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mmahut @FRidh @risicle @vcunat @madler