-
Notifications
You must be signed in to change notification settings - Fork 893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wallet openssl ed25519 #26770
base: master
Are you sure you want to change the base?
Wallet openssl ed25519 #26770
Conversation
963dcb0
to
8f325e8
Compare
e7e8470
to
1cd3484
Compare
efc3048
to
0d76610
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments for you as I'm starting to review.
From a quick look at BoringSSL I can't see any reason that there would be a difference between it and dalek such that key recovery attacks would occur, but I want to dig in a bit more and will do that as you get a chance to go through these changes.
third_party/rust/chromium_crates_io/vendor/ed25519-dalek-2.1.1/Cargo.toml
Outdated
Show resolved
Hide resolved
components/brave_wallet/browser/internal/hd_key_ed25519_unittest.cc
Outdated
Show resolved
Hide resolved
return nullptr; | ||
} | ||
DCHECK(out_len == kSHA512Length); | ||
auto hmac = HmacSha512(base::byte_span_from_cstring(kMasterSecret), seed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be typed as a SecureVector still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why it was a secure vector before. seed
and mnemonic
are not 'secure' in this way but have same or higher level of importance.
It makes sense to clear private keys from memory when wallet is locked after browser being idle for some time. But I don't see a consistent approach when/what we should clear at active runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is when it was introduced.
It seems like we may have just missed some of these as refactors occurred since the change. Let's keep this as a SecureVector in here and then can do a follow-up issue to be more consistent about using SecureVector once ZCash work is finished up. In general, my preference is that if it's a secret we don't want left in memory after the wallet is locked then we should set it as a SecureVector. The one area where this might be a bit tricky is going to be zcash with their various key structures hence wanting to wait until that's finished.
std::unique_ptr<HDKeyEd25519> HDKeyEd25519::GenerateFromSeedAndPath( | ||
base::span<const uint8_t> seed, | ||
std::string_view hd_path) { | ||
auto hd_key = base::WrapUnique( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a SecureVector type too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it's fields to be SecureByteArray
if (path_ != kMasterNode) { | ||
VLOG(0) << "must derive only from master key"; | ||
// static | ||
std::unique_ptr<HDKeyEd25519> HDKeyEd25519::GenerateFromSeedAndPath( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo for myself: check to make sure that BoringSSL behaves as expected with this compared to Dalek-Ed25519 to avoid key recovery attacks
things still blocking this are:
|
0198666
to
dbea600
Compare
I’m planning to review this change in the coming days, however I would like to ask beforehand for this PR to be broken apart into digestible smaller PRs. The GitHub UI is not the most friendly for code review of changes this size. If the high file count is due to deps being added/removed, this could be submitted separately. |
base::ToLowerASCII(base::HexEncode(master_key->GetPublicKeyBytes())), | ||
"a4b2856bfec510abab89753fac1ac0e1112364e7d250545963f135f2a33188ed"); | ||
auto master_key = HDKeyEd25519::GenerateFromSeedAndPath(bytes, "m"); | ||
EXPECT_EQ(HexEncodeLower(master_key->GetPrivateKeyAsSpan()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better if you didn't make this change to use HexEncodeLower
in this PR. It makes it harder to see what is still the still the same vs changed vs added. Not critical, but it would be an easier read and you could follow-up to change the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rolled back
// derivations in terms of DeriveHardened/DeriveNormal calls. | ||
std::optional<std::vector<uint32_t>> ParseFullHDPath(std::string_view path); | ||
|
||
class ScopedSecureZeroSpan { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the filename should be scoped_secure_zero_span to match the class name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there is also ParseFullHDPath
above so this should go in a separate file. We shouldn't use *utils.h
as a kind of dumping ground for random related class and methods. Classes should generally go in their own files and it's better to make the "utils" more specific to the actual function vs generic "hd_key"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually this doesn't seem related to the change in implementation and has some other issues related to base::span storage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed to be redone in a follow up
namespace brave_wallet { | ||
|
||
namespace internal { | ||
void SecureZeroBuffer(base::span<uint8_t> data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this would make more sense as a static private method on ScopedSecureZeroSpan
and use friend for SecureByteArray
and tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually this method is just calling crypto::internal::SecureZeroBuffer
so we don't need it at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was to avoid exposing crypto/process_bound_string.h
through header.
Now removed
// TODO(apaymyshev): We most likely do not need this parsing as at runtime all | ||
// parsing is done from hardcoded strings. Worth implementing account roots | ||
// derivations in terms of DeriveHardened/DeriveNormal calls. | ||
std::optional<std::vector<uint32_t>> ParseFullHDPath(std::string_view path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParseBIP32FullDerivationPath
? https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this also possibly make more sense as part of some existing class? If not that's ok, but maybe this should be bip32_utils
or something after moving ScopedSecureZeroSpan
to its own class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@@ -17,9 +17,11 @@ namespace brave_wallet { | |||
|
|||
inline constexpr size_t kKeccakHashLength = 32; | |||
inline constexpr size_t kRipemd160HashLength = 20; | |||
inline constexpr size_t kSHA512HashLength = 64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://commondatastorage.googleapis.com/chromium-boringssl-docs/sha.h.html#SHA512_DIGEST_LENGTH
sha.h
has this constant SHA512_DIGEST_LENGTH
defined as 64
.
We shouldn't hardcoded these ourselves.
Likewise RIPEMD160_DIGEST_LENGTH
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to leak OpenSSL headers through this header.
Chrome is OK to hardcode this
https://source.chromium.org/chromium/chromium/src/+/main:crypto/sha2.h;l=26?q=kSHA256Length&ss=chromium
https://source.chromium.org/chromium/chromium/src/+/main:crypto/hash.h;l=15-17?q=kSha512Size&ss=chromium
|
||
// Invalid because program derived addresses have to be off-curve. | ||
if (bytes_are_curve25519_point( | ||
rust::Slice<const uint8_t>{hash_vec.data(), hash_vec.size()})) { | ||
rust::Slice<const uint8_t>{hash_array.data(), hash_array.size()})) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base::SpanToRustSlice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
base::span<const uint8_t> data) { | ||
SHA512HashArray hmac; | ||
unsigned int out_len = 0; | ||
CHECK(HMAC(EVP_sha512(), key.data(), key.size(), data.data(), data.size(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we want to crash the browser if this fails, can it fail for any reason other than running out of memory? Actually no memory is heap allocated in this call so it should definitely not CHECK
. See https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/checks.md#failures-beyond-chromium_s-control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's similarly CHECK'ed in many places like this
https://source.chromium.org/chromium/chromium/src/+/main:remoting/protocol/webrtc_transport.cc;l=829-831;drc=b31f44704280a3ec2acb6d7c0542cbe0ce47e53f;bpv=1;bpt=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevertheless now using crypto::hmac::SignSha512
novelty
unsigned int out_len = 0; | ||
CHECK(HMAC(EVP_sha512(), key.data(), key.size(), data.data(), data.size(), | ||
hmac.data(), &out_len)); | ||
CHECK_EQ(out_len, hmac.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, this is not an appropriate use of CHECK
. Any problem with the generation of the hmac should be handled by the caller, not by crashing. Maybe this should return std::optional<SHA512HashArray>
?
|
||
namespace brave_wallet { | ||
|
||
// This class implement basic EdDSA over ed25519 functionality of bip32-ed25519 | ||
inline constexpr size_t kEd25519PrivateKeySize = 32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ED25519_PRIVATE_KEY_LEN
, ED25519_PUBLIC_KEY_LEN
also it looks like kEd25519PrivateKeySize
is incorrect and should be 64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
32 bytes is private key size per spec.
64 bytes is what OpenSSL names as private_key which is actually a pair of private and public keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had left a comment elsewhere about this, but I guess both me and Brian were confused, so I guess maybe we need some better naming, the comment above could also better explain this distinction, besides the link to the rfc.
inline constexpr size_t kEd25519KeypairSize = | ||
kEd25519PrivateKeySize + kEd25519PublicKeySize; | ||
inline constexpr size_t kEd25519ChainCodeSize = 32; | ||
inline constexpr size_t kEd25519SignatureSize = 64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ED25519_SIGNATURE_LEN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed by now
inline constexpr size_t kEd25519PublicKeySize = 32; | ||
inline constexpr size_t kEd25519KeypairSize = | ||
kEd25519PrivateKeySize + kEd25519PublicKeySize; | ||
inline constexpr size_t kEd25519ChainCodeSize = 32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please provide a link or other reference for this value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
result.reserve(entries.size() - 1); | ||
for (auto node : base::span(entries).subspan(1)) { | ||
uint32_t offset = 0; | ||
if (node.ends_with('\'')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add comments for the expected format(s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed completely
// expected to end with a single quote per BIP-44 style. | ||
// https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki | ||
// https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki | ||
// TODO(apaymyshev): We most likely do not need this parsing as at runtime all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please create (and link) issues for TODOs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
base::span<const uint8_t> seed); | ||
static std::unique_ptr<HDKeyEd25519> GenerateFromPrivateKey( | ||
base::span<const uint8_t> private_key); | ||
static std::unique_ptr<HDKeyEd25519> GenerateFromSeedAndPath( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any changes to the code should be a follow-up. We don't want to mix up replacing the ed25519 implementation with other changes to the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed unrelated changes
std::vector<uint8_t> HDKeyEd25519::GetPrivateKeyBytes() const { | ||
auto secret_key = private_key_->unwrap().secret_key_raw(); | ||
return {secret_key.begin(), secret_key.end()}; | ||
base::span<const uint8_t, kEd25519PrivateKeySize> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these changes to the method signature necessary for something in the new implementation? Please don't change anything that doesn't strictly need to be changed. Any other changes should be follow-ups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
@supermassive @kdenhartog Let's have all changes to anything relating to |
dbea600
to
2d23f5a
Compare
@@ -18,7 +34,6 @@ TEST(HDKeyEd25519UnitTest, TestVector1) { | |||
|
|||
// m | |||
auto master_key_base = HDKeyEd25519::GenerateFromSeed(bytes); | |||
EXPECT_EQ(master_key_base->GetPath(), "m"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need path
property at all
2d23f5a
to
54d34ad
Compare
@kdenhartog @Brandon-T @bridiver @cdesouza-chromium |
std::array<uint8_t, ED25519_PRIVATE_KEY_LEN> key_pair = {}; | ||
base::span(key_pair).first<kEd25519PrivateKeySize>().copy_from(private_key); | ||
base::span(key_pair).last<kEd25519PublicKeySize>().copy_from(public_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have ED25519_PRIVATE_KEY_LEN
, but we also have kEd25519PrivateKeySize
. The distinction between these two doesn't seem very clear to me, as they have identical names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed underlying storage to key pair, added some static_asserts and comments. Hope it's more clear now.
std::vector<uint8_t> HDKeyEd25519::GetPublicKeyBytes() const { | ||
auto public_key = private_key_->unwrap().public_key_raw(); | ||
return {public_key.begin(), public_key.end()}; | ||
return base::ToVector(public_key_); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do this in a follow up if you prefer, but every time someone calls GetPublicKeyBytes
, it results on a copy, as we have to wrap the value on a vector
. This doesn't seem to be necessary and it looks like we could just have:
const std::array<uint8_t, kEd25519PrivateKeySize> private_key() const {
return private_key_;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That in the first version of PR(actually returning span on const), but we later decided not change signature of public methods at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind elaborating why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do this in a follow up if you prefer
Please change this to be a reference to the member in a follow up.
54d34ad
to
c622ddb
Compare
[puLL-Merge] - brave/brave-core@26770 DescriptionThis PR updates the implementation of Ed25519 key derivation in the Brave Wallet to use a new Rust-based library (ed25519-dalek-bip32) instead of the previous C++ implementation. This change aims to improve security and maintainability by leveraging a well-tested Rust library for cryptographic operations. ChangesChanges
sequenceDiagram
participant App as Brave Wallet App
participant HDKeyEd25519 as HDKeyEd25519 (C++)
participant RustLib as Rust Library
participant Ed25519DalekBip32 as ed25519-dalek-bip32
App->>HDKeyEd25519: Generate from seed
HDKeyEd25519->>RustLib: generate_ed25519_extended_secret_key_from_seed
RustLib->>Ed25519DalekBip32: ExtendedSigningKey::from_seed
Ed25519DalekBip32-->>RustLib: ExtendedSigningKey
RustLib-->>HDKeyEd25519: Ed25519DalekExtendedSecretKey
HDKeyEd25519-->>App: HDKeyEd25519 instance
App->>HDKeyEd25519: Derive child key
HDKeyEd25519->>RustLib: derive or derive_hardened_child
RustLib->>Ed25519DalekBip32: derive or derive_child
Ed25519DalekBip32-->>RustLib: Derived ExtendedSigningKey
RustLib-->>HDKeyEd25519: Derived Ed25519DalekExtendedSecretKey
HDKeyEd25519-->>App: Derived HDKeyEd25519 instance
App->>HDKeyEd25519: Sign message
HDKeyEd25519->>RustLib: sign
RustLib->>Ed25519DalekBip32: sign
Ed25519DalekBip32-->>RustLib: Signature
RustLib-->>HDKeyEd25519: Signature bytes
HDKeyEd25519-->>App: Signature
Possible Issues
Security Hotspots
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Resolves brave/brave-browser#42579
SecReview: https://github.com/brave/reviews/issues/1808
ed25519-dalek-bip32
and some related crates.ED25519_*
functions from OpenSSL as a replacement.Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: