Do not import private keys to wallets with private keys disabled

achow101 commented at 6:42 am on January 23, 2019: member

Fixes a bug where private keys could be imported to wallets with private keys disabled. Now every RPC which can import private keys checks for whether the wallet has private keys are disabled and errors if it is. Also added an belt-and-suspenders check to AddKeyPubkeyWithDB to have it assert that the wallet has private keys enabled.

fanquake added the label Wallet on Jan 23, 2019

DrahtBot commented at 8:38 am on January 23, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15226 (Allow creating blank (empty) wallets (alternative) by achow101)
#15093 (rpc: Change importwallet to return additional errors by marcinja)
#15032 (Nit fixes for PR 14565 by MeshCollider)
#15024 (Allow specific private keys to be derived from descriptor by MeshCollider)
#15006 (Add option to create an encrypted wallet by achow101)
#14491 (Allow descriptor imports with importmulti by MeshCollider)
#14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)
#14021 (Import key origin data through descriptors in importmulti by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/wallet/rpcdump.cpp:557 in ab677edbd8 outdated

551@@ -549,6 +552,10 @@ UniValue importwallet(const JSONRPCRequest& request)
552             + HelpExampleRpc("importwallet", "\"test\"")
553         );
554 
555+    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
556+        throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled when private keys are disabled");

Sjors commented at 10:54 am on January 23, 2019:

Add todo that this should permit importing a wallet that also has private keys disabled? (I assume currently that information isn’t stored in the dump)

achow101 commented at 4:02 am on January 24, 2019:

Done

laanwj commented at 9:39 am on January 24, 2019:

Please open an issue for this as well, a TODO in the source code hasn’t historically turned out very effective.

Sjors commented at 11:05 am on January 25, 2019:

@laanwj wrote:

Please open an issue for this as well, a TODO in the source code hasn’t historically turned out very effective.

Although it says “todo”, I see it more as an explanation that this is strictly speaking a bug, and that we should pay attention to this if we move to descriptor based wallets and (presumably) overhaul the dump and import commands.

Sjors commented at 11:10 am on January 23, 2019: member

Concept ACK. Travis is unhappy. On my own machine test/functional/wallet_disableprivatekeys.py failed when it was part of the full test suite, but not when run individually. Odd…

sethdseed should also have this check, since it currently throws a confusing “Cannot set a HD seed on a non-HD wallet.”

Maybe add the same check to addmultisigaddress, since the help says “This functionality is only intended for use with non-watchonly addresses.”?

ab677ed looks good otherwise

Suggest tagging this for v0.18 and 0.17 backport.

By the way, we never elaborated on disableprivatekey in the release notes.

achow101 commented at 8:36 pm on January 23, 2019: member

The test failure is running wallet_disableprivatekeys.py with --usecli. It seems that bitcoin-cli is erroring, giving:

error: Error parsing JSON:[{'timestamp': 'now', 'scriptPubKey': {'address': 'n2xmfzEaZi2XyjFYCXxfMbUHyKGk2PaL2k'}, 'keys': ['cQdMcowhL91CXuSRs7FSaujMPNArV8dLZcjq16kHDSDgNaAVTB99']}]

(tested on my own machine with some debugging changes)

Sjors commented at 8:42 pm on January 23, 2019: member

So that’s the importmulti call? Check if CRPCConvertParam in src/rpc/client.cpp contains all the non-string params; I think there’s a bunch of RPC calls where that’s not up to date.

achow101 force-pushed on Jan 23, 2019

achow101 commented at 8:53 pm on January 23, 2019: member

I fixed the test failure. It’s because --usecli did not properly handle arguments that are dicts and lists.

jonasschnelli commented at 3:27 am on January 24, 2019: contributor

utACK d8ac347a798e65f3a1808c7767f886a4626a0471 Somehow this was missed in #9662.

IMO this is a bugfix and should be backported.

jonasschnelli added the label Needs backport on Jan 24, 2019

achow101 force-pushed on Jan 24, 2019

achow101 commented at 4:01 am on January 24, 2019: member

sethdseed should also have this check, since it currently throws a confusing “Cannot set a HD seed on a non-HD wallet.”

Done

Maybe add the same check to addmultisigaddress, since the help says “This functionality is only intended for use with non-watchonly addresses.”?

I don’t think that’s necessary. IMO we should only have this warning for anything where private keys can be imported.

(Missed these in the earlier push)

in test/functional/test_framework/test_node.py:437 in 2fa2a444c7 outdated

432@@ -433,8 +433,8 @@ def batch(self, requests):
433 
434     def send_cli(self, command=None, *args, **kwargs):
435         """Run bitcoin-cli command. Deserializes returned string as python object."""
436-        pos_args = [str(arg).lower() if type(arg) is bool else str(arg) for arg in args]
437-        named_args = [str(key) + "=" + str(value) for (key, value) in kwargs.items()]
438+        pos_args = [str(arg).lower() if type(arg) is bool else (json.dumps(arg) if (isinstance(arg, dict) or isinstance(arg, list)) else str(arg)) for arg in args]
439+        named_args = [str(key) + "=" + (json.dumps(value) if (isinstance(value, dict) or isinstance(value, list)) else str(value)) for (key, value) in kwargs.items()]

Sjors commented at 7:46 am on January 24, 2019:

paging @jnewbery, looks ok?

laanwj commented at 10:33 am on January 24, 2019:

Might want to use named function, say value_to_cli, here for clarity, and avoid repetition; this duplicated expression keeps getting longer

Edit: I also think it’s wrong that booleans are handled differently for pos and named args.

jnewbery commented at 4:50 pm on January 24, 2019:

Definitely agree with pulling this monstrosity out into a function :)

bools in named args were fixed here: #14938 (review) but that PR wasn’t merged.

achow101 commented at 6:47 pm on January 24, 2019:

I’ve pulled this into a new function which is easier to read. It also fixes the bools in named args issue.

achow101 commented at 3:38 pm on January 31, 2019:

Since this issue affects multiple PRs, I’ve split it into a separate PR: #15301

Sjors approved

Sjors commented at 7:52 am on January 24, 2019: member

tACK 2fa2a44

achow101 force-pushed on Jan 24, 2019

tests: unify RPC argument to cli argument conversion and handle dicts and lists

When running tests with --usecli, unify the conversion from argument objects to
strings using a new function arg_to_cli(). This fixes boolean arguments when
using named arguments.

Also use json.dumps() to get the string values for arguments that are dicts and
lists so that bitcoind's JSON parser does not become confused.

1f77f6754c

achow101 force-pushed on Jan 24, 2019

gmaxwell commented at 7:11 pm on January 24, 2019: contributor

Concept ACK. (maybe we should have a warning that prints in the debug log if you load a non-private-key wallet with private keys in it, like we do for overlong multisig.)

MarcoFalke added this to the milestone 0.17.2 on Jan 24, 2019

Sjors approved

Sjors commented at 11:08 am on January 25, 2019: member

re-tACK 7b70a14 on macOS (without --enable-debug, because that will crash until the next rebase)

achow101 force-pushed on Jan 25, 2019

achow101 commented at 7:46 pm on January 25, 2019: member

I’ve gotten rid of the todo in importwallet and just implemented it. The latest push refactors importwallet so that the import happens in two steps. First the import file is read and the keys and scripts extracted. Then the check for private keys being imported and private keys disabled is done. Then it imports. This will avoid importing wallet dump file that have any private keys and ensure that nothing is imported at all if the file has private keys.

in src/wallet/rpcdump.cpp:630 in e6c9c61f04 outdated

641+                int64_t birth_time = DecodeDumpTime(vstr[1]);
642+                scripts.push_back(std::pair<CScript, int64_t>(script, birth_time));
643             }
644         }
645         file.close();
646+        for (unsigned int i = 0; i < keys.size(); ++i) {

Sjors commented at 4:50 pm on January 31, 2019:

nit: calculate total = keys.size() + scripts.size() before the loop, then set progress=0

0for ( const auto& key_tuple : keys ) {
1... ShowProgress(... progress / total * 100)
2...
3progress++

achow101 commented at 6:07 pm on January 31, 2019:

Done

Sjors commented at 6:56 pm on January 31, 2019:

re-nit: still missing the for ( const auto& key_tuple : keys ) bit :-)

achow101 commented at 7:29 pm on January 31, 2019:

Oops fixed

in src/wallet/rpcdump.cpp:671 in e6c9c61f04 outdated

683+                fGood = false;
684+                continue;
685+            }
686+            if (time > 0) {
687+                pwallet->m_script_metadata[id].nCreateTime = time;
688+                nTimeBegin = std::min(nTimeBegin, time);

Sjors commented at 5:11 pm on January 31, 2019:

Shouldn’t this be birthtime? int64_t birth_time = DecodeDumpTime(vstr[1]) (because all script timestamps are 0 in the dump). This line should be moved outside the loop probably.

achow101 commented at 6:03 pm on January 31, 2019:

No because birth_time just becomes time in this loop.

Sjors commented at 7:14 pm on January 31, 2019:

Ok, but then what’s causing the full rescan?

Just tried on HEAD~2 and getting the same thing, so I’m blaming my own wallet. Just tried this again with a fresh wallet that only has one transaction. Now the rescan is a modest 10623 blocks (I guess that’s a built in safety margin?).

I also got a ton of Skipping import of ... (script already present) messages both my original weird wallet and on the fresh one. That doesn’t make sense to me, because I’m importing into a brand new wallet. But it’s not new behavior.

achow101 commented at 7:43 pm on January 31, 2019:

I’m not sure what might be causing that problem. However the Skipping import messages for scripts is expected since segwit scripts are in the dump, but already automatically imported when the keys were imported.

Sjors commented at 5:12 pm on January 31, 2019: member

I’m not sure if d9d6bed is working correctly, or if I’m seeing an already existing bug in dumpwallet:

When I dump a wallet that I created a few months ago, the oldest key in the dump file is 2018-08-01T14:51:24Z, but all the scripts have 0 in the time column. If I then import that into a fresh wallet, it triggers a rescan from genesis.

The line if (time > 0) should prevent nTimeBegin from becoming 0 this way, but see my comment where I think this goes wrong.

achow101 force-pushed on Jan 31, 2019

Sjors commented at 7:16 pm on January 31, 2019: member

tACK 2911ef603

Refactor importwallet to extract data from the file and then import

Instead of importing keys and scripts as each line in the file is
read, first extract the data then import them.

b5c5021b64

Do not import private keys to wallets with private keys disabled e6c58d3b01

achow101 force-pushed on Jan 31, 2019

Sjors commented at 8:49 pm on January 31, 2019: member

re-tACK 2911ef6

laanwj merged this on Feb 1, 2019

laanwj closed this on Feb 1, 2019

laanwj referenced this in commit 3e38d40873 on Feb 1, 2019

fanquake removed the label Needs backport on Sep 19, 2019

fanquake removed this from the milestone 0.17.2 on Sep 19, 2019

pravblockc referenced this in commit eb17a31f8e on Aug 13, 2021

pravblockc referenced this in commit 55761c479d on Aug 13, 2021

pravblockc referenced this in commit d9e13701d7 on Aug 14, 2021

pravblockc referenced this in commit 866c2c29f4 on Aug 17, 2021

pravblockc referenced this in commit ac01dbee63 on Aug 17, 2021

DrahtBot locked this on Dec 16, 2021

Do not import private keys to wallets with private keys disabled #15235

Conflicts