wallet: Add size check on meta.key_origin.path #23277

1. 
   rooprob commented at 4:38 am on October 14, 2021: contributor

   Resolves #21605 segfault on legacy wallet

   I also encountered same [*] when resyncing a 2017 era blockchain.

   • https://gist.github.com/rooprob/91d78358b920b8f8b7df78af8b2ce99c
2. fanquake added the label Wallet on Oct 14, 2021
3. in src/wallet/scriptpubkeyman.cpp:377 in e3ac2c0d41 outdated

   373@@ -374,7 +374,7 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
   374         auto it = mapKeyMetadata.find(keyid);
   375         if (it != mapKeyMetadata.end()){
   376             CKeyMetadata meta = it->second;
   377-            if (!meta.hd_seed_id.IsNull() && meta.hd_seed_id != m_hd_chain.seed_id) {
   378+            if (!meta.hd_seed_id.IsNull() && meta.hd_seed_id != m_hd_chain.seed_id && meta.key_origin.path.size() > 0) {
   

   ---

   ---

   

   laanwj commented at 3:45 pm on October 14, 2021:

   Wouldn’t meta.key_origin.path.size() >= 3 be better if the worry is to go out of range here? Might also log a warning when this happens as it means (I guess?) that the wallet is somehow corrupted.
4. 
   laanwj commented at 3:49 pm on October 14, 2021: member

   Concept ACK, I agree this shouldn’t cause a crash.
5. Add size check on meta.key_origin.path

   Resolves segfault on legacy wallet
   
   Log warning when meta.key_origin.path is below expected size

   d58118fa87
6. rooprob force-pushed on Oct 14, 2021
7. log has_key_origin 517c7265c2
8. fanquake requested review from achow101 on Oct 15, 2021
9. 
   achow101 commented at 2:34 am on October 15, 2021: member

   The root cause of the problem this is fixing is that the wallet does not contain the upgraded key metadata because it is encrypted and has not been unlocked yet, and a transaction was found that involved inactive HD chains. So this segfault can be triggered with a non-corrupted wallet. I don’t think it is necessarily correct to log an error and move on as doing so could result in missing funds.

   To avoid the crash, I think access to meta.key_origin.path should be guarded by both the length check and meta.has_key_origin. Then if has_key_origin == false, meta.hdKeypath can be used to figure out what to derive. This would avoid the segfault while not losing any functionality.

10. 
    rooprob commented at 8:57 am on October 15, 2021: contributor

    when I encountered this, this was the state of meta,

    0(gdb)
    1p meta
    2$1 = {static VERSION_BASIC = 1, static VERSION_WITH_HDDATA = 10, static VERSION_WITH_KEY_ORIGIN = 12, 
    3  static CURRENT_VERSION = 12, nVersion = 10, nCreateTime = 1509688921, hdKeypath = "m/0'/0'/0'", 
    4  hd_seed_id = {<uint160> = {<base_blob<160>> = {static WIDTH = 20, 
    5        m_data = "4\264Y\351....", <incomplete sequence \313>}, <No data fields>}, <No data fields>}, key_origin = {fingerprint = "\000\000\000", path = std::vector of length 0, capacity 0}, has_key_origin = false}
    6(gdb)
    7                                                                             
    

11. MarcoFalke renamed this:
    Add size check on meta.key_origin.path
    wallet: Add size check on meta.key_origin.path
    on Oct 15, 2021
12. 
    achow101 commented at 4:44 pm on October 15, 2021: member

    when I encountered this, this was the state of meta,

    This matches what I expect. The meta is not upgraded (nVersion is 10, not 12, and has_key_origin is false).

13. 
    achow101 commented at 6:33 pm on October 18, 2021: member

    Please squash your commits.

    I will implement the derivation from the keypath string in a followup PR as that is a bit more complicated to deal with.

14. 
    DrahtBot commented at 8:52 am on October 19, 2021: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #23304 (wallet: Derive inactive HD chains in addtional places by achow101)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

15. 
    MarcoFalke commented at 9:09 am on October 19, 2021: member

    Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
16. 
    achow101 commented at 4:45 pm on November 2, 2021: member

    @rooprob Are you planning on continuing to work on this PR? If not, I can take it over in #23304
17. 
    rooprob commented at 2:38 am on November 3, 2021: contributor

    @rooprob Are you planning on continuing to work on this PR? If not, I can take it over in #23304

    I think it’s better to follow up in your PR. I shall withdraw this PR.

    Thanks for picking this up in #23304

18. rooprob closed this on Nov 3, 2021

    ---

19. DrahtBot locked this on Nov 3, 2022

Contributors
rooprob laanwj achow101 DrahtBot MarcoFalke

Review Requested
achow101

Labels
Wallet