From 7e46d4077a15ca711ebd0141ca075cedbaf1a41c Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 23 Sep 2024 15:09:44 +0200 Subject: [PATCH 1/5] builtin:fetchurl: Enable TLS verification This is better for privacy and to avoid leaking netrc credentials in a MITM attack, but also the assumption that we check the hash no longer holds in some cases (in particular for impure derivations). Partially reverts https://github.com/NixOS/nix/commit/5db358d4d78aea7204a8f22c5bf2a309267ee038. (cherry picked from commit c04bc17a5a0fdcb725a11ef6541f94730112e7b6) --- src/libstore/builtins/fetchurl.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/libstore/builtins/fetchurl.cc b/src/libstore/builtins/fetchurl.cc index 559efcc17..3f4a3076e 100644 --- a/src/libstore/builtins/fetchurl.cc +++ b/src/libstore/builtins/fetchurl.cc @@ -41,10 +41,7 @@ void builtinFetchurl(const BasicDerivation & drv, const std::string & netrcData) auto source = sinkToSource([&](Sink & sink) { - /* No need to do TLS verification, because we check the hash of - the result anyway. */ FileTransferRequest request(url); - request.verifyTLS = false; request.decompress = false; auto decompressor = makeDecompressionSink( From 10e3c1631e1295b6e3a7f2e49de70fbfdf8079be Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 24 Sep 2024 16:13:28 +0200 Subject: [PATCH 2/5] Add a test for builtin:fetchurl cert verification (cherry picked from commit f2f47fa725fc87bfb536de171a2ea81f2789c9fb) # Conflicts: # tests/nixos/default.nix --- tests/nixos/default.nix | 11 ++++++ tests/nixos/fetchurl.nix | 78 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 tests/nixos/fetchurl.nix diff --git a/tests/nixos/default.nix b/tests/nixos/default.nix index 43247b80a..a9235ca64 100644 --- a/tests/nixos/default.nix +++ b/tests/nixos/default.nix @@ -160,4 +160,15 @@ in ca-fd-leak = runNixOSTestFor "x86_64-linux" ./ca-fd-leak; user-sandboxing = runNixOSTestFor "x86_64-linux" ./user-sandboxing; +<<<<<<< HEAD +======= + + s3-binary-cache-store = runNixOSTestFor "x86_64-linux" ./s3-binary-cache-store.nix; + + fsync = runNixOSTestFor "x86_64-linux" ./fsync.nix; + + cgroups = runNixOSTestFor "x86_64-linux" ./cgroups; + + fetchurl = runNixOSTestFor "x86_64-linux" ./fetchurl.nix; +>>>>>>> f2f47fa72 (Add a test for builtin:fetchurl cert verification) } diff --git a/tests/nixos/fetchurl.nix b/tests/nixos/fetchurl.nix new file mode 100644 index 000000000..476f779bc --- /dev/null +++ b/tests/nixos/fetchurl.nix @@ -0,0 +1,78 @@ +# Test whether builtin:fetchurl properly performs TLS certificate +# checks on HTTPS servers. + +{ lib, config, pkgs, ... }: + +let + + makeTlsCert = name: pkgs.runCommand name { + nativeBuildInputs = with pkgs; [ openssl ]; + } '' + mkdir -p $out + openssl req -x509 \ + -subj '/CN=${name}/' -days 49710 \ + -addext 'subjectAltName = DNS:${name}' \ + -keyout "$out/key.pem" -newkey ed25519 \ + -out "$out/cert.pem" -noenc + ''; + + goodCert = makeTlsCert "good"; + badCert = makeTlsCert "bad"; + +in + +{ + name = "nss-preload"; + + nodes = { + machine = { lib, pkgs, ... }: { + services.nginx = { + enable = true; + + virtualHosts."good" = { + addSSL = true; + sslCertificate = "${goodCert}/cert.pem"; + sslCertificateKey = "${goodCert}/key.pem"; + root = pkgs.runCommand "nginx-root" {} '' + mkdir "$out" + echo 'hello world' > "$out/index.html" + ''; + }; + + virtualHosts."bad" = { + addSSL = true; + sslCertificate = "${badCert}/cert.pem"; + sslCertificateKey = "${badCert}/key.pem"; + root = pkgs.runCommand "nginx-root" {} '' + mkdir "$out" + echo 'foobar' > "$out/index.html" + ''; + }; + }; + + security.pki.certificateFiles = [ "${goodCert}/cert.pem" ]; + + networking.hosts."127.0.0.1" = [ "good" "bad" ]; + + virtualisation.writableStore = true; + + nix.settings.experimental-features = "nix-command"; + }; + }; + + testScript = { nodes, ... }: '' + machine.wait_for_unit("nginx") + machine.wait_for_open_port(443) + + out = machine.succeed("curl https://good/index.html") + assert out == "hello world\n" + + # Fetching from a server with a trusted cert should work. + machine.succeed("nix build --no-substitute --expr 'import { url = \"https://good/index.html\"; hash = \"sha256-qUiQTy8PR5uPgZdpSzAYSw0u0cHNKh7A+4XSmaGSpEc=\"; }'") + + # Fetching from a server with an untrusted cert should fail. + err = machine.fail("nix build --no-substitute --expr 'import { url = \"https://bad/index.html\"; hash = \"sha256-rsBwZF/lPuOzdjBZN2E08FjMM3JHyXit0Xi2zN+wAZ8=\"; }' 2>&1") + print(err) + assert "SSL certificate problem: self-signed certificate" in err + ''; +} From b91412595bd297129021ef5baae507cad8c0a6cb Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 25 Sep 2024 22:33:50 +0200 Subject: [PATCH 3/5] Add release note (cherry picked from commit 7b39cd631e0d3c3d238015c6f450c59bbc9cbc5b) --- doc/manual/rl-next/verify-tls.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 doc/manual/rl-next/verify-tls.md diff --git a/doc/manual/rl-next/verify-tls.md b/doc/manual/rl-next/verify-tls.md new file mode 100644 index 000000000..489941d5b --- /dev/null +++ b/doc/manual/rl-next/verify-tls.md @@ -0,0 +1,8 @@ +--- +synopsis: "`` uses TLS verification" +prs: [11585] +--- + +Previously `` did not do TLS verification. This was because the Nix sandbox in the past did not have access to TLS certificates, and Nix checks the hash of the fetched file anyway. However, this can expose authentication data from `netrc` and URLs to man-in-the-middle attackers. In addition, Nix now in some cases (such as when using impure derivations) does *not* check the hash. Therefore we have now enabled TLS verification. This means that downloads by `` will now fail if you're fetching from a HTTPS server that does not have a valid certificate. + +`` is also known as the builtin derivation builder `builtin:fetchurl`. It's not to be confused with the evaluation-time function `builtins.fetchurl`, which was not affected by this issues. From 708ea7cf7fc5df4b0f3f3f24b04ce9c88d81b2df Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 26 Sep 2024 00:15:04 +0200 Subject: [PATCH 4/5] Typo (cherry picked from commit ef8987955be337976ae229c44870cf6adc43bba5) --- doc/manual/rl-next/verify-tls.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/manual/rl-next/verify-tls.md b/doc/manual/rl-next/verify-tls.md index 489941d5b..afc689f46 100644 --- a/doc/manual/rl-next/verify-tls.md +++ b/doc/manual/rl-next/verify-tls.md @@ -5,4 +5,4 @@ prs: [11585] Previously `` did not do TLS verification. This was because the Nix sandbox in the past did not have access to TLS certificates, and Nix checks the hash of the fetched file anyway. However, this can expose authentication data from `netrc` and URLs to man-in-the-middle attackers. In addition, Nix now in some cases (such as when using impure derivations) does *not* check the hash. Therefore we have now enabled TLS verification. This means that downloads by `` will now fail if you're fetching from a HTTPS server that does not have a valid certificate. -`` is also known as the builtin derivation builder `builtin:fetchurl`. It's not to be confused with the evaluation-time function `builtins.fetchurl`, which was not affected by this issues. +`` is also known as the builtin derivation builder `builtin:fetchurl`. It's not to be confused with the evaluation-time function `builtins.fetchurl`, which was not affected by this issue. From ebff89a4e5a6ad77431003d07a5aef916d53f4b4 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 26 Sep 2024 00:17:03 +0200 Subject: [PATCH 5/5] Resolve conflict --- tests/nixos/default.nix | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/nixos/default.nix b/tests/nixos/default.nix index a9235ca64..0b6f20551 100644 --- a/tests/nixos/default.nix +++ b/tests/nixos/default.nix @@ -160,15 +160,6 @@ in ca-fd-leak = runNixOSTestFor "x86_64-linux" ./ca-fd-leak; user-sandboxing = runNixOSTestFor "x86_64-linux" ./user-sandboxing; -<<<<<<< HEAD -======= - - s3-binary-cache-store = runNixOSTestFor "x86_64-linux" ./s3-binary-cache-store.nix; - - fsync = runNixOSTestFor "x86_64-linux" ./fsync.nix; - - cgroups = runNixOSTestFor "x86_64-linux" ./cgroups; fetchurl = runNixOSTestFor "x86_64-linux" ./fetchurl.nix; ->>>>>>> f2f47fa72 (Add a test for builtin:fetchurl cert verification) }