[Qt][RPC] Hide passphrases in debug console history #8746

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:hide-walletpassphrase changing 1 files +9 −1

achow101 commented at 3:25 PM on September 16, 2016: member

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 walletpassphrase and walletpassphrasechange RPCs with a placeholder, **PASSPHRASE**, in order to prevent the loss of a password due to people looking at the debug console history.

Hide passphrases in rpcconsole history

Masks out passphrases with **PASSPHRASE**, **OLDPASSPHRASE**, and **NEWPASSPHRASE** to prevent the loss of passwords from people scrolling through debug console history.

f56998689e

jonasschnelli added the label GUI on Sep 16, 2016
paveljanik commented at 8:12 AM on September 17, 2016: contributor

There are more of them - like encryptwallet.
Mask out passphrase in encryptwallet 6d251655e2
Elbandi commented at 6:54 PM on September 17, 2016: contributor

Is it make sense to put this "masked" commands to history?
laanwj commented at 2:38 PM on September 19, 2016: member

Concept ACK

Is it make sense to put this "masked" commands to history?

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.
in src/qt/rpcconsole.cpp:None in 6d251655e2
```
 615 | @@ -616,8 +616,16 @@ void RPCConsole::on_lineEdit_returnPressed()
 616 |  
 617 |      if(!cmd.isEmpty())
 618 |      {
 619 | -        message(CMD_REQUEST, cmd);
 620 |          Q_EMIT cmdRequest(cmd);
 621 | +        // Remove passphrases from history
 622 | +        QStringList cmdList = cmd.split(' ');
```
laanwj commented at 2:40 PM on September 19, 2016:

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"

achow101 commented at 2:59 PM on September 19, 2016:

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.

laanwj commented at 4:14 PM on September 19, 2016:

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.
jonasschnelli commented at 1:10 PM on September 20, 2016: contributor

Generally concept ACK.

Though, I think we should add a flag (maybe on level of the CRPCTable) that could be used by the QT console to avoid storing a command-line containing one of these "flagged" methods into the local console history.

The flag could be labeled exposeSecrets and could be co-used for other purposed.
achow101 commented at 3:18 PM on September 20, 2016: member

@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.
laanwj commented at 3:38 PM on September 20, 2016: member

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.
luke-jr commented at 9:03 AM on September 22, 2016: member

Probably a good idea to reuse code from #5891
gmaxwell commented at 4:50 PM on September 26, 2016: contributor

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.
achow101 commented at 4:57 PM on September 26, 2016: member

I've been unable to reproduce what he is reporting.

On September 26, 2016 12:50:52 PM Gregory Maxwell notifications@github.com wrote:

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.

You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #8746 (comment)
laanwj commented at 9:47 PM on September 26, 2016: member

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.
luke-jr commented at 1:32 AM on September 27, 2016: member

The person reporting this on Bitcointalk is claiming that the history persists across restarts:

He's probably running Knots, not Core. Seems off-topic for this PR...
laanwj commented at 4:19 PM on October 4, 2016: member

Closing in favor of #8877
laanwj closed this on Oct 4, 2016
achow101 deleted the branch on Oct 29, 2016
achow101 restored the branch on Oct 29, 2016
achow101 deleted the branch on Apr 5, 2017
achow101 restored the branch on Apr 21, 2017
achow101 deleted the branch on May 17, 2017
achow101 restored the branch on Aug 17, 2018
achow101 deleted the branch on Aug 17, 2018
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#8877 Qt RPC console: history sensitive-data filter, and saving input line when browsing history