Implement pinentry wrapper to unlock bitcoin wallet

eklitzke commented at 8:28 am on May 28, 2018: contributor

This adds a new optional build executable called bitcoin-wallet-unlock. This program invokes Pinentry (part of GnuPG) to securely read the user’s wallet passphrase. This is intended to be used by users of bitcion-cli, as bitcoin-qt already reads wallet passphrases securely.

Details

Normally parameters to Bitcoin RPC methods are passed as program arguments to bitcoin-cli. This is insecure for walletpassphrase for two reasons:

The passphrase will be leaked to other processes via the command name, e.g. another user running top or ps might see the passphrase in the clear.
In all likelihood the command invocation will end up in the user’s shell history.

A commonly cited workaround is to use bitcoin-cli -stdin walletpassphrase. In this mode the command arguments are provided to bitcoin-cli over stdin, and this allows the user to enter a passphrase (and timeout) without the issues described above. However, it is still not perfect, as bitcoin-cli keeps stdin in ICANON mode. This means that if you literally type bitcoin-cli -stdin walletpassphrase and enter a passphrase and timeout value, the passphrase you type will be echoed on the terminal in the clear. Another person shoulder-surfing will be able to see the cleartext passphrase, as can any process doing a screen capture.

Pinentry solves this problem and a few others. When reading from the TTY, Pinentry disables echoing to the console, which means that shoulder-surfers can’t see the passphrase. Pinentry has some other nifty features. For example, if you invoke pinentry from a terminal emulator in a graphical session in most cases it can automatically upgrade to a graphical input method, e.g. using GTK. This is what input looks like by default for me in GNOME when running bitcoin-wallet-unlock from a terminal: https://monad.io/bwemqwkg.png

This implementation of bitcoin-wallet-unlock allows using -p to force a particular pinentry backend, and -t to force TTY input. Extra arguments passed to bitcoin-wallet-unlock will be forwarded to the underlying pinentry program; there’s an example of this in the comment for GetPinentryPassphrase(). Communication with Pinentry is done using a limited subset of the assuan protocol, which is described here.

These are the caveats I know of with this PR:

Since the HTTP client code in bitcoin-cli isn’t well factored, I am just execing bitcoin-cli, rather than actually making the RPC request from the bitcoin-wallet-unlock program. This feels a little bit hacky, but minimizes code churn.
The default behavior is to use a relative path for bitcoin-cli and pinentry when calling execvp(). This could potentially cause a malicious program to be invoked if an attacker can put bad things in the users PATH. This can be mitigated by invoking bitcoin-wallet-unlock with absolute paths, e.g. bitcoin-wallet-unlock -c /usr/bin/bitcoin-cli -p /usr/bin/pinentry.
Graphical pinentry methods can delegate to a desktop password manager (e.g. with gnome-keyring-daemon on GNOME) when pinentry is linked with libsecret. That is a cool feautre, but I don’t enable this because I don’t think it’s secure (it’s trivial to dump the plaintext passwords with these once you’re logged in as a user).
This code is not portable to Windows as it uses pipe() and exec() (although I suspect most Windows users are using bitcoin-qt).

Implement pinentry wrapper to unlock bitcoin wallet 06b238bb0c

ken2812221 commented at 8:59 am on May 28, 2018: contributor

I don’t think it is a good idea to depends on programs. If it could depend on libraries, it would be better.

laanwj commented at 1:15 pm on May 28, 2018: member

Concept ACK.

I don’t think it is a good idea to depends on programs. If it could depend on libraries, it would be better.

In general I don’t think that’s bad advice. But, specifically, reading passphrases might be something that is preferable to do in a separate process address space, to minimize how much it sticks around.

laanwj added the label RPC/REST/ZMQ on May 29, 2018

laanwj added the label Wallet on May 29, 2018

in src/bitcoin-wallet-unlock.cpp:130 in 06b238bb0c

125+// separated using -- as in the example above to prevent getopt_long() to
126+// interpret these arguments as program flags.
127+//
128+// These extra arguments are forwarded directly to execvp(), so there are no
129+// shell expansion or escaping issues to be aware of.
130+static std::string GetPinentryPassphrase(

ryanofsky commented at 6:03 pm on May 29, 2018:

Does it make sense to use SecureString type in some of these places?

ryanofsky commented at 6:12 pm on May 29, 2018: member

This seems good. I looked a little at the code. It seems like the main advantage pinentry provides here is GUI integration. Unless I’m missing something, the other benefits cited in the PR description could just as well come from shipping a little bash script like:

0read -s pass
1bitcoin-cli -stdin walletpassphrase <<<"$pass"

ryanofsky commented at 6:22 pm on May 29, 2018: member

It also occurred to me that maybe it would be more convenient for users if instead of providing a “bitcoin-wallet-unlock” tool that calls “bitcoin-cli”, we provided a “bitcoin-askpass” tool that could get called by “bitcoin-cli” by default when a password wasn’t provided.

kristapsk commented at 9:16 pm on May 29, 2018: contributor

It also occurred to me that maybe it would be more convenient for users if instead of providing a “bitcoin-wallet-unlock” tool that calls “bitcoin-cli”, we provided a “bitcoin-askpass” tool that could get called by “bitcoin-cli” by default when a password wasn’t provided.

Something like this sounds like a very good idea to me. I don’t put walletpassphrase in my scripts and very often I forgot to run it manually and it’s little bit annoying. :)

Probably good idea would also be to ask for walletpassphrase and then automatically lock wallet again when the PID which called bitcoin-cli, which caused walletpassphrase prompt, has terminated.

jonasschnelli commented at 8:34 am on May 30, 2018: contributor

I think providing users a way how to securely enter a passphrase on console level make sense. A couple of points/thoughts:

Unlocking the traditional, insecure way would still be possible, would it make sense to make users aware if they use an insecure way? Bitcoin-cli could integrate the pinentry process and use a -insecure option if one wants to use walletpassphrase with the insecure passphrase parameter. Using it on pure RPC level is another things (we don’t want to handhold there).
Similar to the point above, I think it should be integrated into bitcoin-cli (not another executable, if, then combined with other wallet functions, see #8745)
If not integrated, could this be something that would be better in the contrib/ folder since there are other ways how to do this securely (see @ryanofsky comment)
Also, using external software for passphrase entry would move a critical part further away from our controlled stack (shell is not in our controlled stack, so not sure if this is an argument).
If this gets added to the main binaries, disable it by default (via configure.ac) for the first (or the first two) releases would probably make sense, since this is security critical and deserves more field test before defaulting to true.

MarcoFalke added the label Up for grabs on Jul 29, 2018

MarcoFalke commented at 3:05 pm on July 29, 2018: member

No activity for 2 months and travis is failing, so I marked this up for grabs to be submitted in a new pull request.

MarcoFalke closed this on Jul 29, 2018

DrahtBot locked this on Sep 8, 2021

Implement pinentry wrapper to unlock bitcoin wallet #13335

Details