Do not verify SSL peer when fetching sources #26273

kirelagin · 2017-05-31T13:27:28Z

I would be greatful if as many people as possible could review this pull request as it potentially has security implications.

The idea is that we check the hashes of sources after fetching them, so there is no need to verify the SSL peer when making the connection. For example, fetchurl uses the --insecure option in order to make curl skip verification. Additionally, this way we get rid of the pkgs.cacert dependency in some places.

Fetchers that use pkgs.cacert:

fetchgit (tested)
fetchrepoproject (can someone test the change please?)
docker/pull.{nix,sh} (something crazy is going on there)
fetchbower (haven’t looked yet)
fetchdarcs (looks like there is no way to disable)
fetchgx (haven’t looked yet)
fetchcargo (haven’t looked yet)

Various other places, which I haven’t looked at yet:

pkgs/applications/networking/browsers/chromium/update.nix:          if SSL_CERT_FILE="${cacert}/etc/ssl/certs/ca-bundle.crt" \
pkgs/applications/networking/remote/citrix-receiver/default.nix:    awk 'BEGIN {c=0;} /BEGIN CERT/{c++} { print > "cert." c ".pem"}' < ${cacert}/etc/ssl/certs/ca-bundle.crt
pkgs/build-support/rust/default.nix:    export SSL_CERT_FILE=${cacert}/etc/ssl/certs/ca-bundle.crt
pkgs/development/compilers/go/1.7.nix:  NIX_SSL_CERT_FILE = "${cacert}/etc/ssl/certs/ca-bundle.crt";
pkgs/development/compilers/go/1.8.nix:  NIX_SSL_CERT_FILE = "${cacert}/etc/ssl/certs/ca-bundle.crt";
pkgs/development/compilers/mono/generic.nix:    $out/bin/cert-sync ${cacert}/etc/ssl/certs/ca-bundle.crt
pkgs/development/compilers/openjdk/7.nix:      perl ${./generate-cacerts.pl} $jre/lib/openjdk/jre/bin/keytool ${cacert}/etc/ssl/certs/ca-bundle.crt
pkgs/development/compilers/openjdk/8.nix:      perl ${./generate-cacerts.pl} $jre/lib/openjdk/jre/bin/keytool ${cacert}/etc/ssl/certs/ca-bundle.crt
pkgs/development/compilers/rust/beta.nix:      export SSL_CERT_FILE=${cacert}/etc/ssl/certs/ca-bundle.crt
pkgs/development/compilers/rust/cargo.nix:      --set CARGO_HTTP_CAINFO "${cacert}/etc/ssl/certs/ca-bundle.crt" \
pkgs/development/compilers/rust/cargo.nix:      --set SSL_CERT_FILE "${cacert}/etc/ssl/certs/ca-bundle.crt" \
pkgs/development/compilers/rust/cargo.nix:    export SSL_CERT_FILE=${cacert}/etc/ssl/certs/ca-bundle.crt
pkgs/development/compilers/rust/nightly.nix:      export SSL_CERT_FILE=${cacert}/etc/ssl/certs/ca-bundle.crt
pkgs/development/haskell-modules/generic-stack-builder.nix:  GIT_SSL_CAINFO = "${cacert}/etc/ssl/certs/ca-bundle.crt";
pkgs/development/libraries/openssl/use-etc-ssl-certs-darwin.patch:+#  define X509_CERT_FILE          "/nix/var/nix/profiles/default/etc/ssl/certs/ca-bundle.crt"
pkgs/development/libraries/qt-5/5.6/qtwebengine/default.nix:    sed -i -e 's,/cert.pem,/certs/ca-bundle.crt,' src/3rdparty/chromium/third_party/boringssl/src/crypto/x509/x509_def.c
pkgs/games/factorio/fetch.sh: --cacert $cacert/etc/ssl/certs/ca-bundle.crt \
pkgs/servers/xmpp/ejabberd/default.nix:    GIT_SSL_CAINFO = "${cacert}/etc/ssl/certs/ca-bundle.crt";

I strongly suspect that some of those usages are either not needed or are plain wrong, given all that movement with NIX_SSL_CERT_FILE and attempts to make packages behave use trusted roots from the system as opposed to pulling pkgs.cacert.

kirelagin · 2017-05-31T13:31:24Z

More details on fetchdarcs. Darcs uses libcurl and there is no code in Darcs that would make it possible to configure libcurl to skip peer verification (this has to be done by calling curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, FALSE)). I also don’t think there is a way to influence the options of libcurl via environment, so it looks like, indeed, the only choice we have is to allow darcs to verify its peers.

kirelagin · 2017-05-31T13:35:48Z

Also, fetchrepoproject passes pkgs.cacert to the builder. I’m not really sure if it is needed. repo itself is implemented in Python and does some https calls on its own, so this probably makes sense...

7c6f434c · 2017-05-31T13:38:55Z

I think we have a larger problem of expectation mismatch… It is sometimes very convenient to just bump the version in the expression and get the hash from the error message. But fetchurl doesn't verify SSL certificates, so this workflow is insecure…

kirelagin · 2017-05-31T13:43:50Z

@7c6f434c Yeah, you are right. This is a troubling issue indeed, although, honestly, I don’t think it is critical. Even if the original author of the expression gets MITMed, others will quickly figure this out.

But it would much better if there was a warning or even if the verification was automatically switched all somehow when updating expressions... Not sure how this would work. Do you wanna file a separate issue? :)

7c6f434c · 2017-05-31T13:52:17Z

@7c6f434c Yeah, you are right. This is a troubling issue indeed, although, honestly, I don’t think it is critical. Even if the original author of the expression gets MITMed, others will quickly figure this out. But it would much better if there was a warning or even if the verification was automatically switched all somehow when updating expressions... Not sure how this would work. Do you wanna file a separate issue? :)

Well, nix-prefetch-git is even more annoying to use than nix-prefetch-url so I decided to comment on the «not needed anyway» here, mostly so that this statement doesn't get to be written without the fine print. If I had a _good_ solution I would probably have committed it already, also not sure what should happen…

shlevy · 2017-05-31T14:16:03Z

Strongly opposed to this. I'd rather not give any MITM insight into which specific github repos I'm downloading, thanks.

kirelagin · 2017-05-31T14:30:43Z

@shlevy Can you please submit a negative review so that it is clearly visible?

An opposite option would be to modify fetchurl and all the other fetch* functions to make sure they are forcing TLS verification. The question then is which bundle of root certificates do we want to use when fetching? These have to be system-independent, otherwise this would be an impurity (admittedly a minor one). And we will have to make sure that path to the bundle does not get stored in the binaries. See also #8247.

shlevy

Result integrity is not the only reason to use SSL. We shouldn't expose the specific packages our users download to a clever enough MITM

shlevy · 2017-05-31T14:35:43Z

The question then is which bundle of root certificates do we want to use when fetching? These have to be system-independent, otherwise this would be an impurity (admittedly a minor one).

Why do these have to be system-independent? They're not an impurity by virtue of being fixed output. Also seems like a plausible case for impureEnvVars.

vcunat · 2017-05-31T14:40:06Z

It would be impurity in the sense of failure vs. success, not in the sense of getting a different output.

shlevy · 2017-05-31T15:01:13Z

So is the existing proxy stuff, or the fact of internet connection at all

vcunat · 2017-06-01T07:34:36Z

IIRC nixos.org is tried first.

grahamc

I disagree for several reasons:

Authenticity is just one feature of SSL, the privacy of our users is also paramount.
Our update strategy is frequently TOFU, or"break the hash then use the fixed one," and I don't want to require a user upstream their packages in order for them to find out their upgrade was pwned.
cacert is a dependency of the fixed output derivation, which means if cacert changes all fixed outputs don't have to be updated.

I think the upsides here are very minimal, for potentially tragic downsides.

FlorentBecker · 2017-06-26T08:53:30Z

Should we just close this?

fpletz · 2017-06-26T08:59:53Z

I don't think that most project members and users will agree to this change.

kirelagin added 2 commits May 31, 2017 16:14

fetchgit: do not verify SSL peer

50cbd07

fetchrepoproject: do not verify SSL peer

7792ebd

kirelagin mentioned this pull request May 31, 2017

fetchdarcs: add SSL_CERT_FILE environment variable #25251

Merged

shlevy requested changes May 31, 2017

View reviewed changes

grahamc requested changes Jun 3, 2017

View reviewed changes

FRidh added the 9.needs: community feedback label Jun 6, 2017

fpletz closed this Jun 26, 2017

aneeshusa mentioned this pull request Sep 25, 2017

dockerTools: Fix pullImage with sandboxing enabled #29777

Closed

8 tasks

lukateras added the 6.topic: fetch label Dec 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not verify SSL peer when fetching sources #26273

Do not verify SSL peer when fetching sources #26273

kirelagin commented May 31, 2017 •

edited

kirelagin commented May 31, 2017

kirelagin commented May 31, 2017

7c6f434c commented May 31, 2017

kirelagin commented May 31, 2017

7c6f434c commented May 31, 2017 via email

shlevy commented May 31, 2017

kirelagin commented May 31, 2017

shlevy left a comment

shlevy commented May 31, 2017

vcunat commented May 31, 2017

shlevy commented May 31, 2017

vcunat commented Jun 1, 2017

grahamc left a comment •

edited

FlorentBecker commented Jun 26, 2017

fpletz commented Jun 26, 2017

Do not verify SSL peer when fetching sources #26273

Do not verify SSL peer when fetching sources #26273

Conversation

kirelagin commented May 31, 2017 • edited

kirelagin commented May 31, 2017

kirelagin commented May 31, 2017

7c6f434c commented May 31, 2017

kirelagin commented May 31, 2017

7c6f434c commented May 31, 2017 via email

shlevy commented May 31, 2017

kirelagin commented May 31, 2017

shlevy left a comment

Choose a reason for hiding this comment

shlevy commented May 31, 2017

vcunat commented May 31, 2017

shlevy commented May 31, 2017

vcunat commented Jun 1, 2017

grahamc left a comment • edited

Choose a reason for hiding this comment

FlorentBecker commented Jun 26, 2017

fpletz commented Jun 26, 2017

kirelagin commented May 31, 2017 •

edited

grahamc left a comment •

edited