From 3644ec43ab3621fb1a2b36e2e91c8591c8d3dee8 Mon Sep 17 00:00:00 2001 From: Anton Paymyshev Date: Tue, 14 Jan 2025 18:24:17 +0700 Subject: [PATCH] fix review --- .../src/crypto/curve25519/curve25519.c | 23 +++- .../src/include/openssl/curve25519.h | 4 +- .../browser/internal/hd_key_common.cc | 6 +- .../browser/internal/hd_key_ed25519_slip23.cc | 124 ++++++++---------- .../browser/internal/hd_key_ed25519_slip23.h | 5 +- .../hd_key_ed25519_slip23_unittest.cc | 6 +- 6 files changed, 87 insertions(+), 81 deletions(-) diff --git a/chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c b/chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c index d7f92f11dc64..c5717eb4169b 100644 --- a/chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c +++ b/chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c @@ -11,28 +11,43 @@ #pragma allow_unsafe_buffers #endif +static int IsScalarPruned(const uint8_t scalar[32]) { + return (scalar[0] & 0b00000111) == 0b00000000 && + (scalar[31] & 0b11000000) == 0b01000000; +} + // Produces pubkey form scalar. -// `scalar` must be pruned. https://www.rfc-editor.org/rfc/rfc8032.html#section-5.1.5 +// Function fails if `scalar` is not pruned. https://www.rfc-editor.org/rfc/rfc8032.html#section-5.1.5 // See `ED25519_keypair_from_seed` as origin. -void ED25519_pubkey_from_scalar(uint8_t out_public_key[32], - const uint8_t scalar[32]) { +int ED25519_pubkey_from_scalar(uint8_t out_public_key[32], + const uint8_t scalar[32]) { + if (!IsScalarPruned(scalar)) { + return 0; + } + ge_p3 A; x25519_ge_scalarmult_base(&A, scalar); ge_p3_tobytes(out_public_key, &A); CONSTTIME_DECLASSIFY(out_public_key, 32); + + return 1; } // Same as `ED25519_sign` but without hashing private key. `scalar` and `prefix` // come from ED25519_BIP32 algorithm. -// `scalar` must be pruned. https://www.rfc-editor.org/rfc/rfc8032.html#section-5.1.5 +// Function fails if `scalar` is not pruned. https://www.rfc-editor.org/rfc/rfc8032.html#section-5.1.5 int ED25519_sign_with_scalar_and_prefix(uint8_t out_sig[64], const uint8_t* message, size_t message_len, const uint8_t scalar[32], const uint8_t prefix[32], const uint8_t public_key[32]) { + if (!IsScalarPruned(scalar)) { + return 0; + } + SHA512_CTX hash_ctx; SHA512_Init(&hash_ctx); SHA512_Update(&hash_ctx, prefix, 32); diff --git a/chromium_src/third_party/boringssl/src/include/openssl/curve25519.h b/chromium_src/third_party/boringssl/src/include/openssl/curve25519.h index 45418f9bb88e..417494dbf357 100644 --- a/chromium_src/third_party/boringssl/src/include/openssl/curve25519.h +++ b/chromium_src/third_party/boringssl/src/include/openssl/curve25519.h @@ -9,8 +9,8 @@ extern "C" { #endif -OPENSSL_EXPORT void ED25519_pubkey_from_scalar(uint8_t out_public_key[32], - const uint8_t scalar[32]); +OPENSSL_EXPORT int ED25519_pubkey_from_scalar(uint8_t out_public_key[32], + const uint8_t scalar[32]); OPENSSL_EXPORT int ED25519_sign_with_scalar_and_prefix( uint8_t out_sig[64], const uint8_t* message, diff --git a/components/brave_wallet/browser/internal/hd_key_common.cc b/components/brave_wallet/browser/internal/hd_key_common.cc index ad2cf703079c..5474effa68dd 100644 --- a/components/brave_wallet/browser/internal/hd_key_common.cc +++ b/components/brave_wallet/browser/internal/hd_key_common.cc @@ -25,7 +25,11 @@ DerivationIndex DerivationIndex::Hardened(uint32_t index) { // static DerivationIndex DerivationIndex::FromRawValueForTesting(uint32_t index) { - return DerivationIndex(index % kHardenedOffset, index / kHardenedOffset); + if (index >= kHardenedOffset) { + return DerivationIndex(index - kHardenedOffset, true); + } else { + return DerivationIndex(index, false); + } } bool DerivationIndex::IsValid() const { diff --git a/components/brave_wallet/browser/internal/hd_key_ed25519_slip23.cc b/components/brave_wallet/browser/internal/hd_key_ed25519_slip23.cc index afe131d04ba9..6c3f2f028eed 100644 --- a/components/brave_wallet/browser/internal/hd_key_ed25519_slip23.cc +++ b/components/brave_wallet/browser/internal/hd_key_ed25519_slip23.cc @@ -14,18 +14,13 @@ #include "base/containers/span_writer.h" #include "base/memory/ptr_util.h" #include "base/strings/string_number_conversions.h" -#include "brave/components/brave_wallet/common/hash_utils.h" #include "crypto/hmac.h" #include "third_party/boringssl/src/include/openssl/curve25519.h" #include "third_party/boringssl/src/include/openssl/evp.h" namespace brave_wallet { -namespace { -struct DerivedHmacs { - std::array z_hmac = {}; - std::array cc_hmac = {}; -}; +namespace { // https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-eddsa-05#section-5.1.5 // requires scalar to follow this requirements 'The lowest 3 bits of the first @@ -55,58 +50,16 @@ bool IsValidEd25519Scalar(base::span scalar) { (scalar[31] & 0b1100'0000) == 0b0100'0000; } -std::array PubkeyFromScalar(base::span scalar) { +std::optional> PubkeyFromScalar( + base::span scalar) { DCHECK(IsValidEd25519Scalar(scalar)); std::array public_key; - ED25519_pubkey_from_scalar(public_key.data(), scalar.data()); + if (!ED25519_pubkey_from_scalar(public_key.data(), scalar.data())) { + return std::nullopt; + } return public_key; } -DerivedHmacs DeriveHardenedHmacs( - uint32_t index, - base::span scalar, - base::span prefix, - base::span cc) { - std::array data; - auto span_writer = base::SpanWriter(base::span(data)); - span_writer.Skip(1u); - span_writer.Write(scalar); - span_writer.Write(prefix); - span_writer.WriteU32LittleEndian(index); - - DerivedHmacs result; - - data[0] = 0x00; - result.z_hmac = crypto::hmac::SignSha512(cc, data); - - data[0] = 0x01; - result.cc_hmac = crypto::hmac::SignSha512(cc, data); - - return result; -} - -DerivedHmacs DeriveNormalHmacs( - uint32_t index, - base::span scalar, - base::span public_key, - base::span cc) { - std::array data; - auto span_writer = base::SpanWriter(base::span(data)); - span_writer.Skip(1u); - span_writer.Write(public_key); - span_writer.WriteU32LittleEndian(index); - - DerivedHmacs result; - - data[0] = 0x02; - result.z_hmac = crypto::hmac::SignSha512(cc, data); - - data[0] = 0x03; - result.cc_hmac = crypto::hmac::SignSha512(cc, data); - - return result; -} - std::array CalculateDerivedScalar( base::span parent_scalar, base::span zl) { @@ -158,11 +111,12 @@ HDKeyEd25519Slip23::~HDKeyEd25519Slip23() = default; HDKeyEd25519Slip23::HDKeyEd25519Slip23( base::span scalar, base::span prefix, - base::span chain_code) { + base::span chain_code, + base::span public_key) { base::span(scalar_).copy_from(scalar); base::span(prefix_).copy_from(prefix); base::span(chain_code_).copy_from(chain_code); - public_key_ = PubkeyFromScalar(scalar_); + base::span(public_key_).copy_from(public_key); } std::unique_ptr HDKeyEd25519Slip23::DeriveChild( @@ -172,19 +126,47 @@ std::unique_ptr HDKeyEd25519Slip23::DeriveChild( return nullptr; } - auto derived = - index.is_hardened() - ? DeriveHardenedHmacs(*raw_index_value, scalar_, prefix_, chain_code_) - : DeriveNormalHmacs(*raw_index_value, scalar_, public_key_, - chain_code_); + std::array z_hmac = {}; + std::array cc_hmac = {}; + + if (index.is_hardened()) { + std::array data; + auto span_writer = base::SpanWriter(base::span(data)); + span_writer.Skip(1u); + span_writer.Write(scalar_); + span_writer.Write(prefix_); + span_writer.WriteU32LittleEndian(*raw_index_value); + + data[0] = 0x00; + z_hmac = crypto::hmac::SignSha512(chain_code_, data); + data[0] = 0x01; + cc_hmac = crypto::hmac::SignSha512(chain_code_, data); + } else { + std::array data; + auto span_writer = base::SpanWriter(base::span(data)); + span_writer.Skip(1u); + span_writer.Write(public_key_); + span_writer.WriteU32LittleEndian(*raw_index_value); + + data[0] = 0x02; + z_hmac = crypto::hmac::SignSha512(chain_code_, data); + data[0] = 0x03; + cc_hmac = crypto::hmac::SignSha512(chain_code_, data); + } + + auto derived_scalar = CalculateDerivedScalar( + scalar_, base::span(z_hmac).first()); + + auto pubkey = PubkeyFromScalar(derived_scalar); + if (!pubkey) { + return nullptr; + } return base::WrapUnique(new HDKeyEd25519Slip23( - CalculateDerivedScalar( - scalar_, - base::span(derived.z_hmac).first()), - CalculateDerivedPrefix( - prefix_, base::span(derived.z_hmac).last()), - CalculateDerivedChainCode(derived.cc_hmac))); + derived_scalar, + CalculateDerivedPrefix(prefix_, + base::span(z_hmac).last()), + CalculateDerivedChainCode(cc_hmac), *pubkey)); } // static @@ -204,11 +186,15 @@ HDKeyEd25519Slip23::GenerateMasterKeyFromBip39Entropy( } auto xprv_span = base::span(xprv); + auto scalar = ClampScalarEd25519Bip32(xprv_span.first()); + auto pubkey = PubkeyFromScalar(scalar); + if (!pubkey) { + return nullptr; + } return base::WrapUnique(new HDKeyEd25519Slip23( - ClampScalarEd25519Bip32(xprv_span.first()), - xprv_span.subspan(), - xprv_span.last())); + scalar, xprv_span.subspan(), + xprv_span.last(), *pubkey)); } std::optional> diff --git a/components/brave_wallet/browser/internal/hd_key_ed25519_slip23.h b/components/brave_wallet/browser/internal/hd_key_ed25519_slip23.h index 1983f96ed3f7..fc2c410c5d3c 100644 --- a/components/brave_wallet/browser/internal/hd_key_ed25519_slip23.h +++ b/components/brave_wallet/browser/internal/hd_key_ed25519_slip23.h @@ -7,7 +7,7 @@ #define BRAVE_COMPONENTS_BRAVE_WALLET_BROWSER_INTERNAL_HD_KEY_ED25519_SLIP23_H_ #include -#include +#include #include "base/containers/span.h" #include "base/gtest_prod_util.h" @@ -52,7 +52,8 @@ class HDKeyEd25519Slip23 { HDKeyEd25519Slip23( base::span scalar, base::span prefix, - base::span chain_code); + base::span chain_code, + base::span public_key); static std::unique_ptr FromBip32Entropy( base::span seed, diff --git a/components/brave_wallet/browser/internal/hd_key_ed25519_slip23_unittest.cc b/components/brave_wallet/browser/internal/hd_key_ed25519_slip23_unittest.cc index 77c09a1166be..f19a1699da42 100644 --- a/components/brave_wallet/browser/internal/hd_key_ed25519_slip23_unittest.cc +++ b/components/brave_wallet/browser/internal/hd_key_ed25519_slip23_unittest.cc @@ -228,18 +228,18 @@ TEST(HDKeyEd25519Slip23UnitTest, CardanoSdkCryptoSlip23) { } // Reference implementation encodes pubkey as `pubkey|chain_code`. - EXPECT_EQ(*test_dict.FindString("pubkey"), + ASSERT_EQ(*test_dict.FindString("pubkey"), HexEncodeLower(key->GetPublicKeyAsSpan()) + HexEncodeLower(key->GetChainCodeAsSpanForTesting())); // Reference implementation encodes private key as // `scalar|prefix|chain_code`. - EXPECT_EQ(*test_dict.FindString("privatekey"), + ASSERT_EQ(*test_dict.FindString("privatekey"), HexEncodeLower(key->GetScalarAsSpanForTesting()) + HexEncodeLower(key->GetPrefixAsSpanForTesting()) + HexEncodeLower(key->GetChainCodeAsSpanForTesting())); - EXPECT_EQ( + ASSERT_EQ( *test_dict.FindString("signature"), HexEncodeLower(*key->Sign(base::byte_span_from_cstring("message")))); }