fix review

components/brave_wallet/browser/internal/hd_key.cc

@@ -134,6 +134,7 @@ std::unique_ptr<HDKey> HDKey::GenerateFromSeed(base::span<const uint8_t> 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> hdkey = std::make_unique<HDKey>();
@@ -538,6 +539,7 @@ std::unique_ptr<HDKey> 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> hdkey = std::make_unique<HDKey>();

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 <utility>
-#include <vector>
 
 #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<const uint8_t, kEd25519KeypairSize> key_pair) {
-  std::array<uint8_t, kEd25519SignatureSize> signature = {};
-  auto msg = base::byte_span_from_cstring("brave");
+  std::array<uint8_t, kEd25519KeypairSize> validated_key_pair;
 
-  CHECK(
-      ED25519_sign(signature.data(), msg.data(), msg.size(), key_pair.data()));
+  std::array<uint8_t, ED25519_PUBLIC_KEY_LEN> public_key;
+  ED25519_keypair_from_seed(public_key.data(), validated_key_pair.data(),
+                            key_pair.first<ED25519_PRIVATE_KEY_LEN>().data());
 
-  return !!ED25519_verify(msg.data(), msg.size(), signature.data(),
-                          key_pair.last<kEd25519PublicKeySize>().data());
+  return validated_key_pair == key_pair;
 }
 
 }  // namespace
@@ -47,13 +45,15 @@ HDKeyEd25519::~HDKeyEd25519() = default;
 HDKeyEd25519::HDKeyEd25519(base::span<const uint8_t> key,
                            base::span<const uint8_t> 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<uint8_t, ED25519_PUBLIC_KEY_LEN> 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> HDKeyEd25519::GenerateFromKeyPair(
   }
 
   auto result = std::make_unique<HDKeyEd25519>();
-  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> HDKeyEd25519::DeriveHardenedChild(
 }
 
 std::unique_ptr<HDKeyEd25519> HDKeyEd25519::DeriveChild(uint32_t index) {
-  std::vector<uint8_t> hmac_payload(37);
+  std::array<uint8_t, 37> 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,31 +118,33 @@ std::unique_ptr<HDKeyEd25519> HDKeyEd25519::GenerateFromSeedAndPath(
   return hd_key;
 }
 
-std::array<uint8_t, kEd25519SignatureSize> HDKeyEd25519::Sign(
+std::optional<std::array<uint8_t, kEd25519SignatureSize>> HDKeyEd25519::Sign(
     base::span<const uint8_t> msg) {
   std::array<uint8_t, kEd25519SignatureSize> 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<const uint8_t, kEd25519SecretKeySize>
 HDKeyEd25519::GetPrivateKeyAsSpan() const {
-  return base::span(key_pair_).first<kEd25519SecretKeySize>();
+  return key_pair_.AsSpan().first<kEd25519SecretKeySize>();
 }
 
 base::span<const uint8_t, kEd25519PublicKeySize>
 HDKeyEd25519::GetPublicKeyAsSpan() const {
-  return base::span(key_pair_).last<kEd25519PublicKeySize>();
+  return key_pair_.AsSpan().last<kEd25519PublicKeySize>();
 }
 
 std::string HDKeyEd25519::GetBase58EncodedPublicKey() const {
   return EncodeBase58(GetPublicKeyAsSpan());
 }
 
 std::string HDKeyEd25519::GetBase58EncodedKeypair() const {
-  return EncodeBase58(key_pair_);
+  return EncodeBase58(key_pair_.AsSpan());
 }
 
 }  // namespace brave_wallet

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 <memory>
+#include <optional>
 #include <string>
 
 #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<HDKeyEd25519> DeriveHardenedChild(uint32_t index);
 
-  std::array<uint8_t, kEd25519SignatureSize> Sign(
+  std::optional<std::array<uint8_t, kEd25519SignatureSize>> Sign(
       base::span<const uint8_t> msg);
 
   base::span<const uint8_t, kEd25519SecretKeySize> GetPrivateKeyAsSpan() const
@@ -55,8 +57,8 @@ class HDKeyEd25519 {
 
   std::unique_ptr<HDKeyEd25519> DeriveChild(uint32_t index);
 
-  std::array<uint8_t, kEd25519KeypairSize> key_pair_ = {};
-  std::array<uint8_t, kEd25519ChainCodeSize> chain_code_ = {};
+  SecureByteArray<kEd25519KeypairSize> key_pair_;
+  SecureByteArray<kEd25519ChainCodeSize> chain_code_;
 };
 
 }  // namespace brave_wallet

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<uint8_t> msg_a(32, 0x00);
   const std::vector<uint8_t> 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<uint8_t, 64> another_key_pair;
+  std::array<uint8_t, 32> 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));
 }
 

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<uint8_t> data) {
+  crypto::internal::SecureZeroBuffer(data);
+}
+
+}  // namespace internal
+
 std::optional<std::vector<uint32_t>> ParseFullHDPath(std::string_view path) {
   auto entries = base::SplitStringPiece(
       path, "/", base::WhitespaceHandling::TRIM_WHITESPACE,
@@ -47,4 +55,11 @@ std::optional<std::vector<uint32_t>> ParseFullHDPath(std::string_view path) {
   return result;
 }
 
+ScopedSecureZeroSpan::ScopedSecureZeroSpan(base::span<uint8_t> span)
+    : span_(span) {}
+
+ScopedSecureZeroSpan::~ScopedSecureZeroSpan() {
+  crypto::internal::SecureZeroBuffer(span_);
+}
+
 }  // namespace brave_wallet

components/brave_wallet/browser/internal/hd_key_utils.h

@@ -10,8 +10,14 @@
 #include <string_view>
 #include <vector>
 
+#include "base/containers/span.h"
+
 namespace brave_wallet {
 
+namespace internal {
+void SecureZeroBuffer(base::span<uint8_t> 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<std::vector<uint32_t>> ParseFullHDPath(std::string_view path);
 
+class ScopedSecureZeroSpan {
+ public:
+  explicit ScopedSecureZeroSpan(base::span<uint8_t> span);
+  ~ScopedSecureZeroSpan();
+
+ private:
+  base::span<uint8_t> span_;
+};
+
+template <size_t N>
+class SecureByteArray {
+ public:
+  SecureByteArray() = default;
+  ~SecureByteArray() { internal::SecureZeroBuffer(AsSpan()); }
+
+  base::span<uint8_t, N> AsSpan() { return data_; }
+  base::span<const uint8_t, N> AsSpan() const { return data_; }
+
+ private:
+  std::array<uint8_t, N> data_ = {};
+};
+
 }  // namespace brave_wallet
 #endif  // BRAVE_COMPONENTS_BRAVE_WALLET_BROWSER_INTERNAL_HD_KEY_UTILS_H_

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<uint8_t>({1, 2, 3, 4, 5});
+  {
+    ScopedSecureZeroSpan scoped_zero_span(array);
+    EXPECT_EQ(array, std::to_array<uint8_t>({1, 2, 3, 4, 5}));
+  }
+
+  EXPECT_EQ(array, std::to_array<uint8_t>({0, 0, 0, 0, 0}));
+}
+
 }  // namespace brave_wallet

components/brave_wallet/browser/solana_keyring.cc

@@ -113,7 +113,12 @@ std::vector<uint8_t> SolanaKeyring::SignMessage(
     return std::vector<uint8_t>();
   }
 
-  return base::ToVector(hd_key->Sign(message));
+  auto signature = hd_key->Sign(message);
+  if (!signature) {
+    return std::vector<uint8_t>();
+  }
+
+  return base::ToVector(*signature);
 }
 
 std::vector<std::string> SolanaKeyring::GetHDAccountsForTesting() const {

components/brave_wallet/common/hash_utils.cc

@@ -20,14 +20,17 @@
 #include "third_party/boringssl/src/include/openssl/hmac.h"
 
 namespace brave_wallet {
+
 namespace {
+
 std::array<uint8_t, 64> ConcatArrays(const std::array<uint8_t, 32>& arr1,
                                      const std::array<uint8_t, 32>& arr2) {
   std::array<uint8_t, 64> result;
   base::ranges::copy(arr1, result.begin());
   base::ranges::copy(arr2, result.begin() + 32);
   return result;
 }
+
 }  // namespace
 
 KeccakHashArray KeccakHash(base::span<const uint8_t> input) {