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

gettext: apply patch for CVE 2018-18751 #58997

Merged
merged 1 commit into from Apr 5, 2019

Conversation

ctheune
Copy link
Contributor

@ctheune ctheune commented Apr 5, 2019

Re-include an older automake (1.15) because that's explicitly depended upon.

Motivation for this change
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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ctheune
Copy link
Contributor Author

ctheune commented Apr 5, 2019

This security update was under the radar, because the CVE wasn't formatted correctly. 0.19.8.1 is still affected (and the package hasn't seen a release since ages) but the CVE only talks about 0.19.8.

I'd also like to get a backport to 18.09 if possible, thanks!

@andir
Copy link
Member

andir commented Apr 5, 2019

Thank you for looking into this! 👍

I usually prefer if you just open a backport PR yourself. It ensures we have the same level of checks / eval / compile / ofBorg / … on those as well. Stable branches should receive that level of diligence :-)

I am reviewing the changes themselves now.

@ctheune
Copy link
Contributor Author

ctheune commented Apr 5, 2019

Alright, sure thing! I'll get another one out in a minute.

@andir
Copy link
Member

andir commented Apr 5, 2019

I am tempted to have this go into the respective staging branches first since the amount of rebuilds is quiet large and the issue only occurs when used as a commandline tool.

@ctheune
Copy link
Contributor Author

ctheune commented Apr 5, 2019

Yeah, it's a package that's used in the most basic build environment and thus causes massive rebuilds. Do I need to do anything wrt to directing this to the staging branch?

@ctheune
Copy link
Contributor Author

ctheune commented Apr 5, 2019

Alright, I targeted the staging branch for the backport. That's #59000

Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

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

Besides moving this to staging and the few comments below there is nothing I see that is wrong about this.

pkgs/development/tools/misc/automake/automake-1.15.x.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/gettext/CVE-2018-18751.patch Outdated Show resolved Hide resolved
@andir andir closed this Apr 5, 2019
@andir andir reopened this Apr 5, 2019
@andir
Copy link
Member

andir commented Apr 5, 2019

(Sorry hit the wrong button there while constructing a comment)

To move your changes to staging pick the current commit into a local checkout of the staging-next branch. Then force-push into your branch of this PR and edit the target branch to staging-next. (staging-next is where we gather changes that should be staged for inclusion into master in the next iteration)

https://help.github.com/en/articles/changing-the-base-branch-of-a-pull-request shows how to retarget to another branch.

@GrahamcOfBorg GrahamcOfBorg requested a review from andir April 5, 2019 09:22
@ctheune ctheune changed the base branch from master to staging-next April 5, 2019 09:23
@ctheune
Copy link
Contributor Author

ctheune commented Apr 5, 2019

Alright, base branch is changed and the patch isn't vendored any longer. The newline is still there because I'd like to understand whether that's really the policy :)

@ctheune
Copy link
Contributor Author

ctheune commented Apr 5, 2019

Alright, requested changes are in here. Let me know if you're happy and I'll update the stable PR.

@andir
Copy link
Member

andir commented Apr 5, 2019

When trying to build gettext i see the following now:

error: while evaluating the attribute 'builder' of the derivation 'gettext-0.19.8.1' at /home/andi/dev/nixos/nixpkgs-58997/pkgs/development/libraries/gettext/default.nix:4:3:
while evaluating the attribute 'shell' at /home/andi/dev/nixos/nixpkgs-58997/pkgs/stdenv/generic/default.nix:103:14:
while evaluating the attribute 'shell' at /home/andi/dev/nixos/nixpkgs-58997/pkgs/build-support/cc-wrapper/default.nix:84:3:
while evaluating 'getOutput' at /home/andi/dev/nixos/nixpkgs-58997/lib/attrsets.nix:464:23, called from /home/andi/dev/nixos/nixpkgs-58997/pkgs/build-support/cc-wrapper/default.nix:84:11:
while evaluating the attribute 'depsBuildBuild' of the derivation 'bash-4.4-p23' at /home/andi/dev/nixos/nixpkgs-58997/pkgs/shells/bash/4.4.nix:23:3:
while evaluating the attribute 'cc' of the derivation 'bootstrap-stage4-gcc-wrapper-7.4.0' at /home/andi/dev/nixos/nixpkgs-58997/pkgs/build-support/cc-wrapper/default.nix:77:3:
while evaluating the attribute 'buildInputs' of the derivation 'gcc-7.4.0' at /home/andi/dev/nixos/nixpkgs-58997/pkgs/development/compilers/gcc/7/default.nix:138:3:
while evaluating the attribute 'nativeBuildInputs' of the derivation 'libelf-0.8.13' at /home/andi/dev/nixos/nixpkgs-58997/pkgs/development/libraries/libelf/default.nix:6:3:
while evaluating the attribute 'patches' of the derivation 'gettext-0.19.8.1' at /home/andi/dev/nixos/nixpkgs-58997/pkgs/development/libraries/gettext/default.nix:4:3:
while evaluating anonymous function at /home/andi/dev/nixos/nixpkgs-58997/pkgs/build-support/fetchpatch/default.nix:8:1, called from /home/andi/dev/nixos/nixpkgs-58997/pkgs/development/libraries/gettext/default.nix:13:6:
anonymous function at /home/andi/dev/nixos/nixpkgs-58997/pkgs/build-support/fetchurl/boot.nix:5:1 called with unexpected argument 'meta', at /home/andi/dev/nixos/nixpkgs-58997/pkgs/build-support/fetchpatch/default.nix:10:1

@ctheune
Copy link
Contributor Author

ctheune commented Apr 5, 2019

Interesting, what's the command you're using? I did nix-build -A gettext.

@andir
Copy link
Member

andir commented Apr 5, 2019

I use the very same command.

nix-info -m:

  • system: "x86_64-linux"
  • host os: Linux 5.0.4, NixOS, 19.03beta171840.23fd1394dc6 (Koi)
  • multi-user?: yes
  • sandbox: yes
  • version: nix-env (Nix) 2.3pre6631_e58a7144
  • channels(root): "nixos-19.03beta171840.23fd1394dc6, nixos-unstable-19.09pre173349.07b42ccf2de"
  • channels(andi): ""
  • nixpkgs: /nix/var/nix/profiles/per-user/root/channels/nixos

@ctheune
Copy link
Contributor Author

ctheune commented Apr 5, 2019

I'm on MacOS

nixpkgs git:(gettext-security-update) nix --version
nix (Nix) 2.2

It will be built twice, right? Did this fail on the first gettext or the second? I didn't wait for all of GCC and the other environment to build so that could be it ...

@andir
Copy link
Member

andir commented Apr 5, 2019

On 18.09 it is also failing and yes I still believe that it is being built twice due to bootstrapping. Darwin bootstrapping may still be different.

@GrahamcOfBorg build gettext

Re-include an older automake (1.15) because that's explicitly depended upon.
@ctheune
Copy link
Contributor Author

ctheune commented Apr 5, 2019

Ok, so fetchPatch worked on MacOS but not Linux. I was able to reproduce the issue on NixOS and it was fixed after switching to fetchUrl. PR is up to date.

@ctheune
Copy link
Contributor Author

ctheune commented Apr 5, 2019

Alright, I'll update the backport PR now.

@andir andir merged commit f93e176 into NixOS:staging-next Apr 5, 2019
vcunat added a commit that referenced this pull request Apr 5, 2019
This reverts commit f93e176, reversing
changes made to 2771375.
The MR was meant for staging, not staging-next, so let's move it.
vcunat added a commit that referenced this pull request Apr 5, 2019
@ctheune ctheune deleted the gettext-security-update branch April 9, 2019 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants