Fix UNSAFE_TODO for wallet [part 2 of N] #26469
Conversation
|
[puLL-Merge] - brave/brave-core@26469 DescriptionThis PR makes several changes to the Brave Wallet codebase, primarily focusing on improvements to hash functions, memory safety, and code readability. The changes span across multiple files and involve modifications to how Keccak hashes are computed and used throughout the wallet functionality. ChangesChanges
Possible Issues
Security HotspotsNo significant security issues are apparent in this PR. The changes mostly involve refactoring existing cryptographic operations rather than introducing new ones. However, thorough testing should be done to ensure that all hash computations still produce the expected results, especially in critical areas like address generation and transaction signing. |
supermassive
changed the title
supermassive
force-pushed
the
wallet_unsafe_todo_2
branch
from
4758598
to
c5c7b59
Compare
| static_assert(sizeof(result) == sizeof(hash.bytes)); | ||
| base::ranges::copy(hash.bytes, result.begin()); | ||
| return result; | ||
| } | ||
|
|
||
| std::vector<uint8_t> KeccakHashToVector(base::span<const uint8_t> input) { |
There was a problem hiding this comment.
I feel it would be better to dispense this function and just do base::ToVector(KeccakHash(...)) in the places we are creating are instantiating a vector from the resulting hash. That's much easier to the reader to understand what's going on.
| std::move(id)); | ||
| SignMessageInternal( | ||
| account_id, std::move(sign_data), | ||
| std::vector<uint8_t>(message_to_sign.begin(), message_to_sign.end()), |
There was a problem hiding this comment.
Unless I'm missing something, i thinkstd::vector<uint8_t>(message_to_sign.begin(), message_to_sign.end()) should be just std::move(message_to_sign).
| auto pubkey_hash = Hash160(public_key_); | ||
| identifier_.assign(pubkey_hash.begin(), pubkey_hash.end()); |
There was a problem hiding this comment.
Before it used to be just:
identifier_ = Hash160(public_key_);But now we are assigning to a local, and then copying it into identifier. Is there a reason to change this from a move operation to a copy?
| auto pubkey_hash = Hash160(public_key_); | ||
| identifier_.assign(pubkey_hash.begin(), pubkey_hash.end()); |
| auto hash = Hash160(hd_key_base->GetPublicKeyBytes()); | ||
| return std::vector<uint8_t>{hash.begin(), hash.end()}; |
| @@ -179,13 +172,11 @@ EthSignTypedDataHelper::EncodeData(const std::string& primary_type_name, | |||
| if (!encoded_field) { | |||
| return std::nullopt; | |||
| } | |||
| result.insert(result.end(), encoded_field->begin(), encoded_field->end()); | |||
| base::Extend(result, *encoded_field); | |||
There was a problem hiding this comment.
I'm a bit confused about this loop. EncodeField returns <EthSignTypedDataHelper::Eip712HashArray, which is a array<uint8_t, 32u>. So it is extending the vector with this value, but the loop doesn't bail out at this point, so potentially there could be another call, that could extend the vector further. Maybe this doesn't happen because of the data being passed in. Am I missing something?
| for (size_t i = 0; i < 32; ++i) { | ||
| result.push_back(0); | ||
| } | ||
| result.insert(result.end(), 32, 0); |
There was a problem hiding this comment.
Is this some type of failure case?
| const std::string& type, | ||
| const base::Value& value) const { | ||
| std::optional<EthSignTypedDataHelper::Eip712HashArray> | ||
| EthSignTypedDataHelper::EncodeField(const std::string& type_string, |
There was a problem hiding this comment.
Not sure if possible, but if possible, change to std::string_view
| const base::Value& value) const { | ||
| std::optional<EthSignTypedDataHelper::Eip712HashArray> | ||
| EthSignTypedDataHelper::EncodeField(const std::string& type_string, | ||
| const base::Value& value) const { |
There was a problem hiding this comment.
I think you could be taking base::Value::List here, and with that the check could be moved outside.
| @@ -221,22 +212,20 @@ std::optional<std::vector<uint8_t>> EthSignTypedDataHelper::EncodeField( | |||
| if (!encoded_item) { | |||
| return std::nullopt; | |||
| } | |||
| array_result.insert(array_result.end(), encoded_item->begin(), | |||
| encoded_item->end()); | |||
| base::Extend(array_result, *encoded_item); | |||
There was a problem hiding this comment.
array_result should be a Eip712HashArray, not a vector.
Resolves brave/brave-browser#42200
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: