HD Key cleanup #27118
HD Key cleanup #27118
Conversation
supermassive
force-pushed
the
hd_key_cleanup
branch
from
0082f8a to
992733d
Compare
| .cost = (size_t)*n, | ||
| .block_size = (size_t)*r, | ||
| .parallelization = (size_t)*p, |
There was a problem hiding this comment.
shouldn't we also need checked_cast for these?
There was a problem hiding this comment.
We should't CHECK here as this json is coming from user. There is also DeriveKeyScryptNoCheck which intentionally just fails on invalid args.
There was a problem hiding this comment.
but we checked above for iteration which is also from user input
supermassive
force-pushed
the
hd_key_cleanup
branch
from
a8e93dd to
b5aed5e
Compare
|
[puLL-Merge] - brave/brave-core@27118 DescriptionThis PR makes significant changes to the Brave Wallet's key derivation and management system, focusing on improving the HDKey (Hierarchical Deterministic Key) implementation, adding support for JSON keystore parsing, and refactoring various keyring classes for better organization and efficiency. ChangesChanges
sequenceDiagram
participant User
participant KeyringService
participant JSONKeystoreParser
participant HDKey
participant Keyring
User->>KeyringService: Import account from JSON
KeyringService->>JSONKeystoreParser: Parse and decrypt keystore
JSONKeystoreParser-->>KeyringService: Return private key
KeyringService->>HDKey: Generate from private key
HDKey-->>KeyringService: Return HDKey
KeyringService->>Keyring: Import account
Keyring-->>KeyringService: Return imported account info
KeyringService-->>User: Return result
Possible Issues
Security Hotspots
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| std::vector<uint8_t> chain_code(ptr, ptr + 32); | ||
| ptr += chain_code.size(); | ||
| hdkey->SetChainCode(chain_code); | ||
| reader.ReadU32BigEndian(reinterpret_cast<uint32_t&>(result->version)); |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Using reinterpret_cast against some data types may lead to undefined behaviour. In general, when needing to do these conversions, check how Chromium upstream does them. Most of the times a reinterpret_cast is wrong and there's no guarantee the compiler will generate the code that you thought it would.
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/reinterpret_cast.yaml
Cc @stoletheminerals @thypon @cdesouza-chromium
There was a problem hiding this comment.
This doesn't seem correct. What is the type of result->version?
There was a problem hiding this comment.
enum class ExtendedKeyVersion
Resolves brave/brave-browser#43110
DerivationIndexinstead.ImportAccountFromJsonpayload to separate file.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: