[Qt][RPC] Hide passphrases in debug console history #8746
Conversation
Masks out passphrases with **PASSPHRASE**, **OLDPASSPHRASE**, and **NEWPASSPHRASE** to prevent the loss of passwords from people scrolling through debug console history.
|
There are more of them - like |
|
Is it make sense to put this "masked" commands to history? |
|
Concept ACK
I like this idea, it would also make the code simpler. Just have a list of 'masked' RPC calls, instead of having to handle the arguments per call. |
| message(CMD_REQUEST, cmd); | ||
| Q_EMIT cmdRequest(cmd); | ||
| // Remove passphrases from history | ||
| QStringList cmdList = cmd.split(' '); |
There was a problem hiding this comment.
Unfortunately, splitting on ' ' is not enough here. Arguments can contain spaces if they're surrounded with ' or ". E.g. walletpassphrase "my voice is my password verify me"
There was a problem hiding this comment.
That doesn't matter since only the command (first in the list) and the timeout (last in the list) are actually needed. Anything else in between doesn't matter.
There was a problem hiding this comment.
Sure. What I'm afraid of is that in the future someone is bound to add a command for which this is not true. The handling per command is a bit brittle.
So I'd prefer these commands to not be added to history in the first place.
|
Generally concept ACK. Though, I think we should add a flag (maybe on level of the The flag could be labeled |
|
@jonasschnelli @laanwj I will see if I can figure something out for with a flag or lists and not actually checking the arguments in each call. My C++ is not the greatest so I don't know whether I can actually do that. Edit: I don't think something with CRPCTable would work since the string goes straight to the console window before it is even processed by the rpc server. |
|
Moving this to some kind of table makes sense, however let's not use the CRPCTable for this. It should not contain GUI-level (or in general, client-side) logic. That belongs in rpc/client.cpp or the GUI itself. |
|
Probably a good idea to reuse code from #5891 |
|
The person reporting this on Bitcointalk is claiming that the history persists across restarts: https://bitcointalk.org/index.php?topic=1618462.0 If that is true there may be other leaks to be concerned about here. |
|
I've been unable to reproduce what he is reporting. On September 26, 2016 12:50:52 PM Gregory Maxwell
|
|
History does not persist across restarts. There have been proposals for that but I've always been against that. It could be that one of the forks does, but not in Bitcoin Core. |
He's probably running Knots, not Core. Seems off-topic for this PR... |
|
Closing in favor of #8877 |
7 participants
Right now passphrases in the debug console are displayed in plaintext on screen as well as in the history. This will replace all the passphrases entered with the
walletpassphraseandwalletpassphrasechangeRPCs with a placeholder,**PASSPHRASE**, in order to prevent the loss of a password due to people looking at the debug console history.