From 62c0aa9e58eb0dd9a22a2a83fd01d854d575a184 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 18 Oct 2016 17:47:17 -0500 Subject: [PATCH 1/7] Disable wallet encryption Closes #1552 --- src/wallet/rpcwallet.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4d6608570..eb76e667d 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1979,10 +1979,18 @@ Value encryptwallet(const Array& params, bool fHelp) { if (!EnsureWalletIsAvailable(fHelp)) return Value::null; - + + auto fEnableWalletEncryption = GetBoolArg("-developerencryptwallet", false); + + std::string strWalletEncryptionDisabledMsg = ""; + if (!fEnableWalletEncryption) { + strWalletEncryptionDisabledMsg = "\nWARNING: Wallet encryption is DISABLED. This call does nothing.\n"; + } + if (!pwalletMain->IsCrypted() && (fHelp || params.size() != 1)) throw runtime_error( "encryptwallet \"passphrase\"\n" + + strWalletEncryptionDisabledMsg + "\nEncrypts the wallet with 'passphrase'. This is for first time encryption.\n" "After this, any calls that interact with private keys such as sending or signing \n" "will require the passphrase to be set prior the making these calls.\n" @@ -2008,6 +2016,9 @@ Value encryptwallet(const Array& params, bool fHelp) if (fHelp) return true; + if (!fEnableWalletEncryption) { + return false; + } if (pwalletMain->IsCrypted()) throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an encrypted wallet, but encryptwallet was called."); From 1532cb75f36a3e79a3ee324eb83217922a9bd822 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 20 Oct 2016 12:58:20 -0500 Subject: [PATCH 2/7] Throw an error when encryptwallet is disabled --- src/wallet/rpcwallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index eb76e667d..e31619475 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1984,7 +1984,7 @@ Value encryptwallet(const Array& params, bool fHelp) std::string strWalletEncryptionDisabledMsg = ""; if (!fEnableWalletEncryption) { - strWalletEncryptionDisabledMsg = "\nWARNING: Wallet encryption is DISABLED. This call does nothing.\n"; + strWalletEncryptionDisabledMsg = "\nWARNING: Wallet encryption is DISABLED. This call always fails.\n"; } if (!pwalletMain->IsCrypted() && (fHelp || params.size() != 1)) @@ -2017,7 +2017,7 @@ Value encryptwallet(const Array& params, bool fHelp) if (fHelp) return true; if (!fEnableWalletEncryption) { - return false; + throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: wallet encryption is disabled."); } if (pwalletMain->IsCrypted()) throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an encrypted wallet, but encryptwallet was called."); From 750d4e07dd4379d0d1a7e3a56b2997401bdbfa7e Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 20 Oct 2016 12:58:51 -0500 Subject: [PATCH 3/7] Document that wallet encryption is disabled --- doc/security-warnings.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/doc/security-warnings.md b/doc/security-warnings.md index c5c340854..a4a4060a3 100644 --- a/doc/security-warnings.md +++ b/doc/security-warnings.md @@ -14,6 +14,25 @@ make proving keys generated on 64-bit systems unusable on 32-bit and big-endian systems. It's unclear if a warning will be issued in this case, or if the proving system will be silently compromised. +Wallet Encryption +----------------- + +Wallet encryption is disabled, for several reasons: + +- Encrypted wallets are unable to correctly detect shielded spends (due to the + nature of unlinkability of JoinSplits) and will incorrectly show much larger + available shielded balances until the next time the wallet is unlocked. + +- While encrypted wallets prevent spending of funds, they do not maintain the + shielding properties of JoinSplits (due to the need to detect spends). That + is, someone with access to an encrypted wallet.dat has full visibility of + your entire transaction graph (other than newly-detected spends, which suffer + from the earlier issue). + +You should use full-disk encryption (or encryption of your home directory) to +protect your wallet at rest, and should assume (even unprivileged) users who are +runnng on your OS can read your wallet.dat file. + Side-Channel Attacks -------------------- From 35cf6ee26d36cb419b4066463209cabd7a2b1ba6 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 20 Oct 2016 13:33:58 -0500 Subject: [PATCH 4/7] Document another wallet encryption concern --- doc/security-warnings.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/doc/security-warnings.md b/doc/security-warnings.md index a4a4060a3..b610364d6 100644 --- a/doc/security-warnings.md +++ b/doc/security-warnings.md @@ -29,6 +29,12 @@ Wallet encryption is disabled, for several reasons: your entire transaction graph (other than newly-detected spends, which suffer from the earlier issue). +- We were concerned about the resistance of the algorithm used to derive wallet + encryption keys (inherited from Bitcoin) to dictionary attacks by a powerful + attacker. If and when we re-enable wallet encryption, it is likely to be with + a modern passphrase-based key derivation algorithm designed for greater + resistance to dictionary attack, such as Argon2i. + You should use full-disk encryption (or encryption of your home directory) to protect your wallet at rest, and should assume (even unprivileged) users who are runnng on your OS can read your wallet.dat file. From 8ecf6ccfefff0877ec20725313308ca2d5a739ea Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 20 Oct 2016 13:41:00 -0500 Subject: [PATCH 5/7] Improve security documentation --- doc/security-warnings.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/doc/security-warnings.md b/doc/security-warnings.md index b610364d6..2e40e92fe 100644 --- a/doc/security-warnings.md +++ b/doc/security-warnings.md @@ -20,8 +20,11 @@ Wallet Encryption Wallet encryption is disabled, for several reasons: - Encrypted wallets are unable to correctly detect shielded spends (due to the - nature of unlinkability of JoinSplits) and will incorrectly show much larger - available shielded balances until the next time the wallet is unlocked. + nature of unlinkability of JoinSplits) and can incorrectly show larger + available shielded balances until the next time the wallet is unlocked. This + problem was not limited to failing to recognize the spend; it was possible for + the shown balance to increase by the amount of change from a spend, without + deducting the spent amount. - While encrypted wallets prevent spending of funds, they do not maintain the shielding properties of JoinSplits (due to the need to detect spends). That From 2b499f4386d458c32b64ebb2e7554d042e787f88 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 20 Oct 2016 14:35:41 -0500 Subject: [PATCH 6/7] Fix RPC tests that require wallet encryption --- qa/rpc-tests/keypool.py | 2 +- qa/rpc-tests/wallet_nullifiers.py | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/qa/rpc-tests/keypool.py b/qa/rpc-tests/keypool.py index aee29a596..1f8979692 100755 --- a/qa/rpc-tests/keypool.py +++ b/qa/rpc-tests/keypool.py @@ -98,7 +98,7 @@ def main(): os.makedirs(options.tmpdir) initialize_chain(options.tmpdir) - nodes = start_nodes(1, options.tmpdir) + nodes = start_nodes(1, options.tmpdir, extra_args=[['-developerencryptwallet']]) run_test(nodes, options.tmpdir) diff --git a/qa/rpc-tests/wallet_nullifiers.py b/qa/rpc-tests/wallet_nullifiers.py index 7efd70f6a..6d7449138 100755 --- a/qa/rpc-tests/wallet_nullifiers.py +++ b/qa/rpc-tests/wallet_nullifiers.py @@ -10,6 +10,10 @@ from time import * class WalletNullifiersTest (BitcoinTestFramework): + def setup_nodes(self): + return start_nodes(4, self.options.tmpdir, + extra_args=[['-developerencryptwallet']] * 4) + def run_test (self): # add zaddr to node 0 myzaddr0 = self.nodes[0].z_getnewaddress() From d8e06e3f580f799c2774807ac98a5d91890cc1e3 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 20 Oct 2016 14:45:44 -0500 Subject: [PATCH 7/7] Add test that encryptwallet is disabled --- src/test/rpc_wallet_tests.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/rpc_wallet_tests.cpp b/src/test/rpc_wallet_tests.cpp index 68883f2f8..2f674df0f 100644 --- a/src/test/rpc_wallet_tests.cpp +++ b/src/test/rpc_wallet_tests.cpp @@ -1090,6 +1090,9 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_encrypted_wallet_zkeys) Array arr = retValue.get_array(); BOOST_CHECK(arr.size() == n); + // Verify that the wallet encryption RPC is disabled + BOOST_CHECK_THROW(CallRPC("encryptwallet passphrase"), runtime_error); + // Encrypt the wallet (we can't call RPC encryptwallet as that shuts down node) SecureString strWalletPass; strWalletPass.reserve(100);