From 0cd64420d18504ff0d5fb69d20ee49e8870b6d84 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/libfetchers/git-utils.cc | 8 +++++++- src/libstore/machines.cc | 5 +++-- src/libstore/ssh.cc | 14 ++++++++++++-- src/libstore/ssh.hh | 3 +++ src/libutil/hash.cc | 7 ++++++- src/libutil/signature/local-keys.cc | 29 +++++++++++++++++++++-------- src/libutil/signature/local-keys.hh | 12 ++++++++---- src/libutil/util.cc | 2 +- src/libutil/util.hh | 6 +++++- tests/unit/libexpr/nix_api_expr.cc | 2 +- 10 files changed, 67 insertions(+), 21 deletions(-) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index d5acd319049..d52bade6abe 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -583,7 +583,13 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this 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 + ")"; } diff --git a/src/libstore/machines.cc b/src/libstore/machines.cc index 256cf918892..5e038fb28d3 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 b8c5f4d975e..dec733fd516 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 19b30e8838f..4097134d055 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/hash.cc b/src/libutil/hash.cc index ab2a8695dd4..748176d3370 100644 --- a/src/libutil/hash.cc +++ b/src/libutil/hash.cc @@ -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); diff --git a/src/libutil/signature/local-keys.cc b/src/libutil/signature/local-keys.cc index 00c4543f2be..045d9cd173f 100644 --- a/src/libutil/signature/local-keys.cc +++ b/src/libutil/signature/local-keys.cc @@ -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 @@ -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"); @@ -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"); @@ -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"); diff --git a/src/libutil/signature/local-keys.hh b/src/libutil/signature/local-keys.hh index 4aafc123944..9977f0dac6e 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 db3ed1ddfa8..0d728e2d5dc 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -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; diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 25128a9009f..75a4af76e8c 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -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); diff --git a/tests/unit/libexpr/nix_api_expr.cc b/tests/unit/libexpr/nix_api_expr.cc index 8b97d692345..b37ac44b317 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 {