crypto: Use secure_allocator for AES256_ctx #31774

pull davidgumberg wants to merge 3 commits into bitcoin:master from davidgumberg:1-31-25-aes-secure-alloc changing 4 files +22 −13
  1. davidgumberg commented at 8:10 pm on January 31, 2025: contributor

    Fixes #31744

    Reuse secure_allocator for AES256_ctx in the aes 256 encrypters and decrypters and the iv of AES256CBC encrypters and decrypters. These classes are relevant to CCrypter, used for encrypting wallets, and my understanding is that if an attacker knows some or all of the contents of these data structures (AES256_ctx & iv) they might be able to decrypt a user’s wallet.

    Presently the secure_allocator tries to protect sensitive data with mlock() on POSIX systems and VirtualLock() on Windows to prevent memory being paged to disk, and by zero’ing out memory contents on deallocation with memory_cleanse() which is similar to OPENSSL_cleanse() by scaring compilers away from optimizing memset calls on non-Windows systems, and using SecureZeroMemory() on Windows.

  2. DrahtBot commented at 8:10 pm on January 31, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31774.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK theStack

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

  3. DrahtBot added the label Utils/log/libs on Jan 31, 2025
  4. build: Move `lockedpool.cpp` from util -> crypto
    Allows `crypto` functions and classes to use `secure_allocator`.
    2e0fac57e3
  5. crypto: Use `secure_allocator` for `AES256_ctx` a463001828
  6. crypto: Use `secure_allocator` for `AES256CBC*::iv` 68f1370bd3
  7. davidgumberg force-pushed on Jan 31, 2025
  8. DrahtBot added the label CI failed on Jan 31, 2025
  9. DrahtBot commented at 9:32 pm on January 31, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36501336640

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  10. DrahtBot removed the label CI failed on Jan 31, 2025
  11. theStack commented at 3:51 pm on February 4, 2025: contributor

    Concept ACK on clearing out the ctx/iv members

    I’m wondering if a minimum-diff fix which simply replaces the memset calls in the dtors with memory_cleanse would be largely sufficient here? In #31166 (comment) one argument for not needing secure allocators was the short-lived nature of the secrets. Looking at the only usage in the wallet, this would imho apply here too (en/decrypting might take a while for larger inputs, but I still wouldn’t classify that as long-lived):

    https://github.com/bitcoin/bitcoin/blob/94ca99ac51dddbee79d6409ebcc43b1119b0aca9/src/wallet/crypter.cpp#L85-L91 https://github.com/bitcoin/bitcoin/blob/94ca99ac51dddbee79d6409ebcc43b1119b0aca9/src/wallet/crypter.cpp#L102-L108

    The reasoning might be a bit loose as there might be other uses of these classes in the future where the situation is different. Curious about other opinions.

  12. sipa commented at 4:35 pm on February 4, 2025: member

    I think the approach here might have an unacceptable performance impact, as it is allocating and deallocating a secure area for every individual key being encrypted/decrypted.

    It may be better to:

    • Somehow make the AES256CBC classes members of CCrypter, surviving an individual encryption/decryption (e.g. by adding a reset function that can get called for each encryption/decryption, resetting the CBC state, but letting the key material survive).
    • Follow @theStack’s suggestion of not locking the memory, but just securely wiping it after each operation.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-02-07 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me