Skip to content

Commit

Permalink
Ensure error messages don't leak private key
Browse files Browse the repository at this point in the history
Since #8766, invalid base64 is rendered in errors, but we don't actually
want to show this in the case of an invalid private keys.
  • Loading branch information
Ericson2314 committed Sep 20, 2024
1 parent d0c351b commit 0cd6442
Show file tree
Hide file tree
Showing 10 changed files with 67 additions and 21 deletions.
8 changes: 7 additions & 1 deletion src/libfetchers/git-utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,13 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this<GitRepoImpl>
std::string re = R"(Good "git" signature for \* with .* key SHA256:[)";
for (const fetchers::PublicKey & k : publicKeys){
// Calculate sha256 fingerprint from public key and escape the regex symbol '+' to match the key literally
auto fingerprint = trim(hashString(HashAlgorithm::SHA256, base64Decode(k.key)).to_string(nix::HashFormat::Base64, false), "=");
std::string keyDecoded;
try {
keyDecoded = base64Decode(k.key);
} catch (Error & e) {
e.addTrace({}, "While decoding public key '%s' used for git signature", k.key);
}
auto fingerprint = trim(hashString(HashAlgorithm::SHA256, keyDecoded).to_string(nix::HashFormat::Base64, false), "=");
auto escaped_fingerprint = std::regex_replace(fingerprint, std::regex("\\+"), "\\+" );
re += "(" + escaped_fingerprint + ")";
}
Expand Down
5 changes: 3 additions & 2 deletions src/libstore/machines.cc
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,9 @@ static Machine parseBuilderLine(const std::set<std::string> & defaultSystems, co
const auto & str = tokens[fieldIndex];
try {
base64Decode(str);
} catch (const Error & e) {
throw FormatError("bad machine specification: a column #%lu in a row: '%s' is not valid base64 string: %s", fieldIndex, line, e.what());
} catch (FormatError & e) {
e.addTrace({}, "while parsing machine specification at a column #%lu in a row: '%s'", fieldIndex, line);
throw;
}
return str;
};
Expand Down
14 changes: 12 additions & 2 deletions src/libstore/ssh.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@

namespace nix {

static std::string parsePublicHostKey(std::string_view host, std::string_view sshPublicHostKey)
{
try {
return base64Decode(sshPublicHostKey);
} catch (Error & e) {
e.addTrace({}, "while decoding ssh public host key for host '%s'", host);
throw;
}
}

SSHMaster::SSHMaster(
std::string_view host,
std::string_view keyFile,
Expand All @@ -15,7 +25,7 @@ SSHMaster::SSHMaster(
: host(host)
, fakeSSH(host == "localhost")
, keyFile(keyFile)
, sshPublicHostKey(sshPublicHostKey)
, sshPublicHostKey(parsePublicHostKey(host, sshPublicHostKey))
, useMaster(useMaster && !fakeSSH)
, compress(compress)
, logFD(logFD)
Expand All @@ -39,7 +49,7 @@ void SSHMaster::addCommonSSHOpts(Strings & args)
std::filesystem::path fileName = state->tmpDir->path() / "host-key";
auto p = host.rfind("@");
std::string thost = p != std::string::npos ? std::string(host, p + 1) : host;
writeFile(fileName.string(), thost + " " + base64Decode(sshPublicHostKey) + "\n");
writeFile(fileName.string(), thost + " " + sshPublicHostKey + "\n");
args.insert(args.end(), {"-oUserKnownHostsFile=" + fileName.string()});
}
if (compress)
Expand Down
3 changes: 3 additions & 0 deletions src/libstore/ssh.hh
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ private:
const std::string host;
bool fakeSSH;
const std::string keyFile;
/**
* Raw bytes, not Base64 encoding.
*/
const std::string sshPublicHostKey;
const bool useMaster;
const bool compress;
Expand Down
7 changes: 6 additions & 1 deletion src/libutil/hash.cc
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,12 @@ Hash::Hash(std::string_view rest, HashAlgorithm algo, bool isSRI)
}

else if (isSRI || rest.size() == base64Len()) {
auto d = base64Decode(rest);
std::string d;
try {
d = base64Decode(rest);
} catch (Error & e) {
e.addTrace({}, "While decoding hash '%s'", rest);
}
if (d.size() != hashSize)
throw BadHash("invalid %s hash '%s'", isSRI ? "SRI" : "base-64", rest);
assert(hashSize);
Expand Down
29 changes: 21 additions & 8 deletions src/libutil/signature/local-keys.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,25 @@ BorrowedCryptoValue BorrowedCryptoValue::parse(std::string_view s)
return {s.substr(0, colon), s.substr(colon + 1)};
}

Key::Key(std::string_view s)
Key::Key(std::string_view s, bool sensitiveValue)
{
auto ss = BorrowedCryptoValue::parse(s);

name = ss.name;
key = ss.payload;

if (name == "" || key == "")
throw Error("key is corrupt");

key = base64Decode(key);
try {
if (name == "" || key == "")
throw FormatError("key is corrupt");

key = base64Decode(key);
} catch (Error & e) {
std::string extra;
if (!sensitiveValue)
extra = fmt(" with raw value '%s'", key);
e.addTrace({}, "while decoding key named '%s'%s", name, extra);
throw;
}
}

std::string Key::to_string() const
Expand All @@ -33,7 +41,7 @@ std::string Key::to_string() const
}

SecretKey::SecretKey(std::string_view s)
: Key(s)
: Key{s, true}
{
if (key.size() != crypto_sign_SECRETKEYBYTES)
throw Error("secret key is not valid");
Expand Down Expand Up @@ -66,7 +74,7 @@ SecretKey SecretKey::generate(std::string_view name)
}

PublicKey::PublicKey(std::string_view s)
: Key(s)
: Key{s, false}
{
if (key.size() != crypto_sign_PUBLICKEYBYTES)
throw Error("public key is not valid");
Expand All @@ -83,7 +91,12 @@ bool PublicKey::verifyDetached(std::string_view data, std::string_view sig) cons

bool PublicKey::verifyDetachedAnon(std::string_view data, std::string_view sig) const
{
auto sig2 = base64Decode(sig);
std::string sig2;
try {
sig2 = base64Decode(sig);
} catch (Error & e) {
e.addTrace({}, "While decoding signature '%s'", sig);
}
if (sig2.size() != crypto_sign_BYTES)
throw Error("signature is not valid");

Expand Down
12 changes: 8 additions & 4 deletions src/libutil/signature/local-keys.hh
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,19 @@ struct Key
std::string name;
std::string key;

std::string to_string() const;

protected:

/**
* Construct Key from a string in the format
* ‘<name>:<key-in-base64>’.
*
* @param sensitiveValue Avoid displaying the raw Base64 in error
* messages to avoid leaking private keys.
*/
Key(std::string_view s);

std::string to_string() const;
Key(std::string_view s, bool sensitiveValue);

protected:
Key(std::string_view name, std::string && key)
: name(name), key(std::move(key)) { }
};
Expand Down
2 changes: 1 addition & 1 deletion src/libutil/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ std::string base64Decode(std::string_view s)

char digit = base64DecodeChars[(unsigned char) c];
if (digit == npos)
throw Error("invalid character in Base64 string: '%c'", c);
throw FormatError("invalid character in Base64 string: '%c'", c);

bits += 6;
d = d << 6 | digit;
Expand Down
6 changes: 5 additions & 1 deletion src/libutil/util.hh
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,13 @@ constexpr char treeNull[] = " ";


/**
* Base64 encoding/decoding.
* Encode arbitrary bytes as Base64.
*/
std::string base64Encode(std::string_view s);

/**
* Decode arbitrary bytes to Base64.
*/
std::string base64Decode(std::string_view s);


Expand Down
2 changes: 1 addition & 1 deletion tests/unit/libexpr/nix_api_expr.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#include "tests/nix_api_expr.hh"
#include "tests/string_callback.hh"

#include "gmock/gmock.h"
#include <gmock/gmock.h>
#include <gtest/gtest.h>

namespace nixC {
Expand Down

0 comments on commit 0cd6442

Please sign in to comment.