wallet: SecureString to allow null characters #27068

pull john-moffett wants to merge 4 commits into bitcoin:master from john-moffett:2023_02_SecurerSecureString changing 6 files +73 −22
  1. john-moffett commented at 6:53 pm on February 9, 2023: contributor

    SecureString is a std::string specialization with a secure allocator. However, in practice it’s treated like a C- string (no explicit length and null-terminated). This can cause unexpected and potentially insecure behavior. For instance, if a user enters a passphrase with embedded null characters (which is possible through Qt and the JSON-RPC), it will ignore any characters after the first null, potentially giving the user a false sense of security.

    Instead of assigning to SecureString via std::string::c_str(), assign it via a std::string_view of the original. This explicitly captures the size and still doesn’t make any extraneous copies in memory.

    Note to reviewers, the following all compile identically in recent GCC (x86-64 and ARM64) with -O2 (and -std=c++17):

    0std::string orig_string;
    1std::cin >> orig_string;
    2SecureString s;
    3s.reserve(100);
    4// The following all compile identically
    5s = orig_string;
    6s = std::string_view{orig_string};
    7s.assign(std::string_view{orig_string});
    8s.assign(orig_string.data(), orig_string.size());
    

    So it’s largely a matter of preference. However, one thing to keep in mind is that we want to avoid making unnecessary copies of any sensitive data in memory.

    Something like SecureString s{orig_string}; is still invalid and probably unwanted in our case, since it’d get treated as a short string and optimized away from the secure allocator. I presume that’s the reason for the reserve() calls.

    Fixes #27067.

  2. DrahtBot commented at 6:53 pm on February 9, 2023: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK stickies-v, achow101, furszy

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

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Wallet on Feb 9, 2023
  4. in src/wallet/rpc/encrypt.cpp:229 in d7b59b498c outdated
    225@@ -229,11 +226,10 @@ RPCHelpMan encryptwallet()
    226         throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an encrypted wallet, but encryptwallet was called.");
    227     }
    228 
    229-    // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
    230-    // Alternately, find a way to make request.params[0] mlock()'d to begin with.
    231+    // Consider finding a way to make request.params[0] mlock()ed
    


    maflcko commented at 7:36 pm on February 9, 2023:
    Instead of repeating this in all places where SecureString is used, what about moving it to src/support/allocators/secure.h once?
  5. maflcko commented at 7:41 pm on February 9, 2023: member
    Should this include a release note, given the behaviour change?
  6. john-moffett force-pushed on Feb 9, 2023
  7. john-moffett force-pushed on Feb 9, 2023
  8. achow101 commented at 11:48 pm on February 9, 2023: member
    ACK 6ac272390a3d39ee0151cddc6cfa03877a9fd600
  9. furszy approved
  10. furszy commented at 1:06 pm on February 10, 2023: member

    Nice finding, code review ACK 6ac27239

    As the impossibility to unlock the wallet could be an stressful time, what about notifying the user if the passphrase has a null character and decryption failed?. Could return something like “Error, decryption failed; the passphrase contain a null character, try running a previous release and change your passphrase to one without null characters”.

  11. john-moffett commented at 5:36 pm on February 10, 2023: contributor

    As the impossibility to unlock the wallet could be an stressful time, what about notifying the user if the passphrase has a null character and decryption failed?.

    Interesting idea! I agree that it could be useful to provide feedback in these (likely unusual) cases.

    I can think of three potential approaches:

    1. The one you described – though we may want to be explicit about the version they’d need to run.
    2. Return something like “Error: decryption failed: the passphrase contains a null character. If this passphrase was set with a version of this software prior to 25.0, please try again with only the characters up to the first null character.”
    3. Check to see if the wallet would decrypt using the substring up to the first null and only then emit errors (or even just warnings) like in 1 or 2.

    The third option is probably the most user-friendly, and I don’t think it’s a significant security reduction (it’s a free second attempt), but my intuition says it’s the wrong approach.

  12. stickies-v commented at 6:44 pm on February 10, 2023: contributor

    Concept ACK, great catch indeed.

    I presume that’s the reason for the reserve() calls.

    Or, perhaps, it’s so the password length doesn’t leak?

    1. Check to see if the wallet would decrypt using the substring up to the first null and only then emit errors (or even just warnings) like in 1 or 2.

    I think this is the best approach. It’s not that far-fetched for a user to have used a null character in their password (e.g. through random password generator) so this will quite likely actually affect some users? I think we need to make this as smooth and least scary as possible. Additionally, I think we could also encourage the user to re-encrypt the wallet with a different passphrase (without null char) for maximum backward compatibility, and perhaps even offer a one-click solution to do so in qt?

  13. john-moffett commented at 5:04 pm on February 13, 2023: contributor

    Concept ACK, great catch indeed.

    Thank you (and @furszy)!

    Check to see if the wallet would decrypt using the substring up to the first null and only then emit errors (or even just warnings) like in 1 or 2.

    I think this is the best approach. … Additionally, I think we could also encourage the user to re-encrypt the wallet with a different passphrase (without null char) for maximum backward compatibility, and perhaps even offer a one-click solution to do so in qt?

    Sounds good, but it’s a somewhat extensive change, so it’d be nice to first get more feedback on whether this is the best approach. @achow101, @Sjors, @MarcoFalke, @furszy any thoughts?

  14. achow101 commented at 8:25 pm on February 13, 2023: member
    1. Check to see if the wallet would decrypt using the substring up to the first null and only then emit errors (or even just warnings) like in 1 or 2.

    I think this is the best approach. It’s not that far-fetched for a user to have used a null character in their password (e.g. through random password generator) so this will quite likely actually affect some users?

    I highly doubt it. I would be surprised if any password manager produced a passphrase that included non-typeable characters. I’m pretty sure they will stick to ascii, or the unicode for whatever the users’ locale is, none of which will include a null character. Password managers make passwords that the user can still type out manually if they have to. I think this change will effect 0 people.

    I think the best approach is an error message that tells the user to both try with up to the null character and to change their passphrase.

  15. stickies-v commented at 11:22 am on February 14, 2023: contributor

    I would be surprised if any password manager produced a passphrase that included non-typeable characters.

    I googled “random password generator” and with the second result I was able to generate a null-character containing password (\039"`\9%|+’]1+5) in less than a minute (disabled using letters to speed it up, but otherwise using default settings). It seems like indeed a fair amount of generators do not use backslashes as symbols (e.g. Bitwarden, Lastpass), but I personally would not want to vouch for that?

    I’m not against an instruction-only approach to minimize code burden. I just think we need to be cautious here and assume some users will be affected.

  16. john-moffett commented at 3:12 pm on February 14, 2023: contributor

    Thanks for the feedback!

    I agree that it likely will only affect a very small proportion of users, but given that Bitcoiners can be somewhat peculiar, it’s impossible to be certain. Since it’s a non-destructive change, I think it’s acceptable to just return a detailed error message for now. If it turns out that it affects more people than expected, we can add a more user-friendly process.

    If nobody has any objections, I’ll work on a commit that adds a detailed error message suggesting that they try the password up to the null character and subsequently change it if it’s successful.

  17. achow101 commented at 4:21 pm on February 14, 2023: member

    I googled “random password generator” and with the second result I was able to generate a null-character containing password (\039"`\9%|+’]1+5)

    But that doesn’t actually contain a null character. That contains a backslash followed by a 0, which some things may interpret as a null character, but not always, and not necessarily in a way that will affect us. For example. my terminal will interpret it as a null character if I don’t put quotes around it. It will give to the program everything up to the null character, so we wouldn’t even know that the passphrase is supposed to have a null character. Qt will not interpret that sequence as a null character, it will find it to be a backslash followed by a 0. In both of these cases, the passphrase that we receive would not contain a null character.

  18. achow101 commented at 10:47 pm on February 14, 2023: member
    ACK d73721b2be27afd9906ed362e286dd3d0a414413
  19. DrahtBot requested review from furszy on Feb 14, 2023
  20. furszy approved
  21. furszy commented at 11:40 pm on February 15, 2023: member

    ACK d73721b2 The current approach sounds good to me.

    Side note: I’m not fan of the dup error strings but.. it’s the less of the evils.

  22. achow101 requested review from stickies-v on Feb 16, 2023
  23. in src/qt/askpassphrasedialog.cpp:194 in d73721b2be outdated
    191+                    QMessageBox::critical(this, tr("Passphrase change failed"),
    192+                                          tr("The passphrase entered for the wallet decryption was incorrect."));
    193+                } else {
    194+                    QMessageBox::critical(this, tr("Passphrase change failed"),
    195+                                          tr("The passphrase entered for the wallet decryption was incorrect. "
    196+                                             "The passphrase contains a null character (ie - a zero byte). "
    


    stickies-v commented at 6:31 pm on February 21, 2023:

    nit: slight rephrasing for tense consistency and highlighting that it’s the old password that has the null character (also (partially) affects the other if-branch as well as the other sites where this message is raised).

    0                                          tr("The old passphrase entered for the wallet decryption is incorrect. "
    1                                             "It contains a null character (ie - a zero byte). "
    
  24. DrahtBot added the label Needs rebase on Feb 21, 2023
  25. stickies-v approved
  26. stickies-v commented at 7:30 pm on February 21, 2023: contributor

    ACK d73721b2be27afd9906ed362e286dd3d0a414413

    2 non-blocking nits best to be ignored if no further force pushes are necessary. (edit: looks like rebase is now needed anyway)


    But that doesn’t actually contain a null character. That contains a backslash followed by a 0, which some things may interpret as a null character, but not always, and not necessarily in a way that will affect us.

    Thanks, I didn’t realize this. I verified it both on cli and Qt and you’re right. I agree that the chances of this actually being a problem are now much lower, and I think the current approach is appropriate.


    nit: I think the commit title of decddc88364dc7d271d9e72d141ecde9612b2468 is a bit misleading. SecureString already supported null chars, we just weren’t initializing it properly.

  27. Pass all characters to SecureString including nulls
    `SecureString` is a `std::string` specialization with
    a secure allocator. However, it's treated like a C-
    string (no explicit length and null-terminated). This
    can cause unexpected behavior. For instance, if a user
    enters a passphrase with an embedded null character
    (which is possible through Qt and the JSON-RPC), it will
    ignore any characters after the null, giving the user
    a false sense of security.
    
    Instead of assigning `SecureString` via `std::string::c_str()`,
    assign it via a `std::string_view` of the original. This
    explicitly captures the size and doesn't make any extraneous
    copies in memory.
    00a0861181
  28. Test case for passphrases with null characters
    Add a functional test to make sure the system
    properly accepts passphrases with null characters.
    4b1205ba37
  29. doc: Release notes for 27068
    To reflect the change in behavior.
    b4bdabc223
  30. Detailed error message for passphrases with null chars
    Since users may have thought the null characters in their
    passphrases were actually evaluated prior to this change,
    they may be surprised to learn that their passphrases no
    longer work. Give them feedback to explain how to remedy
    the issue.
    4bbf5ddd44
  31. john-moffett force-pushed on Feb 21, 2023
  32. john-moffett commented at 8:36 pm on February 21, 2023: contributor

    Thanks @stickies-v! Incorporated both suggestions.

    range-diff:

     0    @@ src/qt/askpassphrasedialog.cpp: void AskPassphraseDialog::accept()
     1-                                          tr("The passphrase entered for the wallet decryption was incorrect. "
     2-                                             "The passphrase contains a null character (ie - a zero byte). "
     3-                                             "If this passphrase was set with a version of this software prior to 25.0, "
     4+                                          tr("The passphrase entered for the wallet decryption is incorrect. "
     5+                                             "It contains a null character (ie - a zero byte). "
     6+                                             "If the passphrase was set with a version of this software prior to 25.0, "
     7    @@ src/qt/askpassphrasedialog.cpp: void AskPassphraseDialog::accept()
     8-                                          tr("The passphrase entered for the wallet decryption was incorrect. "
     9-                                             "The passphrase contains a null character (ie - a zero byte). "
    10-                                             "If this passphrase was set with a version of this software prior to 25.0, "
    11+                                          tr("The old passphrase entered for the wallet decryption is incorrect. "
    12+                                             "It contains a null character (ie - a zero byte). "
    13+                                             "If the passphrase was set with a version of this software prior to 25.0, "
    14    @@ src/wallet/rpc/encrypt.cpp: RPCHelpMan walletpassphrase()
    15-                throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect. "
    16-                                                                    "The passphrase contains a null character (ie - a zero byte). "
    17-                                                                    "If this passphrase was set with a version of this software prior to 25.0, "
    18+                throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered is incorrect. "
    19+                                                                    "It contains a null character (ie - a zero byte). "
    20+                                                                    "If the passphrase was set with a version of this software prior to 25.0, "
    21    @@ src/wallet/rpc/encrypt.cpp: RPCHelpMan walletpassphrasechange()
    22-            throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect. "
    23-                                                                "The passphrase contains a null character (ie - a zero byte). "
    24-                                                                "If this passphrase was set with a version of this software prior to 25.0, "
    25+            throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The old wallet passphrase entered is incorrect. "
    26+                                                                "It contains a null character (ie - a zero byte). "
    27+                                                                "If the old passphrase was set with a version of this software prior to 25.0, "
    
  33. DrahtBot removed the label Needs rebase on Feb 21, 2023
  34. stickies-v approved
  35. stickies-v commented at 11:15 pm on February 21, 2023: contributor

    re-ACK 4bbf5dd

    I verified that the only changes are as per review comments: updated commit title and slight phrasing tweaks to error messages.

  36. DrahtBot requested review from achow101 on Feb 21, 2023
  37. DrahtBot requested review from furszy on Feb 21, 2023
  38. achow101 commented at 1:34 am on February 22, 2023: member
    re-ACK 4bbf5ddd44bde15b328be131922123eaa3212a7e
  39. furszy commented at 4:06 pm on February 22, 2023: member
    utACK 4bbf5ddd
  40. achow101 merged this on Feb 22, 2023
  41. achow101 closed this on Feb 22, 2023

  42. sidhujag referenced this in commit df24a357d3 on Feb 25, 2023
  43. bitcoin locked this on Feb 22, 2024

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: 2024-11-17 12:12 UTC

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