diff --git a/components/brave_wallet/browser/internal/hd_key.cc b/components/brave_wallet/browser/internal/hd_key.cc index aea339ad3142..cce44591a57a 100644 --- a/components/brave_wallet/browser/internal/hd_key.cc +++ b/components/brave_wallet/browser/internal/hd_key.cc @@ -134,6 +134,7 @@ std::unique_ptr HDKey::GenerateFromSeed(base::span seed) { } auto hmac = HmacSha512(base::byte_span_from_cstring(kMasterSecret), seed); + auto scoped_zero_span = ScopedSecureZeroSpan(hmac); auto [IL, IR] = base::span(hmac).split_at(kSHA512HashLength / 2); std::unique_ptr hdkey = std::make_unique(); @@ -538,6 +539,7 @@ std::unique_ptr HDKey::DeriveChild(uint32_t index) { data.push_back(index & 0xFF); auto hmac = HmacSha512(chain_code_, data); + auto scoped_zero_span = ScopedSecureZeroSpan(hmac); auto [IL, IR] = base::span(hmac).split_at(kSHA512HashLength / 2); std::unique_ptr hdkey = std::make_unique(); diff --git a/components/brave_wallet/browser/internal/hd_key_ed25519.cc b/components/brave_wallet/browser/internal/hd_key_ed25519.cc index e2558b5e65c6..de8def162c28 100644 --- a/components/brave_wallet/browser/internal/hd_key_ed25519.cc +++ b/components/brave_wallet/browser/internal/hd_key_ed25519.cc @@ -6,7 +6,6 @@ #include "brave/components/brave_wallet/browser/internal/hd_key_ed25519.h" #include -#include #include "base/check.h" #include "base/containers/span.h" @@ -24,17 +23,16 @@ namespace { constexpr char kMasterSecret[] = "ed25519 seed"; -// Validate keypair by signing and verifying signature to ensure included -// private key matches public key. +// Validate keypair(private key matches public key) by using first half as a +// seed. bool ValidateKeypair(base::span key_pair) { - std::array signature = {}; - auto msg = base::byte_span_from_cstring("brave"); + std::array validated_key_pair; - CHECK( - ED25519_sign(signature.data(), msg.data(), msg.size(), key_pair.data())); + std::array public_key; + ED25519_keypair_from_seed(public_key.data(), validated_key_pair.data(), + key_pair.first().data()); - return !!ED25519_verify(msg.data(), msg.size(), signature.data(), - key_pair.last().data()); + return validated_key_pair == key_pair; } } // namespace @@ -47,13 +45,15 @@ HDKeyEd25519::~HDKeyEd25519() = default; HDKeyEd25519::HDKeyEd25519(base::span key, base::span data) { auto hmac = HmacSha512(key, data); + auto scoped_zero_span = ScopedSecureZeroSpan(hmac); auto [il, ir] = base::span(hmac).split_at<32>(); // `public_key` is not used, we use key pair instead. std::array public_key; - ED25519_keypair_from_seed(public_key.data(), key_pair_.data(), il.data()); + ED25519_keypair_from_seed(public_key.data(), key_pair_.AsSpan().data(), + il.data()); - base::span(chain_code_).copy_from(ir); + chain_code_.AsSpan().copy_from(ir); } // static @@ -64,7 +64,7 @@ std::unique_ptr HDKeyEd25519::GenerateFromKeyPair( } auto result = std::make_unique(); - base::span(result->key_pair_).copy_from(key_pair); + result->key_pair_.AsSpan().copy_from(key_pair); return result; } @@ -81,15 +81,16 @@ std::unique_ptr HDKeyEd25519::DeriveHardenedChild( } std::unique_ptr HDKeyEd25519::DeriveChild(uint32_t index) { - std::vector hmac_payload(37); + std::array hmac_payload = {}; auto span_writer = base::SpanWriter(base::span(hmac_payload)); // https://github.com/satoshilabs/slips/blob/master/slip-0010.md#private-parent-key--private-child-key span_writer.Write(base::U8ToBigEndian(0)); span_writer.Write(GetPrivateKeyAsSpan()); span_writer.Write(base::U32ToBigEndian(index)); + DCHECK_EQ(span_writer.remaining(), 0u); - return base::WrapUnique(new HDKeyEd25519(chain_code_, hmac_payload)); + return base::WrapUnique(new HDKeyEd25519(chain_code_.AsSpan(), hmac_payload)); } // static @@ -117,23 +118,25 @@ std::unique_ptr HDKeyEd25519::GenerateFromSeedAndPath( return hd_key; } -std::array HDKeyEd25519::Sign( +std::optional> HDKeyEd25519::Sign( base::span msg) { std::array signature = {}; - CHECK( - ED25519_sign(signature.data(), msg.data(), msg.size(), key_pair_.data())); + if (!ED25519_sign(signature.data(), msg.data(), msg.size(), + key_pair_.AsSpan().data())) { + return std::nullopt; + } return signature; } base::span HDKeyEd25519::GetPrivateKeyAsSpan() const { - return base::span(key_pair_).first(); + return key_pair_.AsSpan().first(); } base::span HDKeyEd25519::GetPublicKeyAsSpan() const { - return base::span(key_pair_).last(); + return key_pair_.AsSpan().last(); } std::string HDKeyEd25519::GetBase58EncodedPublicKey() const { @@ -141,7 +144,7 @@ std::string HDKeyEd25519::GetBase58EncodedPublicKey() const { } std::string HDKeyEd25519::GetBase58EncodedKeypair() const { - return EncodeBase58(key_pair_); + return EncodeBase58(key_pair_.AsSpan()); } } // namespace brave_wallet diff --git a/components/brave_wallet/browser/internal/hd_key_ed25519.h b/components/brave_wallet/browser/internal/hd_key_ed25519.h index ae5992fba707..f6cc6c243ab0 100644 --- a/components/brave_wallet/browser/internal/hd_key_ed25519.h +++ b/components/brave_wallet/browser/internal/hd_key_ed25519.h @@ -7,9 +7,11 @@ #define BRAVE_COMPONENTS_BRAVE_WALLET_BROWSER_INTERNAL_HD_KEY_ED25519_H_ #include +#include #include #include "base/containers/span.h" +#include "brave/components/brave_wallet/browser/internal/hd_key_utils.h" namespace brave_wallet { @@ -39,7 +41,7 @@ class HDKeyEd25519 { std::unique_ptr DeriveHardenedChild(uint32_t index); - std::array Sign( + std::optional> Sign( base::span msg); base::span GetPrivateKeyAsSpan() const @@ -55,8 +57,8 @@ class HDKeyEd25519 { std::unique_ptr DeriveChild(uint32_t index); - std::array key_pair_ = {}; - std::array chain_code_ = {}; + SecureByteArray key_pair_; + SecureByteArray chain_code_; }; } // namespace brave_wallet diff --git a/components/brave_wallet/browser/internal/hd_key_ed25519_unittest.cc b/components/brave_wallet/browser/internal/hd_key_ed25519_unittest.cc index 7d655a26b76a..391d719afd43 100644 --- a/components/brave_wallet/browser/internal/hd_key_ed25519_unittest.cc +++ b/components/brave_wallet/browser/internal/hd_key_ed25519_unittest.cc @@ -238,8 +238,8 @@ TEST(HDKeyEd25519UnitTest, SignAndVerify) { auto key = HDKeyEd25519::GenerateFromSeedAndPath(bytes, "m"); const std::vector msg_a(32, 0x00); const std::vector msg_b(32, 0x08); - const auto sig_a = key->Sign(msg_a); - const auto sig_b = key->Sign(msg_b); + const auto sig_a = *key->Sign(msg_a); + const auto sig_b = *key->Sign(msg_b); EXPECT_TRUE(VerifySignature(*key, msg_a, sig_a)); EXPECT_TRUE(VerifySignature(*key, msg_b, sig_b)); @@ -265,9 +265,16 @@ TEST(HDKeyEd25519UnitTest, GenerateFromPrivateKey) { EXPECT_EQ( "6260C446B2BA429541722F6BAABBEEAF3D1B5981DA326A2DB66804B5EACE770D" "58CFBA0E0D409A3054E80C00505215C7CCD7A040F23364005A47CDE7FCED1400", - base::HexEncode(master_key->Sign(base::byte_span_from_cstring("hello")))); + base::HexEncode( + *master_key->Sign(base::byte_span_from_cstring("hello")))); - key_pair.back() = 0; + // `GenerateFromKeyPair` should fail when private and public key don't match + // each other + std::array another_key_pair; + std::array another_pubkey; + ED25519_keypair(another_pubkey.data(), another_key_pair.data()); + // Replace pubkey part with another valid pubkey. + base::span(key_pair).last<32>().copy_from(another_pubkey); EXPECT_FALSE(HDKeyEd25519::GenerateFromKeyPair(key_pair)); } diff --git a/components/brave_wallet/browser/internal/hd_key_utils.cc b/components/brave_wallet/browser/internal/hd_key_utils.cc index 1de62153bc6b..c8689a75248e 100644 --- a/components/brave_wallet/browser/internal/hd_key_utils.cc +++ b/components/brave_wallet/browser/internal/hd_key_utils.cc @@ -11,9 +11,17 @@ #include "base/numerics/checked_math.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_split.h" +#include "crypto/process_bound_string.h" namespace brave_wallet { +namespace internal { +void SecureZeroBuffer(base::span data) { + crypto::internal::SecureZeroBuffer(data); +} + +} // namespace internal + std::optional> ParseFullHDPath(std::string_view path) { auto entries = base::SplitStringPiece( path, "/", base::WhitespaceHandling::TRIM_WHITESPACE, @@ -47,4 +55,11 @@ std::optional> ParseFullHDPath(std::string_view path) { return result; } +ScopedSecureZeroSpan::ScopedSecureZeroSpan(base::span span) + : span_(span) {} + +ScopedSecureZeroSpan::~ScopedSecureZeroSpan() { + crypto::internal::SecureZeroBuffer(span_); +} + } // namespace brave_wallet diff --git a/components/brave_wallet/browser/internal/hd_key_utils.h b/components/brave_wallet/browser/internal/hd_key_utils.h index 66b86d1b1bd7..5c823cb694fa 100644 --- a/components/brave_wallet/browser/internal/hd_key_utils.h +++ b/components/brave_wallet/browser/internal/hd_key_utils.h @@ -10,8 +10,14 @@ #include #include +#include "base/containers/span.h" + namespace brave_wallet { +namespace internal { +void SecureZeroBuffer(base::span data); +} + inline constexpr char kMasterNode[] = "m"; inline constexpr uint32_t kHardenedOffset = 0x80000000; @@ -21,5 +27,27 @@ inline constexpr uint32_t kHardenedOffset = 0x80000000; // https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki std::optional> ParseFullHDPath(std::string_view path); +class ScopedSecureZeroSpan { + public: + explicit ScopedSecureZeroSpan(base::span span); + ~ScopedSecureZeroSpan(); + + private: + base::span span_; +}; + +template +class SecureByteArray { + public: + SecureByteArray() = default; + ~SecureByteArray() { internal::SecureZeroBuffer(AsSpan()); } + + base::span AsSpan() { return data_; } + base::span AsSpan() const { return data_; } + + private: + std::array data_ = {}; +}; + } // namespace brave_wallet #endif // BRAVE_COMPONENTS_BRAVE_WALLET_BROWSER_INTERNAL_HD_KEY_UTILS_H_ diff --git a/components/brave_wallet/browser/internal/hd_key_utils_unittest.cc b/components/brave_wallet/browser/internal/hd_key_utils_unittest.cc index e12689cbb84d..ed693cea4264 100644 --- a/components/brave_wallet/browser/internal/hd_key_utils_unittest.cc +++ b/components/brave_wallet/browser/internal/hd_key_utils_unittest.cc @@ -47,4 +47,14 @@ TEST(HDKeyUtilsUnitTest, ParseFullHDPath) { EXPECT_FALSE(ParseFullHDPath("m/1'1")); } +TEST(HDKeyUtilsUnitTest, ScopedSecureZeroSpan) { + auto array = std::to_array({1, 2, 3, 4, 5}); + { + ScopedSecureZeroSpan scoped_zero_span(array); + EXPECT_EQ(array, std::to_array({1, 2, 3, 4, 5})); + } + + EXPECT_EQ(array, std::to_array({0, 0, 0, 0, 0})); +} + } // namespace brave_wallet diff --git a/components/brave_wallet/browser/solana_keyring.cc b/components/brave_wallet/browser/solana_keyring.cc index 7f7c2b4ade33..b29c51c6b3c5 100644 --- a/components/brave_wallet/browser/solana_keyring.cc +++ b/components/brave_wallet/browser/solana_keyring.cc @@ -113,7 +113,12 @@ std::vector SolanaKeyring::SignMessage( return std::vector(); } - return base::ToVector(hd_key->Sign(message)); + auto signature = hd_key->Sign(message); + if (!signature) { + return std::vector(); + } + + return base::ToVector(*signature); } std::vector SolanaKeyring::GetHDAccountsForTesting() const { diff --git a/components/brave_wallet/common/hash_utils.cc b/components/brave_wallet/common/hash_utils.cc index c2c7cfaac7e4..1578e1707ea3 100644 --- a/components/brave_wallet/common/hash_utils.cc +++ b/components/brave_wallet/common/hash_utils.cc @@ -20,7 +20,9 @@ #include "third_party/boringssl/src/include/openssl/hmac.h" namespace brave_wallet { + namespace { + std::array ConcatArrays(const std::array& arr1, const std::array& arr2) { std::array result; @@ -28,6 +30,7 @@ std::array ConcatArrays(const std::array& arr1, base::ranges::copy(arr2, result.begin() + 32); return result; } + } // namespace KeccakHashArray KeccakHash(base::span input) {