Merge bitcoin/bitcoin#28500: Prevent default/invalid CKey objects from allocating secure memory

6ef405d key: don't allocate secure mem for null (invalid) key (Pieter Wuille)
d9841a7 Add make_secure_unique helper (Anthony Towns)

Pull request description:

  Bitcoin Core has `secure_allocator`, which allocates inside special "secure" (non-swappable) memory pages, which may be limited in availability. Currently, every `CKey` object uses 32 such secure bytes, even when the `CKey` object contains the (invalid) value zero.

  Change this to not use memory when the `CKey` is invalid. This is particularly relevant for `BIP324Cipher` which briefly holds a `CKey`, but after receiving the remote's public key and initializing the encryption ciphers, the key is wiped. In case secure memory usage is in high demand, it'd be silly to waste it on P2P encryption keys instead of wallet keys.

ACKs for top commit:
  ajtowns:
    ACK 6ef405d
  john-moffett:
    ACK 6ef405d

Tree-SHA512: 987f4376ed825daf034ea4d7c4b4952fe664b25b48f1c09fbcfa6257a40b06c4da7c2caaafa35c346c86bdf298ae21f16c68ea4b1039836990d1a205de2034fd

Author: fanquake

src/key.cpp

@@ -159,21 +159,21 @@ bool CKey::Check(const unsigned char *vch) {
 }
 
 void CKey::MakeNewKey(bool fCompressedIn) {
+    MakeKeyData();
     do {
-        GetStrongRandBytes(keydata);
-    } while (!Check(keydata.data()));
-    fValid = true;
+        GetStrongRandBytes(*keydata);
+    } while (!Check(keydata->data()));
     fCompressed = fCompressedIn;
 }
 
 bool CKey::Negate()
 {
-    assert(fValid);
-    return secp256k1_ec_seckey_negate(secp256k1_context_sign, keydata.data());
+    assert(keydata);
+    return secp256k1_ec_seckey_negate(secp256k1_context_sign, keydata->data());
 }
 
 CPrivKey CKey::GetPrivKey() const {
-    assert(fValid);
+    assert(keydata);
     CPrivKey seckey;
     int ret;
     size_t seckeylen;
@@ -186,7 +186,7 @@ CPrivKey CKey::GetPrivKey() const {
 }
 
 CPubKey CKey::GetPubKey() const {
-    assert(fValid);
+    assert(keydata);
     secp256k1_pubkey pubkey;
     size_t clen = CPubKey::SIZE;
     CPubKey result;
@@ -212,7 +212,7 @@ bool SigHasLowR(const secp256k1_ecdsa_signature* sig)
 }
 
 bool CKey::Sign(const uint256 &hash, std::vector<unsigned char>& vchSig, bool grind, uint32_t test_case) const {
-    if (!fValid)
+    if (!keydata)
         return false;
     vchSig.resize(CPubKey::SIGNATURE_SIZE);
     size_t nSigLen = CPubKey::SIGNATURE_SIZE;
@@ -253,7 +253,7 @@ bool CKey::VerifyPubKey(const CPubKey& pubkey) const {
 }
 
 bool CKey::SignCompact(const uint256 &hash, std::vector<unsigned char>& vchSig) const {
-    if (!fValid)
+    if (!keydata)
         return false;
     vchSig.resize(CPubKey::COMPACT_SIGNATURE_SIZE);
     int rec = -1;
@@ -301,10 +301,12 @@ bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint2
 }
 
 bool CKey::Load(const CPrivKey &seckey, const CPubKey &vchPubKey, bool fSkipCheck=false) {
-    if (!ec_seckey_import_der(secp256k1_context_sign, (unsigned char*)begin(), seckey.data(), seckey.size()))
+    MakeKeyData();
+    if (!ec_seckey_import_der(secp256k1_context_sign, (unsigned char*)begin(), seckey.data(), seckey.size())) {
+        ClearKeyData();
         return false;
+    }
     fCompressed = vchPubKey.IsCompressed();
-    fValid = true;
 
     if (fSkipCheck)
         return true;
@@ -325,22 +327,21 @@ bool CKey::Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const
         BIP32Hash(cc, nChild, 0, begin(), vout.data());
     }
     memcpy(ccChild.begin(), vout.data()+32, 32);
-    memcpy((unsigned char*)keyChild.begin(), begin(), 32);
+    keyChild.Set(begin(), begin() + 32, true);
     bool ret = secp256k1_ec_seckey_tweak_add(secp256k1_context_sign, (unsigned char*)keyChild.begin(), vout.data());
-    keyChild.fCompressed = true;
-    keyChild.fValid = ret;
+    if (!ret) keyChild.ClearKeyData();
     return ret;
 }
 
 EllSwiftPubKey CKey::EllSwiftCreate(Span<const std::byte> ent32) const
 {
-    assert(fValid);
+    assert(keydata);
     assert(ent32.size() == 32);
     std::array<std::byte, EllSwiftPubKey::size()> encoded_pubkey;
 
     auto success = secp256k1_ellswift_create(secp256k1_context_sign,
                                              UCharCast(encoded_pubkey.data()),
-                                             keydata.data(),
+                                             keydata->data(),
                                              UCharCast(ent32.data()));
 
     // Should always succeed for valid keys (asserted above).
@@ -350,7 +351,7 @@ EllSwiftPubKey CKey::EllSwiftCreate(Span<const std::byte> ent32) const
 
 ECDHSecret CKey::ComputeBIP324ECDHSecret(const EllSwiftPubKey& their_ellswift, const EllSwiftPubKey& our_ellswift, bool initiating) const
 {
-    assert(fValid);
+    assert(keydata);
 
     ECDHSecret output;
     // BIP324 uses the initiator as party A, and the responder as party B. Remap the inputs
@@ -359,7 +360,7 @@ ECDHSecret CKey::ComputeBIP324ECDHSecret(const EllSwiftPubKey& their_ellswift, c
                                           UCharCast(output.data()),
                                           UCharCast(initiating ? our_ellswift.data() : their_ellswift.data()),
                                           UCharCast(initiating ? their_ellswift.data() : our_ellswift.data()),
-                                          keydata.data(),
+                                          keydata->data(),
                                           initiating ? 0 : 1,
                                           secp256k1_ellswift_xdh_hash_function_bip324,
                                           nullptr);

src/key.h

@@ -46,57 +46,77 @@ class CKey
         "COMPRESSED_SIZE is larger than SIZE");
 
 private:
-    //! Whether this private key is valid. We check for correctness when modifying the key
-    //! data, so fValid should always correspond to the actual state.
-    bool fValid{false};
+    /** Internal data container for private key material. */
+    using KeyType = std::array<unsigned char, 32>;
 
     //! Whether the public key corresponding to this private key is (to be) compressed.
     bool fCompressed{false};
 
-    //! The actual byte data
-    std::vector<unsigned char, secure_allocator<unsigned char> > keydata;
+    //! The actual byte data. nullptr for invalid keys.
+    secure_unique_ptr<KeyType> keydata;
 
     //! Check whether the 32-byte array pointed to by vch is valid keydata.
     bool static Check(const unsigned char* vch);
 
+    void MakeKeyData()
+    {
+        if (!keydata) keydata = make_secure_unique<KeyType>();
+    }
+
+    void ClearKeyData()
+    {
+        keydata.reset();
+    }
+
 public:
-    //! Construct an invalid private key.
-    CKey()
+    CKey() noexcept = default;
+    CKey(CKey&&) noexcept = default;
+    CKey& operator=(CKey&&) noexcept = default;
+
+    CKey& operator=(const CKey& other)
     {
-        // Important: vch must be 32 bytes in length to not break serialization
-        keydata.resize(32);
+        if (other.keydata) {
+            MakeKeyData();
+            *keydata = *other.keydata;
+        } else {
+            ClearKeyData();
+        }
+        fCompressed = other.fCompressed;
+        return *this;
     }
 
+    CKey(const CKey& other) { *this = other; }
+
     friend bool operator==(const CKey& a, const CKey& b)
     {
         return a.fCompressed == b.fCompressed &&
             a.size() == b.size() &&
-            memcmp(a.keydata.data(), b.keydata.data(), a.size()) == 0;
+            memcmp(a.data(), b.data(), a.size()) == 0;
     }
 
     //! Initialize using begin and end iterators to byte data.
     template <typename T>
     void Set(const T pbegin, const T pend, bool fCompressedIn)
     {
-        if (size_t(pend - pbegin) != keydata.size()) {
-            fValid = false;
+        if (size_t(pend - pbegin) != std::tuple_size_v<KeyType>) {
+            ClearKeyData();
         } else if (Check(&pbegin[0])) {
-            memcpy(keydata.data(), (unsigned char*)&pbegin[0], keydata.size());
-            fValid = true;
+            MakeKeyData();
+            memcpy(keydata->data(), (unsigned char*)&pbegin[0], keydata->size());
             fCompressed = fCompressedIn;
         } else {
-            fValid = false;
+            ClearKeyData();
         }
     }
 
     //! Simple read-only vector-like interface.
-    unsigned int size() const { return (fValid ? keydata.size() : 0); }
-    const std::byte* data() const { return reinterpret_cast<const std::byte*>(keydata.data()); }
-    const unsigned char* begin() const { return keydata.data(); }
-    const unsigned char* end() const { return keydata.data() + size(); }
+    unsigned int size() const { return keydata ? keydata->size() : 0; }
+    const std::byte* data() const { return keydata ? reinterpret_cast<const std::byte*>(keydata->data()) : nullptr; }
+    const unsigned char* begin() const { return keydata ? keydata->data() : nullptr; }
+    const unsigned char* end() const { return begin() + size(); }
 
     //! Check whether this private key is valid.
-    bool IsValid() const { return fValid; }
+    bool IsValid() const { return !!keydata; }
 
     //! Check whether the public key corresponding to this private key is (to be) compressed.
     bool IsCompressed() const { return fCompressed; }

src/support/allocators/secure.h

@@ -57,4 +57,28 @@ struct secure_allocator {
 // TODO: Consider finding a way to make incoming RPC request.params[i] mlock()ed as well
 typedef std::basic_string<char, std::char_traits<char>, secure_allocator<char> > SecureString;
 
+template<typename T>
+struct SecureUniqueDeleter {
+    void operator()(T* t) noexcept {
+        secure_allocator<T>().deallocate(t, 1);
+    }
+};
+
+template<typename T>
+using secure_unique_ptr = std::unique_ptr<T, SecureUniqueDeleter<T>>;
+
+template<typename T, typename... Args>
+secure_unique_ptr<T> make_secure_unique(Args&&... as)
+{
+    T* p = secure_allocator<T>().allocate(1);
+
+    // initialize in place, and return as secure_unique_ptr
+    try {
+        return secure_unique_ptr<T>(new (p) T(std::forward(as)...));
+    } catch (...) {
+        secure_allocator<T>().deallocate(p, 1);
+        throw;
+    }
+}
+
 #endif // BITCOIN_SUPPORT_ALLOCATORS_SECURE_H