encrypting wallet reuses keys that have been written to disk in the clear

dooglus commented at 5:08 pm on July 20, 2016: contributor

I don’t know if that has been discussed already, but I just noticed when testing the 0.13 rc1 that when I first create a wallet, the first 100 private keys are written to the wallet, then when I encrypt the wallet with a passphrase the same 100 private keys are re-used even though they have already been written unencrypted to the disk.

In previous releases we were careful when encrypting the wallet to discard any keys that had been written unencrypted, so it seems like we may be introducing a security hole with this HD wallet change.

Edit: I’m sorry, none of that is true. The keypool is discarded upon encryption, but the same HD seed is used after encryption.

Maybe the best way to fix this would be to create a new HD seed when encrypting the wallet. Because the HD seed itself has been written to disk when the wallet was created, and so we cannot safely continue using it once the wallet is encrypted.

jonasschnelli added the label Wallet on Jul 20, 2016

jonasschnelli commented at 6:38 pm on July 20, 2016: contributor

Thanks @dooglus for reporting. I just double-checked and right – as you have edited – the keypool gets discarded once you encrypt the wallet.

But you are right. The seed key will be kept which can be problematic. Generating a new seed once the user encrypt could be a solution, though I think it could be unexpected from the user perspective (it would invalidate his old backup probably done before he encrypted the wallet).

Other solutions? Add a boolean parameter to encryptwallet to force regeneration of the seed?

Maybe another solution would be to allow a startup argument -encryptwallet= which would directly encrypt wallets during the phase of creation. It could also encrypt already existing wallets.

dooglus commented at 8:29 pm on July 20, 2016: contributor

I think it could be unexpected from the user perspective (it would invalidate his old backup probably done before he encrypted the wallet).

I’m not familiar with the behavior of the new HD wallet code. Isn’t each derived private key stored in the wallet as it is derived? If so, changing the seed as we encrypt won’t invalidate old backups any more than encrypting already does in v0.12 and earlier. When we encrypt a wallet in v0.12 we invalidate the keypool that was written in the old backup, while maintaining the validity of the backup for private keys which had already been used at the time of the backup. Changing the HD seed should be the same as that, so long as the private keys that were already derived have been written to the wallet file.

It appears as if the intention of this BIP32 implementation is to make the change pretty much invisible to the user. But we have accidentally introduced a new security hole: it is now impossible (without disabling the HD feature) to create an encrypted wallet without the unencrypted HD seed being written to wallet.dat.

dooglus commented at 11:50 pm on July 20, 2016: contributor

I played around with the code to see what would happen if we changed the HD seed when the wallet is encrypted. It behaves how I would expect - the addresses which were used before encryption are still in the wallet after encryption, along with their private keys, so old unencrypted backups aren’t invalidated. Here’s the change I made:

 0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1index a76085d..cacf102 100644
 2--- a/src/wallet/wallet.cpp
 3+++ b/src/wallet/wallet.cpp
 4@@ -626,6 +626,14 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
 5
 6         Lock();
 7         Unlock(strWalletPassphrase);
 8+
 9+        // if we have a master key replace it with a new one
10+        if (!hdChain.masterKeyID.IsNull()) {
11+            CKey newMasterKey;
12+            newMasterKey.MakeNewKey(true);
13+            SetHDMasterKey(newMasterKey);
14+        }
15+
16         NewKeyPool();
17         Lock();

It is probably not enough, because I don’t know how to handle the case that SetHDMasterKey() throws an exception. But it’s enough to demonstrate that changing the HD seed on encryption doesn’t break old backups.

jonasschnelli commented at 7:50 am on July 21, 2016: contributor

The recent change to HD does allow users to regenerate every possible key as long as they have a first backup (regardless if it was done directly after wallet creation).

I think you change would makes sense, but, we need to inform the user that once they encrypt the wallet, old backup would no longer be capable of recreating future keys.

dooglus commented at 3:49 pm on July 21, 2016: contributor

The recent change to HD does allow users to regenerate every possible key as long as they have a first backup

It does, but it also invalidates encryption, since it likely leaves an unencrypted copy of their HD seed on disk somewhere.

we need to inform the user that once they encrypt the wallet, old backup would no longer be capable of recreating future keys

That has always been the case (except in the flawed 0.13rc1) and we should always have been warning about it.

sipa commented at 3:52 pm on July 21, 2016: member

I agree that encrypting should cause a change of deterministic seed. We should check that sufficient warning is given in the UI about backups being invalidated.

I think the warning requirement is stronger now. Before, users always had to backup their wallets at least every keypool receive addresses. HD support is however switching to a model where continuous backups are no longer necessarily required, so it may become less expected that encrypting invalidates them nonetheless.

laanwj added this to the milestone 0.13.0 on Jul 21, 2016

laanwj commented at 5:41 pm on July 21, 2016: member

Seems like an issue that should be solved before 0.13 final - to keep using the master key that is stored in plain breaks with how we’ve always done things. It is a fatal leak.

I think you change would makes sense, but, we need to inform the user that once they encrypt the wallet, old backup would no longer be capable of recreating future keys.

Don’t we warn of this yet when encrypting?

MarcoFalke commented at 5:50 pm on July 21, 2016: member

I could imagine some people want their wallet encrypted but keep the HD seed they generated initially. We should not make this impossible.

I think @jonasschnelli mentioned that the encryptwallet rpc could take a bool parameter to specify if the existing HD seed should be discarded. This needs to be implemented in qt as well, imo.

jonasschnelli commented at 6:15 pm on July 21, 2016: contributor

Working on a simple fix now which should be ready in a couple of hours.

laanwj commented at 12:06 pm on July 25, 2016: member

I could imagine some people want their wallet encrypted but keep the HD seed they generated initially. We should not make this impossible.

I agree, no need to make it impossible. Advanced users may have their own reasons for opting to keep using the same seed. But for added security, the default should be to generate a new one.

laanwj commented at 11:14 am on July 28, 2016: member

Closed by #8389

laanwj closed this on Jul 28, 2016

MarcoFalke commented at 1:51 pm on July 31, 2016: member

I agree, no need to make it impossible. Advanced users may have their own reasons for opting to keep using the same seed. But for added security, the default should be to generate a new one.

Right now it seems, it was made impossible. Should this issue be reopened?

MarcoFalke removed this from the milestone 0.13.0 on Aug 4, 2016

MarcoFalke added the label Feature on Aug 4, 2016

MarcoFalke removed the label Feature on Aug 4, 2016

MarcoFalke added this to the milestone 0.13.0 on Aug 4, 2016

DrahtBot locked this on Sep 8, 2021

encrypting wallet reuses keys that have been written to disk in the clear #8383