Add a footgun warning to any privkey operation.

[survic]

As discussed on #bitcoin-dev it happens that a large number of Bitcoin has been lost by users misunderstanding the gravity of the situation when playing with private keys. Online tutorials encourage users to use the importprivkey and dumpprivkey commands in general situations, leading to cases where they unwittingly erase their wallets or otherwise compromise them (private key to QR code websites were mentioned).

I propose that large warnings are given when executing these commands from the RPC console to attempt to give the user a sense of how much risk is involved in this action. A gate that requires a clickthrough or a written out message "I understand how dangerous this is" would do. More radically the commands could be removed altogether or hidden when a switch is not given at startup time.

Relevant discussions can be found in the logs for the development channel for half an hour ago.

[gwillen]

I am a strong advocate for making them type out the warning message, rather than letting them click through it. I think this is the only way to get them to actually read it.

I think the contents should be as explicit as possible, to reduce people's ability to convince themselves that they know better.

A lot of footgun issues happen when people import/export keys, use them for some operation, and then delete the wallet; I think a warning specifically advising that "DELETING ANY WALLET, EVEN AN APPARENTLY EMPTY WALLET, IS EXTREMELY DANGEROUS AND LIKELY TO CAUSE LOSS OF COINS" could potentially help.

(But not every footgun issue involves deleting a perceived-to-be-empty wallet; there's also the issue of exporting a privkey and handing it to an untrusted party. And any tutorial that advises doing that is probably malicious rather than stupid, which means any gate we add, the tutorial will explicitly instruct the user to bypass or ignore. So that's a harder issue than the one above.)

[luke-jr]

Clickthrough only makes sense for stuff exposed to end users, which these things are not. Perhaps a -dangerouswalletinternals flag required to make them available?

[laanwj]

I'd accept a warning message before allowing some RPC commands like importprivkey, dumpprikey. This message should appear only once - QSettings can be used to store a flag that it has been shown.

I don't mind if it has to be typed out or clicked through, though don't make it too patronizing/childish. A command-line option could work too but many people especially on windows don't know how to use them.

A more extreme option would be to deprecate those RPCs and eventually remove them. There is now a dumpwallet/importwallet that can be used to import or export a whole wallet at once which avoids many of the disaster scenarios that happen with keys.

[andronoob]

Clickthrough only makes sense for stuff exposed to end users, which these things are not.

Unfortunately, it's already exposed to end users, just as what @survic had said in the beginning:

Online tutorials encourage users to use the importprivkey and dumpprivkey commands in general situations

I agree that this should not happen, however, it's fait accompli.

So far, there's already a warning message about scammers. Maybe this message could be extended.

[adam2k]

It seems like this issue is pretty stale. I'm willing to attempt to take this on if nobody is working on it.

In the linked issue #8544 there is already a console warning in place merged in #9330

To extend this further, the proposed idea:

Clickthrough only makes sense for stuff exposed to end users, which these things are not. Perhaps a -dangerouswalletinternals flag required to make them available?

Seems reasonable to me. Can I get feedback on whether or not this issue is worth picking up from anybody working on the Wallet regularly?

[aureleoules]

I think this would be useful. listdescriptors with private keys and importdescriptors are also impacted.
Maybe show a warning before showing private keys and require the user to pass an -agree option to run the command?

[adam2k]

Thanks @aureleoules I can pick this up in the next few days. I'll add you as a reviewer when it's ready.

[ryanofsky]

require the user to pass an -agree option to run the command?

Just a suggestion, but maybe call it something like -danger to make it obvious in a log or script this command is worth paying more attention to.

[adam2k]

@ryanofsky and @aureleoules (and anybody else that is interested) do you mind taking a look here?

I made a small PoC for the high level implementation. After talking with @josibake it seems like we would need to implement this change at the RPC level and then within each dangerous request and abort the command if this header/flag (for the CLI) doesn't exist.

https://github.com/adam2k/bitcoin/tree/4176-privkey-warning

In the subsequent commits, I was planning on implementing:

1. The addition of this cURL header when using the -allowdangerous flag within the CLI.
2. Checks within the importprivkey, dumpprivkey, listdescriptors and importdescriptors for if (!allowDangerous) // abort the command

If this seems like a decent plan then I'll continue, but let me know if this is way off base.

[sipa]

@adam2k I don't think such protections belong in the RPC interface; it may be useful to have them in bitcoin-cli though, on the client side.

[adam2k]

@sipa thanks for the feedback! 🙏

What is your thought process behind not protecting the RPC interface? Why would we want people to be able to cURL without any error message in this situation?

I was thinking if this header is generic enough we could use for other use cases too.

[sipa]

My view is that the RPC interface is intended for other software, not humans. And software that needs to do "dangerous" things will just set the danger flag, always. The only effect is adding one step to the development of such software.

The user interfaces we provide (and expect) that use the RPC interface are the GUI and bitcoin-cli. That's where this protection belongs.

[josibake]

My view is that the RPC interface is intended for other software, not humans. And software that needs to do "dangerous" things will just set the danger flag, always. The only effect is adding one step to the development of such software.

I would argue an RPC is either dangerous or not, regardless of who calls it. If an RPC is dangerous, I think that logic should live in the RPC itself (not a client) and be applied regardless of the caller. By doing this through the RPC interface, we are communicating to software writers, bitcoin-cli users, users writing cURL commands, and any future client that these RPCs are dangerous and extra care should be taken. I don't think there is much difference between requiring a user to add an extra flag vs adding an extra step to software development. The goal is to communicate "here be dragons."

Secondly, I think modeling the logic in the RPC interface provides a cleaner way of communicating to all clients "hey, in this new release, we are marking X RPC as dangerous and potentially deprecating it in the future." If we instead model this logic in one specific client (bitcoin-cli), there are likely many other clients who would benefit from knowing they are using dangerous RPCs but would be unaware.

If the worry is we will create a bunch of extra work for software already calling these dangerous RPCs, we could add a config option to bitcoind to bypass danger warnings (-allowdangerousrpcs) which is off by default. This at least requires the node operator to explicitly state they are fine with clients doing dangerous stuff.

[adam2k]

Ah, yes. Thanks @josibake! I forgot to add the part about the config override in the previous comment. It's a little extra work for the 3rd party software developers, but that'll be a lot easier than adding the flag to every one of these "dangerous" requests.

@sipa I understand the annoyance for the developers of the other software. This is the safest approach though if we are trying to prevent a footgun situation (for example, image software logging dumpprivkey in a CI pipeline in a 3rd party solution like Datadog. They would not be willing to delete logs like this.) and because this is for legacy wallets I would think something like this is understandable by 3rd party developers.

I'm going to proceed as planned and I'll open up a PR this week for further comments.

[ryanofsky]

I think I agree with sipa in spirit, that CLI and GUI interfaces are for end users and RPC interface is for developers, and that we should avoid putting obstacles in the RPC interface to implement things that we think only help the CLI and GUI interfaces.

But in this case, I think josibake's statement that an "RPC is either dangerous or not, regardless of who calls it" is more applicable. Requiring a -danger true argument for operations that are potentially dangerous, and should be given a second look, would be beneficial for both developers and users. For example if you are developer, you probably do code reviews, and you probably should spend a little extra time looking at an rpc call with a danger=True argument.

I also think an explicit argument is better than an HTTP header for this reason, so the danger flag is directly part of the call and not something that is likely to be set some other place and then forgotten about.

[adam2k]

I think I agree with sipa in spirit, that CLI and GUI interfaces are for end users and RPC interface is for developers, and that we should avoid putting obstacles in the RPC interface to implement things that we think only help the CLI and GUI interfaces.

But in this case, I think josibake's statement that an "RPC is either dangerous or not, regardless of who calls it" is more applicable. Requiring a -danger true argument for operations that are potentially dangerous, and should be given a second look, would be beneficial for both developers and users. For example if you are developer, you probably do code reviews, and you probably should spend a little extra time looking at an rpc call with a danger=True argument.

I also think an explicit argument is better than an HTTP header for this reason, so the danger flag is directly part of the call and not something that is likely to be set some other place and then forgotten about.

@ryanofsky to clarify, are you thinking that the argument should be part of the RPC params here and not in a header? https://github.com/bitcoin/bitcoin/blob/master/src/rpc/request.h#L31-L38

[ryanofsky]

@ryanofsky to clarify, are you thinking that the argument should be part of the RPC params here and not in a header? https://github.com/bitcoin/bitcoin/blob/master/src/rpc/request.h#L31-L38

Yes I think a named parameter or entry in an options object would be good, and a header would be bad. This is just my opinion, though and I think other choices are reasonable. Would also acknowledge that using a name parameter might seem unappealing right now because support for using named parameters is not very good, though that could be addressed separately with PRs like #19762

[adam2k]

@ryanofsky to clarify, are you thinking that the argument should be part of the RPC params here and not in a header? https://github.com/bitcoin/bitcoin/blob/master/src/rpc/request.h#L31-L38

Yes I think a named parameter or entry in an options object would be good, and a header would be bad. This is just my opinion, though and I think other choices are reasonable. Would also acknowledge that using a name parameter might seem unappealing right now because support for using named parameters is not very good, though that could be addressed separately with PRs like #19762

@ryanofsky I went through your PR and tried applying the same concepts to this issue. It feels a little brittle because of the ordering of the params seems to be required for this to work. For example, if the command looks something like:

Case 1:

$ bitcoin-cli dumpprivkey "${SOME_KEY}" dangerous

...

// From my debugger:
(const std::vector<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > &) args = size=2: {
  [0] = "${OMITTED_KEY}"
  [1] = "dangerous"
}

vs.

Case 2:

$ bitcoin-cli dumpprivkey dangerous "${SOME_KEY}"

...

// From my debugger:

(const std::vector<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > &) args = size=2: {
  [0] = "dangerous"
  [1] = "${OMITTED_KEY}"
}

The two commands have different array locations and in the second case, I believe this would fail because dangerous is not a valid private key. Where this seems to break down for me is that we need to enforce this on a command by command basis. So all of the requests are need this dangerous flag might have varied vector locations to check. It seems like this would add a lot of cruft to the codebase unnecessarily. Am I missing something that makes the management of the params easier?

Maybe the answer is we go with this idea as-is and require some long term follow up work to overhaul the architecture of the CLI at some point to clean up these rough edges?

[ryanofsky]

@ryanofsky I went through your PR and tried applying the same concepts to this issue. It feels a little brittle because of the ordering of the params seems to be required for this to work.

Sorry, I was trying to suggest passing it as a named-parameter like bitcoin-cli -named dumpprivkey danger=true instead of as a positional argument. I agree positional arguments can be brittle. #26485 would improve on #19762 in this respect because it could require the argument to be passed by name, not position.

[pinheadmz]

This issue is unlikely to be fixed in Bitcoin Core. We'll close for now, but feel free to open another issue or pull request with a fix.