fix: cleanse BIP32 secret derivation state

Author: PastaPastaPasta

include/dashbls/chaincode.hpp

@@ -38,6 +38,8 @@ class ChainCode {
     static ChainCode FromBytes(const Bytes& bytes);
 
     ChainCode(const ChainCode &cc);
+    ChainCode& operator=(const ChainCode& cc);
+    ~ChainCode();
 
     // Comparator implementation.
     friend bool operator==(ChainCode const &a,  ChainCode const &b);
@@ -48,12 +50,11 @@ class ChainCode {
     std::vector<uint8_t> Serialize() const;
 
     // Prevent direct construction, use static constructor
-    ChainCode() {}
+    ChainCode();
 private:
 
     bn_t chainCode;
 };
 } // end namespace bls
 
 #endif  // SRC_CHAINCODE_HPP_
-

include/dashbls/util.hpp

@@ -95,6 +95,17 @@ class Util {
         secureFreeCallback(ptr);
     }
 
+    /*
+     * Overwrite sensitive memory through a volatile pointer so the writes are
+     * not removed as dead stores by the compiler.
+     */
+    static void SecureWipe(void* ptr, size_t size) noexcept {
+        volatile uint8_t* bytes = static_cast<volatile uint8_t*>(ptr);
+        while (size-- > 0) {
+            *bytes++ = 0;
+        }
+    }
+
     /*
      * Converts one hex character to an int.
      */

src/chaincode.cpp

@@ -20,17 +20,44 @@ ChainCode ChainCode::FromBytes(const Bytes& bytes) {
     if (bytes.size() != ChainCode::SIZE) {
         throw std::invalid_argument("ChainCode::FromBytes: Invalid size");
     }
-    ChainCode c = ChainCode();
-    bn_new(c.chainCode);
+    ChainCode c;
     bn_read_bin(c.chainCode, bytes.begin(), ChainCode::SIZE);
     return c;
 }
 
-ChainCode::ChainCode(const ChainCode &cc) {
-    uint8_t bytes[ChainCode::SIZE];
-    cc.Serialize(bytes);
+ChainCode::ChainCode() {
+    bn_null(chainCode);
     bn_new(chainCode);
-    bn_read_bin(chainCode, bytes, ChainCode::SIZE);
+    bn_zero(chainCode);
+}
+
+ChainCode::ChainCode(const ChainCode &cc) : ChainCode() {
+    bn_copy(chainCode, cc.chainCode);
+}
+
+ChainCode& ChainCode::operator=(const ChainCode& cc) {
+    if (this != &cc) {
+#if ALLOC == DYNAMIC
+        if (chainCode->dp != nullptr && chainCode->alloc > 0) {
+            Util::SecureWipe(chainCode->dp, chainCode->alloc * sizeof(dig_t));
+        }
+#elif ALLOC == AUTO
+        Util::SecureWipe(chainCode->dp, sizeof(chainCode->dp));
+#endif
+        bn_copy(chainCode, cc.chainCode);
+    }
+    return *this;
+}
+
+ChainCode::~ChainCode() {
+#if ALLOC == DYNAMIC
+    if (chainCode != nullptr && chainCode->dp != nullptr && chainCode->alloc > 0) {
+        Util::SecureWipe(chainCode->dp, chainCode->alloc * sizeof(dig_t));
+    }
+    bn_free(chainCode);
+#elif ALLOC == AUTO
+    Util::SecureWipe(chainCode, sizeof(chainCode));
+#endif
 }
 
 // Comparator implementation.

src/extendedprivatekey.cpp

@@ -13,47 +13,95 @@
 // limitations under the License.
 
 #include <cstring>
+#include <new>
 #include "bls.hpp"
 
 namespace bls {
+namespace {
+
+class SecureBytes {
+ public:
+    explicit SecureBytes(size_t size) : m_size(size), m_data(Util::SecAlloc<uint8_t>(size)) {
+        if (m_data == nullptr) {
+            throw std::bad_alloc();
+        }
+    }
+
+    ~SecureBytes() {
+        Util::SecureWipe(m_data, m_size);
+        Util::SecFree(m_data);
+    }
+
+    SecureBytes(const SecureBytes&) = delete;
+    SecureBytes& operator=(const SecureBytes&) = delete;
+
+    uint8_t* data() { return m_data; }
+
+ private:
+    const size_t m_size;
+    uint8_t* const m_data;
+};
+
+class SecureRelicBn {
+ public:
+    SecureRelicBn() {
+        bn_null(m_value);
+        bn_new(m_value);
+    }
+
+    ~SecureRelicBn() {
+#if ALLOC == DYNAMIC
+        if (m_value != nullptr && m_value->dp != nullptr && m_value->alloc > 0) {
+            Util::SecureWipe(m_value->dp, m_value->alloc * sizeof(dig_t));
+        }
+        bn_free(m_value);
+#elif ALLOC == AUTO
+        Util::SecureWipe(m_value, sizeof(m_value));
+#endif
+    }
+
+    SecureRelicBn(const SecureRelicBn&) = delete;
+    SecureRelicBn& operator=(const SecureRelicBn&) = delete;
+
+    bn_st* get() { return m_value; }
+
+ private:
+    bn_t m_value;
+};
+
+} // namespace
 
 ExtendedPrivateKey ExtendedPrivateKey::FromSeed(const Bytes& bytes) {
     // "BLS HD seed" in ascii
     const uint8_t prefix[] = {66, 76, 83, 32, 72, 68, 32, 115, 101, 101, 100};
 
-    uint8_t* hashInput = Util::SecAlloc<uint8_t>(bytes.size() + 1);
-    std::memcpy(hashInput, bytes.begin(), bytes.size());
+    SecureBytes hashInput(bytes.size() + 1);
+    std::memcpy(hashInput.data(), bytes.begin(), bytes.size());
 
     // 32 bytes for secret key, and 32 bytes for chaincode
-    uint8_t* ILeft = Util::SecAlloc<uint8_t>(
-            PrivateKey::PRIVATE_KEY_SIZE);
-    uint8_t IRight[ChainCode::SIZE];
+    SecureBytes ILeft(PrivateKey::PRIVATE_KEY_SIZE);
+    SecureBytes IRight(ChainCode::SIZE);
 
     // Hash the seed into 64 bytes, half will be sk, half will be cc
-    hashInput[bytes.size()] = 0;
-    md_hmac(ILeft, hashInput, bytes.size() + 1, prefix, sizeof(prefix));
+    hashInput.data()[bytes.size()] = 0;
+    md_hmac(ILeft.data(), hashInput.data(), bytes.size() + 1, prefix, sizeof(prefix));
 
-    hashInput[bytes.size()] = 1;
-    md_hmac(IRight, hashInput, bytes.size() + 1, prefix, sizeof(prefix));
+    hashInput.data()[bytes.size()] = 1;
+    md_hmac(IRight.data(), hashInput.data(), bytes.size() + 1, prefix, sizeof(prefix));
 
     // Make sure private key is less than the curve order
-    bn_t* skBn = Util::SecAlloc<bn_t>(1);
-    bn_t order;
-    bn_new(order);
-    g1_get_ord(order);
+    SecureRelicBn skBn;
+    SecureRelicBn order;
+    g1_get_ord(order.get());
 
-    bn_new(*skBn);
-    bn_read_bin(*skBn, ILeft, PrivateKey::PRIVATE_KEY_SIZE);
-    bn_mod_basic(*skBn, *skBn, order);
-    bn_write_bin(ILeft, PrivateKey::PRIVATE_KEY_SIZE, *skBn);
+    bn_read_bin(skBn.get(), ILeft.data(), PrivateKey::PRIVATE_KEY_SIZE);
+    bn_mod_basic(skBn.get(), skBn.get(), order.get());
+    bn_write_bin(ILeft.data(), PrivateKey::PRIVATE_KEY_SIZE, skBn.get());
 
     ExtendedPrivateKey esk(ExtendedPublicKey::REVISION, 0, 0, 0,
-                           ChainCode::FromBytes(Bytes(IRight, ChainCode::SIZE)),
-                           PrivateKey::FromBytes(Bytes(ILeft, PrivateKey::PRIVATE_KEY_SIZE)));
+                           ChainCode::FromBytes(Bytes(IRight.data(), ChainCode::SIZE)),
+                           PrivateKey::FromBytes(Bytes(ILeft.data(), PrivateKey::PRIVATE_KEY_SIZE)));
 
-    Util::SecFree(skBn);
-    Util::SecFree(ILeft);
-    Util::SecFree(hashInput);
     return esk;
 }
 
@@ -79,48 +127,45 @@ ExtendedPrivateKey ExtendedPrivateKey::PrivateChild(uint32_t i, const bool fLega
     uint32_t cmp = (1 << 31);
     bool hardened = i >= cmp;
 
-    uint8_t* ILeft = Util::SecAlloc<uint8_t>(PrivateKey::PRIVATE_KEY_SIZE);
-    uint8_t IRight[ChainCode::SIZE];
+    SecureBytes ILeft(PrivateKey::PRIVATE_KEY_SIZE);
+    SecureBytes IRight(ChainCode::SIZE);
 
     // Chain code is used as hmac key
-    uint8_t hmacKey[ChainCode::SIZE];
-    chainCode.Serialize(hmacKey);
+    SecureBytes hmacKey(ChainCode::SIZE);
+    chainCode.Serialize(hmacKey.data());
 
     size_t inputLen = hardened ? PrivateKey::PRIVATE_KEY_SIZE + 4 + 1
                                 : G1Element::SIZE + 4 + 1;
     // Hmac input includes sk or pk, int i, and byte with 0 or 1
-    uint8_t* hmacInput = Util::SecAlloc<uint8_t>(inputLen);
+    SecureBytes hmacInput(inputLen);
 
     // Fill the input with the required data
     if (hardened) {
-        sk.Serialize(hmacInput);
-        Util::IntToFourBytes(hmacInput + PrivateKey::PRIVATE_KEY_SIZE, i);
+        sk.Serialize(hmacInput.data());
+        Util::IntToFourBytes(hmacInput.data() + PrivateKey::PRIVATE_KEY_SIZE, i);
     } else {
-        memcpy(hmacInput, sk.GetG1Element().Serialize(fLegacy).data(), G1Element::SIZE);
-        Util::IntToFourBytes(hmacInput + G1Element::SIZE, i);
+        memcpy(hmacInput.data(), sk.GetG1Element().Serialize(fLegacy).data(), G1Element::SIZE);
+        Util::IntToFourBytes(hmacInput.data() + G1Element::SIZE, i);
     }
-    hmacInput[inputLen - 1] = 0;
+    hmacInput.data()[inputLen - 1] = 0;
 
-    md_hmac(ILeft, hmacInput, inputLen,
-                    hmacKey, ChainCode::SIZE);
+    md_hmac(ILeft.data(), hmacInput.data(), inputLen,
+                    hmacKey.data(), ChainCode::SIZE);
 
     // Change 1 byte to generate a different sequence for chaincode
-    hmacInput[inputLen - 1] = 1;
+    hmacInput.data()[inputLen - 1] = 1;
 
-    md_hmac(IRight, hmacInput, inputLen,
-                    hmacKey, ChainCode::SIZE);
+    md_hmac(IRight.data(), hmacInput.data(), inputLen,
+                    hmacKey.data(), ChainCode::SIZE);
 
-    PrivateKey newSk = PrivateKey::FromBytes(Bytes(ILeft, PrivateKey::PRIVATE_KEY_SIZE), true);
+    PrivateKey newSk = PrivateKey::FromBytes(Bytes(ILeft.data(), PrivateKey::PRIVATE_KEY_SIZE), true);
     newSk = PrivateKey::Aggregate({sk, newSk});
 
     ExtendedPrivateKey esk(version, depth + 1,
                            sk.GetG1Element().GetFingerprint(), i,
-                           ChainCode::FromBytes(Bytes(IRight, ChainCode::SIZE)),
+                           ChainCode::FromBytes(Bytes(IRight.data(), ChainCode::SIZE)),
                            newSk);
 
-    Util::SecFree(ILeft);
-    Util::SecFree(hmacInput);
-
     return esk;
 }
 

src/privatekey.cpp

@@ -16,6 +16,20 @@
 #include "legacy.hpp"
 
 namespace bls {
+namespace {
+
+void SecureWipePrivateKey(bn_st* keydata) noexcept
+{
+#if ALLOC == DYNAMIC
+    if (keydata->dp != nullptr && keydata->alloc > 0) {
+        Util::SecureWipe(keydata->dp, keydata->alloc * sizeof(dig_t));
+    }
+#elif ALLOC == AUTO
+    Util::SecureWipe(keydata->dp, sizeof(keydata->dp));
+#endif
+}
+
+} // namespace
 
 const size_t PrivateKey::PRIVATE_KEY_SIZE;
 
@@ -119,6 +133,7 @@ PrivateKey::~PrivateKey()
 void PrivateKey::DeallocateKeyData()
 {
     if(keydata != nullptr) {
+        SecureWipePrivateKey(keydata);
         Util::SecFree(keydata);
         keydata = nullptr;
     }
@@ -133,9 +148,13 @@ void PrivateKey::InvalidateCaches()
 
 PrivateKey& PrivateKey::operator=(const PrivateKey& other)
 {
+    if (this == &other) {
+        return *this;
+    }
     CheckKeyData();
     other.CheckKeyData();
     InvalidateCaches();
+    SecureWipePrivateKey(keydata);
     bn_copy(keydata, other.keydata);
     return *this;
 }

src/test.cpp

@@ -14,6 +14,7 @@
 // limitations under the License.
 
 #define CATCH_CONFIG_RUNNER
+#include <new>
 #include <thread>
 
 #include "bls.hpp"
@@ -73,6 +74,8 @@ TEST_CASE("class PrivateKey") {
         REQUIRE(pk1.GetG1Element() == pk2.GetG1Element());
         REQUIRE(pk1.GetG2Element() == pk2.GetG2Element());
         REQUIRE(pk3 != pk2);
+        pk2 = pk2;
+        REQUIRE(pk1 == pk2);
     }
     SECTION("Move {constructor|assignment operator}") {
         PrivateKey pk1 = PrivateKey::RandomPrivateKey();
@@ -1267,6 +1270,32 @@ TEST_CASE("Schemes") {
 }
 
 TEST_CASE("Legacy HD keys") {
+    SECTION("Chain code copy construction and assignment preserve value") {
+        std::array<uint8_t, ChainCode::SIZE> first{};
+        std::array<uint8_t, ChainCode::SIZE> second{};
+        first.front() = 1;
+        second.back() = 2;
+
+        const ChainCode source = ChainCode::FromBytes(Bytes(first));
+        ChainCode copy{source};
+        ChainCode assigned = ChainCode::FromBytes(Bytes(second));
+
+        assigned = source;
+        REQUIRE(copy == source);
+        REQUIRE(assigned == source);
+
+        assigned = assigned;
+        REQUIRE(assigned == source);
+
+#if ALLOC == AUTO
+        alignas(ChainCode) std::array<uint8_t, sizeof(ChainCode)> storage;
+        storage.fill(0xa5);
+        auto* stored = new (storage.data()) ChainCode{source};
+        stored->~ChainCode();
+        REQUIRE(std::all_of(storage.begin(), storage.end(), [](uint8_t byte) { return byte == 0; }));
+#endif
+    }
+
     SECTION("Should create an extended private key from seed") {
         std::vector<uint8_t> seed{1, 50, 6, 244, 24, 199, 1, 25};
         ExtendedPrivateKey esk = ExtendedPrivateKey::FromSeed(Bytes(seed));