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

thorstenweber83 commented Oct 19, 2019

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

jonringer commented Oct 20, 2019

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 thorstenweber83 force-pushed the thorstenweber83:unzip-CVE-2019-13232 branch from 6b663e1 to 4d33b41 Oct 20, 2019
@thorstenweber83 thorstenweber83 changed the base branch from master to staging Oct 20, 2019
@thorstenweber83

This comment has been minimized.

Copy link
Contributor Author

thorstenweber83 commented Oct 20, 2019

Done! Thanks for the suggestion.

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Oct 20, 2019

@FRidh FRidh self-assigned this Oct 21, 2019
@FRidh

This comment has been minimized.

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 Nov 9, 2019
@vcunat
vcunat approved these changes Nov 9, 2019
Copy link
Member

vcunat left a comment

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
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
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
You can’t perform that action at this time.