Skip to content

Commit

Permalink
Disable TLS verification for builtin fetchurl
Browse files Browse the repository at this point in the history
This makes it consistent with the Nixpkgs fetchurl and makes it work
in chroots. We don't need verification because the hash of the result
is checked anyway.
  • Loading branch information
edolstra committed Oct 21, 2015
1 parent 357d31b commit 5db358d
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 12 deletions.
8 changes: 7 additions & 1 deletion src/libstore/builtins.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,13 @@ void builtinFetchurl(const BasicDerivation & drv)
auto url = drv.env.find("url");
if (url == drv.env.end()) throw Error("attribute ‘url’ missing");
printMsg(lvlInfo, format("downloading ‘%1%’...") % url->second);
auto data = downloadFile(url->second); // FIXME: show progress

/* No need to do TLS verification, because we check the hash of
the result anyway. */
DownloadOptions options;
options.verifyTLS = false;

auto data = downloadFile(url->second, options); // FIXME: show progress

auto out = drv.env.find("out");
if (out == drv.env.end()) throw Error("attribute ‘url’ missing");
Expand Down
26 changes: 17 additions & 9 deletions src/libstore/download.cc
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ struct Curl
if (!curl) throw Error("unable to initialize curl");

curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L);
curl_easy_setopt(curl, CURLOPT_CAINFO, getEnv("SSL_CERT_FILE", "/etc/ssl/certs/ca-certificates.crt").c_str());
curl_easy_setopt(curl, CURLOPT_USERAGENT, ("Nix/" + nixVersion).c_str());
curl_easy_setopt(curl, CURLOPT_FAILONERROR, 1);

Expand All @@ -125,20 +124,27 @@ struct Curl
if (requestHeaders) curl_slist_free_all(requestHeaders);
}

bool fetch(const string & url, const string & expectedETag = "")
bool fetch(const string & url, const DownloadOptions & options)
{
curl_easy_setopt(curl, CURLOPT_URL, url.c_str());

if (options.verifyTLS)
curl_easy_setopt(curl, CURLOPT_CAINFO, getEnv("SSL_CERT_FILE", "/etc/ssl/certs/ca-certificates.crt").c_str());
else {
curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0);
curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0);
}

data.clear();

if (requestHeaders) {
curl_slist_free_all(requestHeaders);
requestHeaders = 0;
}

if (!expectedETag.empty()) {
this->expectedETag = expectedETag;
requestHeaders = curl_slist_append(requestHeaders, ("If-None-Match: " + expectedETag).c_str());
if (!options.expectedETag.empty()) {
this->expectedETag = options.expectedETag;
requestHeaders = curl_slist_append(requestHeaders, ("If-None-Match: " + options.expectedETag).c_str());
}

curl_easy_setopt(curl, CURLOPT_HTTPHEADER, requestHeaders);
Expand All @@ -154,7 +160,7 @@ struct Curl
//std::cerr << "\e[" << moveBack << "D\e[K\n";
std::cerr << "\n";
checkInterrupt();
if (res == CURLE_WRITE_ERROR && etag == expectedETag) return false;
if (res == CURLE_WRITE_ERROR && etag == options.expectedETag) return false;
if (res != CURLE_OK)
throw DownloadError(format("unable to download ‘%1%’: %2% (%3%)")
% url % curl_easy_strerror(res) % res);
Expand All @@ -168,11 +174,11 @@ struct Curl
};


DownloadResult downloadFile(string url, string expectedETag)
DownloadResult downloadFile(string url, const DownloadOptions & options)
{
DownloadResult res;
Curl curl;
if (curl.fetch(url, expectedETag)) {
if (curl.fetch(url, options)) {
res.cached = false;
res.data = curl.data;
} else
Expand Down Expand Up @@ -224,7 +230,9 @@ Path downloadFileCached(const string & url, bool unpack)
if (!skip) {

try {
auto res = downloadFile(url, expectedETag);
DownloadOptions options;
options.expectedETag = expectedETag;
auto res = downloadFile(url, options);

if (!res.cached)
storePath = store->addTextToStore(name, res.data, PathSet(), false);
Expand Down
8 changes: 7 additions & 1 deletion src/libstore/download.hh
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,19 @@

namespace nix {

struct DownloadOptions
{
string expectedETag;
bool verifyTLS{true};
};

struct DownloadResult
{
bool cached;
string data, etag;
};

DownloadResult downloadFile(string url, string expectedETag = "");
DownloadResult downloadFile(string url, const DownloadOptions & options);

Path downloadFileCached(const string & url, bool unpack);

Expand Down
2 changes: 1 addition & 1 deletion src/nix-prefetch-url/nix-prefetch-url.cc
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ int main(int argc, char * * argv)
auto actualUri = resolveMirrorUri(state, uri);

/* Download the file. */
auto result = downloadFile(actualUri);
auto result = downloadFile(actualUri, DownloadOptions());

AutoDelete tmpDir(createTempDir(), true);
Path tmpFile = (Path) tmpDir + "/tmp";
Expand Down

6 comments on commit 5db358d

@LnL7
Copy link
Member

@LnL7 LnL7 commented on 5db358d Feb 18, 2019

Choose a reason for hiding this comment

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

What's the reasoning behind this? I understand making certificates available inside a derivation is tricky, but with 0a2bee3 that's not necessary anymore. While not strictly necessary it seems desirable to have verification on (at least by default).

@vcunat
Copy link
Member

@vcunat vcunat commented on 5db358d Feb 18, 2019

Choose a reason for hiding this comment

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

In general, I think it would be nicer to still verify certificates by default; two possible reasons:

  • slight privacy improvement, as there could be a man-in-the-middle watching what you (attempt to) download;
  • the workflow where you fetchurl with an incorrect hash to find the correct one; it seems relatively common to do that, sometimes for lack of a practical alternative (like in case of fetchpatch)

@LnL7
Copy link
Member

@LnL7 LnL7 commented on 5db358d Feb 18, 2019

Choose a reason for hiding this comment

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

Yeah, I would like to replace this with a nix.conf option but wanted to check since this was added explicitly.

@7c6f434c
Copy link
Member

Choose a reason for hiding this comment

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

Privacy improvement is a bit less than they seem, as the target server and package size are usually possible to observe. TOFU is a popular workflow, though.

@vcunat
Copy link
Member

@vcunat vcunat commented on 5db358d Feb 18, 2019

Choose a reason for hiding this comment

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

Right, it's not much. The privacy point only holds more significantly for binary caches, which is something else.

@nixos-discourse
Copy link

Choose a reason for hiding this comment

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

This commit has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-2-24-8-release-to-fix-builtin-fetchurl-security-issue/52732/1

Please sign in to comment.