key: clear out secret data in DecodeExtKey #31166

1. 
   theStack commented at 2:41 pm on October 27, 2024: contributor

   Same as in DecodeSecret, we should also clear out the secret data from the vector resulting from the Base58Check parsing for xprv keys. Note that the if condition is needed in order to avoid UB, see #14242 (commit d855e4cac8303ad4e34ac31cfa7634286589ce99).
2. key: clear out secret data in `DecodeExtKey`

   Same as in `DecodeSecret`, we should also clear out the secret data from
   the vector resulting from the Base58Check parsing for xprv keys. Note
   that the if condition is needed in order to avoid UB, see #14242 (commit
   d855e4cac8303ad4e34ac31cfa7634286589ce99).

   559a8dd9c0
3. 
   DrahtBot commented at 2:41 pm on October 27, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	tdb3, laanwj, achow101, davidgumberg

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. laanwj added the label Wallet on Oct 27, 2024
5. tdb3 approved
6. 
   tdb3 commented at 10:03 pm on October 28, 2024: contributor

   cr ACK 559a8dd9c0aafcecf00f9ccd9aabe5720bcebe8c

   Good catch

7. laanwj approved
8. 
   laanwj commented at 2:54 pm on October 29, 2024: member

   Code review ACK 559a8dd9c0aafcecf00f9ccd9aabe5720bcebe8c
9. 
   davidgumberg commented at 7:04 am on October 30, 2024: contributor

   What about using the secure allocator instead of manually cleansing?

   0 CExtKey DecodeExtKey(const std::string& str)
   1 {
   2     CExtKey key;
   3-     std::vector<unsigned char> data;
   4+     std::vector<unsigned char, secure_allocator> data;
   

   So that secrets never gets paged to disk, which is worse IMO than being left in memory, this would apply to the rest of the memory_cleanse’d data in key_io.cpp if it makes sense here.

10. 
    laanwj commented at 7:23 am on October 30, 2024: member

    Yes, using secure_allocator is a better option for sensitive key data, if possible.

    Edit: however, most of our API functions do not handle vectors with alternative allocators, including DecodeBase58Check, doing so would be a larger change. i don’t think swapping out is a very big risk here, as the allocation is very short-lived. secure_allocator is mostly meant for objects such as wallet keys that linger around in memory for a long time.

11. 
    theStack commented at 11:41 am on October 30, 2024: contributor

    @davidgumberg: Good point, that makes a lot of sense. It seems there are more changes needed to make this work though, e.g. DecodeBase58Check currently only accepts a std::vector<unsigned char> with the default allocator, so one would need to template that function to accept std::vector<unsigned char, T> (if that works, didn’t try). Also, assigning the Base58Prefix causes a similar type mismatch. Might be a potential follow-up if someone is motivated to look into that?

    // EDIT: ah, laanwj was faster, see one post above :)

12. 
    davidgumberg commented at 2:58 pm on October 30, 2024: contributor

    utACK https://github.com/bitcoin/bitcoin/pull/31166/commits/559a8dd9c0aafcecf00f9ccd9aabe5720bcebe8c

    I agree that given how short lived this data is it doesn’t seem likely to get paged, and think that this PR improves things as-is.

    It would still be nice to encapsulate our allocation strategy for secrets, but not necessary here and can be handled in a follow-up.

13. 
    achow101 commented at 6:18 pm on October 30, 2024: member

    ACK 559a8dd9c0aafcecf00f9ccd9aabe5720bcebe8c
14. achow101 merged this on Oct 30, 2024
15. achow101 closed this on Oct 30, 2024

    ---

16. theStack deleted the branch on Oct 30, 2024

Contributors
theStack DrahtBot tdb3 laanwj davidgumberg achow101

Labels
Wallet