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

[18.09] wget: disable feature responsible for CVE-2018-20483 #52979

Closed
wants to merge 1 commit into from

Conversation

delroth
Copy link
Contributor

@delroth delroth commented Dec 27, 2018

Motivation for this change

wget release 1.20.1 to address CVE-2018-20483. The 1.19 branch used in 18.09 did not receive a new point release fixing the issue. We can just "work around" the problem by disabling xattr support at build time.

Alternatively we could also just rebase the CVE fix onto 1.19.5 ourselves -- I went for the simpler option, let me know if you have a strong preference for the latter option.

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 nox --run "nox-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.

@FRidh
Copy link
Member

FRidh commented Dec 28, 2018

Disabling a feature that's on by default is maybe not such a good idea. I don't know how much people actually rely on this feature, and whether they would be surprised if, without notification, they wouldn't be getting any more extended attributes. Therefore, I think it would be better to backport the actual fix. I personally don't object to this proposed change though.

@delroth
Copy link
Contributor Author

delroth commented Dec 28, 2018

The actual fix disables the option by default anyway, but at runtime instead of build time (putting it being wget --xattr). But I don't mind backporting the patches, they should apply easily.

@delroth
Copy link
Contributor Author

delroth commented Dec 28, 2018

In another episode of "things that seem easy are painful": the CVE fix patches modify wget's info manual sources. That means we can't rely on the prebuild info manual shipped with the 1.19.5 tarball anymore, we have to build it from source. Except the version of texinfo currently in 18.09 goes in an infinite loop when processing wget's texinfo manual with LC_ALL=C (why? not sure! there is a "perl.patch" in the texinfo dir in nixpkgs which I suspect would fix this, but no package actually uses this perl.patch...)

Given that backporting would also be a behavior change (since the patch removes the --no-xattr flag and replaces it with --xattr), I think for now it's better to just --disable-xattr at configure time.

Any more thoughts on this?

@delroth delroth mentioned this pull request Dec 29, 2018
10 tasks
@samueldr
Copy link
Member

Wouldn't using only the patch that changes the xattr behaviour be fine with regards to (1) not changing the behaviour of currently having --xattr be on by default and (2) fixing the behaviour in a not-really-surprising way? (I don't actually know for sure; it's why I'm asking.)

diff --git a/pkgs/tools/networking/wget/default.nix b/pkgs/tools/networking/wget/default.nix
index da655f1dbaa..989b08d9857 100644
--- a/pkgs/tools/networking/wget/default.nix
+++ b/pkgs/tools/networking/wget/default.nix
@@ -1,4 +1,4 @@
-{ stdenv, fetchurl, gettext, pkgconfig, perl
+{ stdenv, fetchurl, fetchpatch, gettext, pkgconfig, perl
 , libidn2, zlib, pcre, libuuid, libiconv, libintl
 , IOSocketSSL, LWP, python3, lzip
 , libpsl ? null
@@ -14,6 +14,10 @@ stdenv.mkDerivation rec {
 
   patches = [
     ./remove-runtime-dep-on-openssl-headers.patch
+    (fetchpatch {
+      url = "http://git.savannah.gnu.org/cgit/wget.git/patch/?id=3cdfb594cf75f11cdbb9702ac5e856c332ccacfa";
+      sha256 = "1rvd4mykbqdsdxvbnhgc9k1bnqwvhvz29cnv2idmf6g0w6vp385h";
+    })
   ];
 
   preConfigure = ''
@@ -35,7 +39,6 @@ stdenv.mkDerivation rec {
     ++ stdenv.lib.optional stdenv.isDarwin perl;
 
   configureFlags = [
-    (stdenv.lib.enableFeature false "xattr")  # CVE-2018-20483 fixed in 1.20.1
     (stdenv.lib.withFeatureAs (openssl != null) "ssl" "openssl")
   ];
 

@delroth
Copy link
Contributor Author

delroth commented Jan 29, 2019

@samueldr it might be, but upstream's decision was to implement both fixes as part of their security release, which is why I also decided to do both here. It seems like upstream is basically saying that this was a misfeature in the first place to have enabled by default.

I don't use 18.09 on any of my systems and this has been dragging on for a while -- closing because I don't really want to spend more time on this. Feel free to pick this up.

@delroth delroth closed this Jan 29, 2019
@vcunat vcunat mentioned this pull request Feb 25, 2019
10 tasks
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

5 participants