Merge pull request #11523 from obsidiansystems/base64Decode-no-leak-private-key-on-error

Ensure error messages don't leak private key

src/libfetchers/git-utils.cc

@@ -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 + ")";
         }

src/libstore/machines.cc

@@ -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;
     };

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)

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;

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);

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;
 
+    try {
         if (name == "" || key == "")
-        throw Error("key is corrupt");
+            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");
 

src/libutil/signature/local-keys.hh

@@ -31,15 +31,19 @@ struct Key
     std::string name;
     std::string key;
 
-    /**
-     * Construct Key from a string in the format
-     * ‘<name>:<key-in-base64>’.
-     */
-    Key(std::string_view s);
-
     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, bool sensitiveValue);
+
     Key(std::string_view name, std::string && key)
         : name(name), key(std::move(key)) { }
 };

src/libutil/util.cc

@@ -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;

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);
 
 

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 <gmock/gmock.h>
 #include <gtest/gtest.h>
 
 namespace nixC {