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
Conversation
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. |
The actual fix disables the option by default anyway, but at runtime instead of build time (putting it being |
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? |
Wouldn't using only the patch that changes the 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")
];
|
@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. |
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
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)