wallet: SecureString to allow null characters #27068

john-moffett · 2023-02-09T18:53:40Z

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):

std::string orig_string;
std::cin >> orig_string;
SecureString s;
s.reserve(100);
// The following all compile identically
s = orig_string;
s = std::string_view{orig_string};
s.assign(std::string_view{orig_string});
s.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.

DrahtBot · 2023-02-09T18:53:43Z

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.

maflcko

Should this include a release note, given the behaviour change?

src/wallet/rpc/encrypt.cpp

achow101 · 2023-02-09T23:48:29Z

ACK 6ac2723

furszy

Nice finding, code review ACK 6ac2723

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".

john-moffett · 2023-02-10T17:36:47Z

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:

The one you described -- though we may want to be explicit about the version they'd need to run.
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."
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.

stickies-v

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?

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?

john-moffett · 2023-02-13T17:04:32Z

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?

achow101 · 2023-02-13T20:25:28Z

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.

stickies-v · 2023-02-14T11:22:16Z

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.

john-moffett · 2023-02-14T15:12:34Z

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.

achow101 · 2023-02-14T16:21:23Z

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.

achow101 · 2023-02-14T22:47:52Z

ACK d73721b

furszy

ACK d73721b
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.

stickies-v

ACK d73721b

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 decddc8 is a bit misleading. SecureString already supported null chars, we just weren't initializing it properly.

stickies-v · 2023-02-21T18:31:48Z

src/qt/askpassphrasedialog.cpp

+                                          tr("The passphrase entered for the wallet decryption was incorrect. "
+                                             "The passphrase contains a null character (ie - a zero byte). "


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).

Suggested change

tr("The passphrase entered for the wallet decryption was incorrect. "

"The passphrase contains a null character (ie - a zero byte). "

tr("The old passphrase entered for the wallet decryption is incorrect. "

"It contains a null character (ie - a zero byte). "

`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.

Add a functional test to make sure the system properly accepts passphrases with null characters.

To reflect the change in behavior.

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.

john-moffett · 2023-02-21T20:36:56Z

Thanks @stickies-v! Incorporated both suggestions.

range-diff:

    @@ src/qt/askpassphrasedialog.cpp: void AskPassphraseDialog::accept()
-                                          tr("The passphrase entered for the wallet decryption was incorrect. "
-                                             "The passphrase contains a null character (ie - a zero byte). "
-                                             "If this passphrase was set with a version of this software prior to 25.0, "
+                                          tr("The passphrase entered for the wallet decryption is incorrect. "
+                                             "It contains a null character (ie - a zero byte). "
+                                             "If the passphrase was set with a version of this software prior to 25.0, "
    @@ src/qt/askpassphrasedialog.cpp: void AskPassphraseDialog::accept()
-                                          tr("The passphrase entered for the wallet decryption was incorrect. "
-                                             "The passphrase contains a null character (ie - a zero byte). "
-                                             "If this passphrase was set with a version of this software prior to 25.0, "
+                                          tr("The old passphrase entered for the wallet decryption is incorrect. "
+                                             "It contains a null character (ie - a zero byte). "
+                                             "If the passphrase was set with a version of this software prior to 25.0, "
    @@ src/wallet/rpc/encrypt.cpp: RPCHelpMan walletpassphrase()
-                throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect. "
-                                                                    "The passphrase contains a null character (ie - a zero byte). "
-                                                                    "If this passphrase was set with a version of this software prior to 25.0, "
+                throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered is incorrect. "
+                                                                    "It contains a null character (ie - a zero byte). "
+                                                                    "If the passphrase was set with a version of this software prior to 25.0, "
    @@ src/wallet/rpc/encrypt.cpp: RPCHelpMan walletpassphrasechange()
-            throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect. "
-                                                                "The passphrase contains a null character (ie - a zero byte). "
-                                                                "If this passphrase was set with a version of this software prior to 25.0, "
+            throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The old wallet passphrase entered is incorrect. "
+                                                                "It contains a null character (ie - a zero byte). "
+                                                                "If the old passphrase was set with a version of this software prior to 25.0, "

stickies-v

re-ACK 4bbf5dd

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

achow101 · 2023-02-22T01:34:58Z

re-ACK 4bbf5dd

furszy

utACK 4bbf5dd

4bbf5dd Detailed error message for passphrases with null chars (John Moffett) b4bdabc doc: Release notes for 27068 (John Moffett) 4b1205b Test case for passphrases with null characters (John Moffett) 00a0861 Pass all characters to SecureString including nulls (John Moffett) Pull request description: `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`): ```C++ std::string orig_string; std::cin >> orig_string; SecureString s; s.reserve(100); // The following all compile identically s = orig_string; s = std::string_view{orig_string}; s.assign(std::string_view{orig_string}); s.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 bitcoin#27067. ACKs for top commit: achow101: re-ACK 4bbf5dd stickies-v: re-ACK [4bbf5dd](bitcoin@4bbf5dd) furszy: utACK 4bbf5dd Tree-SHA512: 47a96905a82ca674b18076a20a388123beedf70e9de73e42574ea68afbb434734e56021835dd9b148cdbf61709926b487cc95e9021d9bc534a7c93b3e143d2f7

DrahtBot added the Wallet label Feb 9, 2023

maflcko reviewed Feb 9, 2023

View reviewed changes

src/wallet/rpc/encrypt.cpp Outdated Show resolved Hide resolved

john-moffett force-pushed the 2023_02_SecurerSecureString branch 2 times, most recently from 2119fd3 to 6ac2723 Compare February 9, 2023 22:35

DrahtBot mentioned this pull request Feb 10, 2023

wallet: ensure the wallet is unlocked when needed for rescanning #26347

Merged

furszy approved these changes Feb 10, 2023

View reviewed changes

stickies-v reviewed Feb 10, 2023

View reviewed changes

UserError0815 mentioned this pull request Feb 12, 2023

Raven Core Passphrase is not working RavenProject/Ravencoin#1176

Open

DrahtBot requested a review from furszy February 14, 2023 22:47

furszy approved these changes Feb 15, 2023

View reviewed changes

achow101 requested a review from stickies-v February 16, 2023 18:15

achow101 mentioned this pull request Feb 16, 2023

wallet: be able to specify a wallet name and passphrase to migratewallet #26595

Merged

DrahtBot added the Needs rebase label Feb 21, 2023

stickies-v approved these changes Feb 21, 2023

View reviewed changes

john-moffett added 4 commits February 21, 2023 14:40

Test case for passphrases with null characters

4b1205b

Add a functional test to make sure the system properly accepts passphrases with null characters.

doc: Release notes for 27068

b4bdabc

To reflect the change in behavior.

john-moffett force-pushed the 2023_02_SecurerSecureString branch from d73721b to 4bbf5dd Compare February 21, 2023 20:27

DrahtBot removed the Needs rebase label Feb 21, 2023

stickies-v approved these changes Feb 21, 2023

View reviewed changes

DrahtBot requested review from achow101 and furszy February 21, 2023 23:15

furszy reviewed Feb 22, 2023

View reviewed changes

achow101 merged commit 5e55534 into bitcoin:master Feb 22, 2023

barton2526 mentioned this pull request Jun 5, 2023

wallet: SecureString to allow null characters gridcoin-community/Gridcoin-Research#2690

Merged

kwvg mentioned this pull request Jan 29, 2024

feat: enable HD wallets by default dashpay/dash#5807

Merged

5 tasks

bitcoin locked and limited conversation to collaborators Feb 22, 2024

		tr("The passphrase entered for the wallet decryption was incorrect. "
		"The passphrase contains a null character (ie - a zero byte). "

wallet: SecureString to allow null characters #27068

wallet: SecureString to allow null characters #27068

Uh oh!

Conversation

john-moffett commented Feb 9, 2023

Uh oh!

DrahtBot commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Feb 9, 2023

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-moffett commented Feb 10, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

john-moffett commented Feb 13, 2023

Uh oh!

achow101 commented Feb 13, 2023

Uh oh!

stickies-v commented Feb 14, 2023

Uh oh!

john-moffett commented Feb 14, 2023

Uh oh!

achow101 commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Feb 14, 2023

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

john-moffett commented Feb 21, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Feb 22, 2023

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Feb 9, 2023 •

edited

Loading

furszy left a comment •

edited

Loading

achow101 commented Feb 14, 2023 •

edited

Loading

stickies-v left a comment •

edited

Loading