Skip to content
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

Ensure error messages don't leak private key #11523

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
edolstra marked this conversation as resolved.
Show resolved Hide resolved
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);
Comment on lines +31 to +32
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need sensitiveValue? I think the context gives enough info.

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh it's pretty self-contained here like the other version (The Key constructor is now protected so it won't get used elsewhere).

I am inclined to keep it also because the thing I need to debug is a public key not private 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
5 changes: 2 additions & 3 deletions src/libutil/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,8 @@ std::string base64Decode(std::string_view s)
if (c == '\n') continue;

char digit = base64DecodeChars[(unsigned char) c];
if (digit == npos) {
throw Error("invalid character in Base64 string: '%c' in '%s'", c, s.data());
}
if (digit == npos)
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
Loading