From d8b36268c81c650961ff5b83e41bcdd6b28516b7 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 17 Sep 2024 15:25:30 -0400 Subject: [PATCH] Ensure error messages don't leak private key 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. --- src/libstore/machines.cc | 5 +++-- src/libstore/ssh.cc | 14 ++++++++++++-- src/libstore/ssh.hh | 3 +++ src/libutil/signature/local-keys.cc | 17 +++++++++++------ src/libutil/signature/local-keys.hh | 12 ++++++++---- src/libutil/util.cc | 7 +++++-- src/libutil/util.hh | 11 +++++++++-- tests/unit/libexpr/nix_api_expr.cc | 2 +- tests/unit/libutil/util.cc | 15 ++++++++++++++- 9 files changed, 66 insertions(+), 20 deletions(-) diff --git a/src/libstore/machines.cc b/src/libstore/machines.cc index 256cf918892e..5e038fb28d36 100644 --- a/src/libstore/machines.cc +++ b/src/libstore/machines.cc @@ -159,8 +159,9 @@ static Machine parseBuilderLine(const std::set & 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; }; diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc index b8c5f4d975ef..dec733fd5161 100644 --- a/src/libstore/ssh.cc +++ b/src/libstore/ssh.cc @@ -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, @@ -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) @@ -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) diff --git a/src/libstore/ssh.hh b/src/libstore/ssh.hh index 19b30e8838fb..4097134d0553 100644 --- a/src/libstore/ssh.hh +++ b/src/libstore/ssh.hh @@ -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; diff --git a/src/libutil/signature/local-keys.cc b/src/libutil/signature/local-keys.cc index 00c4543f2be0..3d8e5c279a2b 100644 --- a/src/libutil/signature/local-keys.cc +++ b/src/libutil/signature/local-keys.cc @@ -14,17 +14,22 @@ 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"); + try { + if (name == "" || key == "") + throw FormatError("key is corrupt"); - key = base64Decode(key); + key = base64Decode(key); + } catch (Error & e) { + e.addTrace({}, "while decoding key named '%s'", name); + throw; + } } std::string Key::to_string() const @@ -33,7 +38,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"); @@ -66,7 +71,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"); diff --git a/src/libutil/signature/local-keys.hh b/src/libutil/signature/local-keys.hh index 4aafc123944a..9977f0dac6ed 100644 --- a/src/libutil/signature/local-keys.hh +++ b/src/libutil/signature/local-keys.hh @@ -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 * ‘:’. + * + * @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)) { } }; diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 6f7a4299de21..149a7a8843d7 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -220,7 +220,7 @@ std::string base64Encode(std::string_view s) } -std::string base64Decode(std::string_view s) +std::string base64Decode(std::string_view s, bool sensitiveValue) { constexpr char npos = -1; constexpr std::array base64DecodeChars = [&] { @@ -244,7 +244,10 @@ std::string base64Decode(std::string_view s) char digit = base64DecodeChars[(unsigned char) c]; if (digit == npos) { - throw Error("invalid character in Base64 string: '%c' in '%s'", c, s.data()); + auto msg = sensitiveValue + ? "" + : fmt("'%s'", s.data()); + throw FormatError("invalid character in Base64 string: '%c' in %s", c, msg); } bits += 6; diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 25128a9009fd..73e2f910897d 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -172,10 +172,17 @@ constexpr char treeNull[] = " "; /** - * Base64 encoding/decoding. + * Encode arbitrary bytes as Base64. */ std::string base64Encode(std::string_view s); -std::string base64Decode(std::string_view s); + +/** + * Decode arbitrary bytes to Base64. + * + * @param sensitiveValue Avoid displaying the raw Base64 in error messages, + * e.g. to avoid leaking private keys. + */ +std::string base64Decode(std::string_view s, bool sensitiveValue = false); /** diff --git a/tests/unit/libexpr/nix_api_expr.cc b/tests/unit/libexpr/nix_api_expr.cc index 8b97d6923459..b37ac44b3172 100644 --- a/tests/unit/libexpr/nix_api_expr.cc +++ b/tests/unit/libexpr/nix_api_expr.cc @@ -8,7 +8,7 @@ #include "tests/nix_api_expr.hh" #include "tests/string_callback.hh" -#include "gmock/gmock.h" +#include #include namespace nixC { diff --git a/tests/unit/libutil/util.cc b/tests/unit/libutil/util.cc index a3f7c720a5c6..af3401118193 100644 --- a/tests/unit/libutil/util.cc +++ b/tests/unit/libutil/util.cc @@ -6,6 +6,7 @@ #include #include +#include #include @@ -99,7 +100,19 @@ TEST(base64Decode, decodeAString) TEST(base64Decode, decodeThrowsOnInvalidChar) { - ASSERT_THROW(base64Decode("cXVvZCBlcm_0IGRlbW9uc3RyYW5kdW0="), Error); + ASSERT_THROW(base64Decode("cXVvZCBlcm_0IGRlbW9uc3RyYW5kdW0="), FormatError); +} + +TEST(base64Decode, decodeThrowsNoLeak) +{ + std::string msg = "cXVvZCBlcm_0IGRlbW9uc3RyYW5kdW0="; + try { + base64Decode(msg, true); + } catch (FormatError & e) { + EXPECT_THAT(e.msg(), testing::Not(testing::HasSubstr(msg))); + return; + } + EXPECT_TRUE(false); } /* ----------------------------------------------------------------------------