Prevent default/invalid CKey objects from allocating secure memory

sipa commented at 4:42 pm on September 18, 2023: member

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.

DrahtBot commented at 4:42 pm on September 18, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	john-moffett, ajtowns

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

in src/key.h:41 in f2156d613e outdated

37@@ -37,6 +38,7 @@ class CKey
38      */
39     static const unsigned int SIZE            = 279;
40     static const unsigned int COMPRESSED_SIZE = 214;
41+    static constexpr unsigned int KEY_SIZE = 32;

ajtowns commented at 1:09 pm on September 21, 2023:

Seems slightly odd to use a vector when we want something that’s always 32 bytes. Could perhaps have secure_unique_ptr<std::array<unsigned char, KEY_SIZE>> keydata instead, essentially replacing .resize(32) with a make_secure_unique() call and replacing ClearShrink with keydata = nullptr.

 0template<typename T>
 1struct SecureUniqueDeleter {
 2    void operator()(T* t) noexcept {
 3        secure_allocator<T>().deallocate(t, 1);
 4    }
 5};
 6
 7template<typename T>
 8using secure_unique_ptr = std::unique_ptr<T,SecureUniqueDeleter<T>>;
 9
10template<typename T, typename... Args>
11secure_unique_ptr<T> make_secure_unique(Args&&... as)
12{
13    T* p = secure_allocator<T>().allocate(1);
14
15    // initialize in place, and return as secure_unique_ptr
16    return secure_unique_ptr<T>(new (p) T(std::forward(as)...));
17}
18
19...
20using KeyType = std::array<unsigned char, 32>;
21secure_unique_ptr<KeyType> keydata{nullptr};

Doing this instead of a vector requires replacing keydata.data() with keydata->data() (etc), and writing an explicit copy constructor for CKey. This would save 16 bytes per CKey (vector presumably stores a 64 bit size/capacity), but it’s not in the secure allocator pool, so we probably don’t care.

sipa commented at 10:55 pm on September 22, 2023:

Done. I’ve added a commit adding secure_unique_ptr, and then switched over to using that as storage type for CKey.

in src/key.h:78 in f2156d613e outdated

72@@ -78,14 +73,17 @@ class CKey
73     template <typename T>
74     void Set(const T pbegin, const T pend, bool fCompressedIn)
75     {
76-        if (size_t(pend - pbegin) != keydata.size()) {
77+        if (size_t(pend - pbegin) != KEY_SIZE) {
78             fValid = false;
79+            ClearShrink(keydata);

ajtowns commented at 1:15 pm on September 21, 2023:

I think you could replace fValid with keydata.size() != 0

sipa commented at 10:55 pm on September 22, 2023:

Done.

in src/key.cpp:331 in f2156d613e outdated

333-    memcpy((unsigned char*)keyChild.begin(), begin(), 32);
334+    memcpy(ccChild.begin(), vout.data() + KEY_SIZE, 32);
335+    keyChild.Set(begin(), begin() + KEY_SIZE, true);
336     bool ret = secp256k1_ec_seckey_tweak_add(secp256k1_context_sign, (unsigned char*)keyChild.begin(), vout.data());
337-    keyChild.fCompressed = true;
338-    keyChild.fValid = ret;

ajtowns commented at 1:27 pm on September 21, 2023:

Not if (!ret) ClearShrink(keyChild.keydata); keyChild.fValid = ret; ? I guess the [[nodiscard]] is good enough though.

sipa commented at 10:59 pm on September 22, 2023:

Done (not with ClearShrink, but by resetting the unique_ptr).

in src/key.cpp:307 in f2156d613e outdated

301@@ -301,8 +302,11 @@ bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint2
302 }
303 
304 bool CKey::Load(const CPrivKey &seckey, const CPubKey &vchPubKey, bool fSkipCheck=false) {
305-    if (!ec_seckey_import_der(secp256k1_context_sign, (unsigned char*)begin(), seckey.data(), seckey.size()))
306+    keydata.resize(KEY_SIZE);
307+    if (!ec_seckey_import_der(secp256k1_context_sign, (unsigned char*)begin(), seckey.data(), seckey.size())) {
308+        ClearShrink(keydata);

ajtowns commented at 1:28 pm on September 21, 2023:

fValid = false; ?

sipa commented at 10:59 pm on September 22, 2023:

No longer needed, because fValid is now implicit as you suggested.

ajtowns commented at 1:34 pm on September 21, 2023: contributor

Approach ACK f2156d613e6e8cc6a6c3d860b74d912515dc782a

sipa force-pushed on Sep 22, 2023

in src/key.h:78 in cd234018e6 outdated

82+    CKey& operator=(const CKey& other)
83     {
84-        // Important: vch must be 32 bytes in length to not break serialization
85-        keydata.resize(32);
86+        MakeKeyData(/*empty=*/!other.keydata);
87+        std::copy(other.keydata->begin(), other.keydata->end(), keydata->begin());

ajtowns commented at 7:54 am on September 23, 2023:

I think this is the only case where you don’t know empty at compile time; could rewrite it as:

0void MakeKeyData() { if (!keydata) keydata = make_secure_unique<KeyType>(); }
1void ClearKeyData() { keydata.reset(); }
2...
3if (other.keydata) {
4    MakeKeyData();
5    *keydata = *other.keydata;
6} else {
7    ClearKeyData();
8}

sipa commented at 11:23 am on September 24, 2023:

Done.

in src/key.h:109 in cd234018e6 outdated

123-    unsigned int size() const { return (fValid ? keydata.size() : 0); }
124-    const std::byte* data() const { return reinterpret_cast<const std::byte*>(keydata.data()); }
125-    const unsigned char* begin() const { return keydata.data(); }
126-    const unsigned char* end() const { return keydata.data() + size(); }
127+    unsigned int size() const { return keydata ? keydata->size() : 0; }
128+    const std::byte* data() const { return reinterpret_cast<const std::byte*>(keydata->data()); }

ajtowns commented at 8:13 am on September 23, 2023:

return keydata ? reinterpret_cast<..>(..) : nullptr; – otherwise CKey() == CKey() will dereference nullptr?

maflcko commented at 10:57 am on September 24, 2023:

For reference, this shouldn’t be an issue with clang/gcc and should fix itself, given enough time, see https://reviews.llvm.org/D86993#4585590

ajtowns commented at 11:14 am on September 24, 2023:

I suppose array::data(/*this=*/keydata) is just returning keydata, so no dereferencing is really taking place, but still…

sipa commented at 11:23 am on September 24, 2023:

Fixed.

maflcko commented at 2:53 pm on September 24, 2023:

I suppose array::data(/*this=*/keydata) is just returning keydata, so no dereferencing is really taking place, but still…

Ah, I misunderstood your initial comment. My reply does not apply here.

sipa force-pushed on Sep 24, 2023

DrahtBot added the label CI failed on Sep 24, 2023

ajtowns commented at 4:31 pm on September 25, 2023: contributor

ACK 94d58fe942fd86fc8d3b11e3f75e4a4f82c86e38 lgtm

fanquake requested review from john-moffett on Sep 26, 2023

DrahtBot removed the label CI failed on Sep 27, 2023

in src/support/allocators/secure.h:76 in 94d58fe942 outdated

71+secure_unique_ptr<T> make_secure_unique(Args&&... as)
72+{
73+    T* p = secure_allocator<T>().allocate(1);
74+
75+    // initialize in place, and return as secure_unique_ptr
76+    return secure_unique_ptr<T>(new (p) T(std::forward(as)...));

john-moffett commented at 2:50 pm on September 27, 2023:

Unlikely and surely a nit, but if T’s construction failed, this would be tiny memory leak along with the thrown exception.

sipa commented at 7:06 pm on September 27, 2023:

Nice catch (pun intended…), fixed!

in src/key.h:115 in 94d58fe942 outdated

129-    const std::byte* data() const { return reinterpret_cast<const std::byte*>(keydata.data()); }
130-    const unsigned char* begin() const { return keydata.data(); }
131-    const unsigned char* end() const { return keydata.data() + size(); }
132+    unsigned int size() const { return keydata ? keydata->size() : 0; }
133+    const std::byte* data() const { return keydata ? reinterpret_cast<const std::byte*>(keydata->data()) : nullptr; }
134+    const unsigned char* begin() const { return keydata->data(); }

john-moffett commented at 3:01 pm on September 27, 2023:

Maybe return keydata ? keydata->data() : nullptr; to avoid potential undefined behavior.

sipa commented at 7:06 pm on September 27, 2023:

Done.

john-moffett commented at 3:34 pm on September 27, 2023: contributor

Approach ACK. One minor suggestion and one nit.

Add make_secure_unique helper

Co-authored-by: Pieter Wuille <bitcoin-dev@wuille.net>

d9841a7ac6

key: don't allocate secure mem for null (invalid) key

Instead of storing the key material as an std::vector (with secure allocator),
use a secure_unique_ptr to a 32-byte array, and use nullptr for invalid keys.
This means a smaller CKey type, and no secure/dynamic memory usage for invalid
keys.

6ef405ddb1

sipa force-pushed on Sep 27, 2023

john-moffett approved

john-moffett commented at 1:31 pm on September 28, 2023: contributor

ACK 6ef405ddb195bbf1b28a906d8c8bb877f0c17d7b

DrahtBot requested review from ajtowns on Sep 28, 2023

ajtowns commented at 2:50 pm on September 28, 2023: contributor

ACK 6ef405ddb195bbf1b28a906d8c8bb877f0c17d7b

DrahtBot removed review request from ajtowns on Sep 28, 2023

fanquake merged this on Oct 2, 2023

fanquake closed this on Oct 2, 2023

Frank-GER referenced this in commit 79881a934c on Oct 5, 2023

bitcoin locked this on Oct 1, 2024

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

Reviews