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

unzip: CVE-2019-13232 #71401

Merged
merged 1 commit into from Nov 9, 2019
Merged

Conversation

thorstenweber83
Copy link
Contributor

Motivation for this change

another try at fixing this cve after #64909 was reverted in #65393

fixes #64663 #70129

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.

i manually tested extraction of lua packages and firefox esr 60's browser/omi.ja file which previously yielded "zip bomb" false positives.

I never did a pr to such an essential package so all your help is welcome :)

@mweinelt
Copy link
Member

mweinelt commented Oct 19, 2019

Did you actually really build this? This forces a rebuild just shy of 23k packages :D

Of course you could've just built unzip, nvm :)

@thorstenweber83
Copy link
Contributor Author

thorstenweber83 commented Oct 20, 2019

I did not claim to have built all dependent packages.
I just verified that the reported false positives (Lua packages and firefox's omni.ja) disappear when adding 6d351831be705cc26d897db44f878a978f4138fc as patch.
How would you like to review my pull request? :D

@jonringer
Copy link
Contributor

please rebase the changes ontop of staging, force push, and change the base branch to staging. I would like to avoid a situation where people building against master have to build 100s of packages to continue developing.

@thorstenweber83
Copy link
Contributor Author

Done! Thanks for the suggestion.

@jonringer
Copy link
Contributor

@GrahamcOfBorg eval

@FRidh FRidh self-assigned this Oct 21, 2019
@FRidh
Copy link
Member

FRidh commented Oct 21, 2019

I think we can give it a try again. I would prefer to keep this not for the upcoming staging-next iteration but the one after given the size already of the upcoming staging-next iteration.

@FRidh FRidh added this to Needs review in Staging Oct 24, 2019
@FRidh FRidh moved this from Needs review to New in Staging Oct 24, 2019
@FRidh FRidh moved this from New to Ready in Staging Oct 24, 2019
@ttuegel ttuegel removed their request for review November 9, 2019 14:16
Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I (also) tested the problematic packages. I don't expect significant regressions now.

vcunat added a commit that referenced this pull request Nov 9, 2019
@vcunat vcunat merged commit 4d33b41 into NixOS:staging Nov 9, 2019
Staging automation moved this from Ready to Done Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants