Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unreliable file systems may lead to wallet file corruption during wallet creation #5082

Closed
SomberNight opened this Issue Feb 6, 2019 · 4 comments

Comments

Projects
None yet
3 participants
@SomberNight
Copy link
Member

SomberNight commented Feb 6, 2019

@ecdsa has been recently looking into #4891, and in more general, ways a user could end up with a corrupted wallet file, where the addresses in the file do not correspond to the xpub.

A potential explanation that might cause this is if os.path.exist "randomly" fails. This is not completely unrealistic if your wallet files are saved on a USB flash drive or a network drive (or similar). Or perhaps on shitty file systems.


os.path.exists is called multiple times during wallet creation. The idea is that a user can end up with a corrupted wallet if he ends up partially overwriting an existing file.

When creating a new wallet, the path would be set to the path of an existing wallet, as due to os.path.exists failing, the wizard would think there is no file there yet. However in the wallet storage constructor, os.path.exists would suddenly work and the existing file would get opened. The wizard would then start modifying the existing file, thinking it's a new one. The wizard would put a new seed and xpub there. Then, the task that creates the addresses for the xpub (create_addresses) would be a no-op, as it would find that there are already "enough" addresses generated in the file (corresponding to the old wallet!).

When the wizard finished, the wallet would then get opened as normally. The wallet file would contain the new seed, new xpub, but old addresses.

Note that while this still can happen on Electrum 3.3.x, it would immediately get recognised, and the user would get several warnings due to the sanity checks added in #4828.


See issues:
#4949 (TARS)
#4891
#5066
#4790
#4462
https://bitcoin.stackexchange.com/questions/78813/same-seed-from-electrum-but-generate-different-bitcoin-addresses


Patch on top of 3.1.3 to trigger the corruption.

diff --git a/gui/qt/installwizard.py b/gui/qt/installwizard.py
index 8ccc0c21b..5f8af70d5 100644
--- a/gui/qt/installwizard.py
+++ b/gui/qt/installwizard.py
@@ -193,7 +193,7 @@ class InstallWizard(QDialog, MessageBoxMixin, BaseWizard):
                 self.storage = None
                 self.next_button.setEnabled(False)
             if self.storage:
-                if not self.storage.file_exists():
+                if True:  #not self.storage.file_exists():
                     msg =_("This file does not exist.") + '\n' \
                           + _("Press 'Next' to create this wallet, or choose another file.")
                     pw = False
@@ -293,7 +293,7 @@ class InstallWizard(QDialog, MessageBoxMixin, BaseWizard):
             self.wallet = Wallet(self.storage)
             return self.wallet
 
-        action = self.storage.get_action()
+        action = 'new'  # self.storage.get_action()
         if action and action != 'new':
             self.hide()
             msg = _("The file '{}' contains an incompletely created wallet.\n"
@SomberNight

This comment has been minimized.

Copy link
Member Author

SomberNight commented Feb 6, 2019

If this happened to someone, and they still have the original seed (corresponding to the old existing wallet file), they should restore from that seed and can recover the coins.

@ecdsa

This comment has been minimized.

Copy link
Member

ecdsa commented Feb 7, 2019

I have found another execution path to trigger that bug, that does not involve an unreliable file system.

  1. user creates default_wallet
  2. user creates another wallet, named wallet_1
  3. close window for wallet_1
  4. close window for default_wallet; now electrum will quit
  5. user restarts electrum; default_wallet gets opened
  6. user opens "File->New/restore"
  7. wizard suggests name: "wallet_2"
  8. user wants to create a wallet with the name "wallet_1", but electrum tells him that the file already exists
  9. user deletes wallet_1 from outside Electrum
  10. user clicks "next", the wizard is launched and a new seed is attributed to wallet_1

This will result in wallet_1 having the new seed and the old addresses

@kyuupichan

This comment has been minimized.

Copy link
Collaborator

kyuupichan commented Feb 7, 2019

@ecdsa in your scenario what results in wallet_1 retaining the old addresses?

cculianu added a commit to Electron-Cash/Electron-Cash that referenced this issue Feb 7, 2019

cculianu added a commit to clifordsymack/Electron-Cash that referenced this issue Feb 7, 2019

cculianu added a commit to simpleledger/Electron-Cash-SLP that referenced this issue Feb 7, 2019

@SomberNight

This comment has been minimized.

Copy link
Member Author

SomberNight commented Feb 7, 2019

@kyuupichan when the user changes the text in the TextEdit, wizard.storage gets set with an instantiated WalletStorage

self.storage = WalletStorage(path, manual_upgrades=True)

then the user deletes the actual file on the file system in another process;
but the WalletStorage object still exists in memory for the deleted file

User will not change the text in the TextEdit anymore.

The user clicks next, and the control flow gets here

action = self.storage.get_action()
if action and action != 'new':

electrum/lib/storage.py

Lines 577 to 582 in 624fa47

def get_action(self):
action = run_hook('get_action', self)
if action:
return action
if not self.file_exists():
return 'new'

At that point, the new wallet creation flow will start, with the existing WalletStorage object still in memory as wizard.storage, containing the "deleted" wallet.
The wizard will overwrite some parts of this storage, but not all, and in the end, the file will have the new seed but the old addresses.

cculianu added a commit to Electron-Cash/Electron-Cash that referenced this issue Feb 8, 2019

[Lib][Qt] Pulled some minor tweaks from upstream to wallet storage
Deleting a wallet from outside the app while it's running could cause
weirdness (see: spesmilo#4984 and
spesmilo#5082). Follow ups to fix
that.

cculianu added a commit to clifordsymack/Electron-Cash that referenced this issue Feb 8, 2019

[Lib][Qt] Pulled some minor tweaks from upstream to wallet storage
Deleting a wallet from outside the app while it's running could cause
weirdness (see: spesmilo#4984 and
spesmilo#5082). Follow ups to fix
that.

cculianu added a commit to simpleledger/Electron-Cash-SLP that referenced this issue Feb 8, 2019

[Lib][Qt] Pulled some minor tweaks from upstream to wallet storage
Deleting a wallet from outside the app while it's running could cause
weirdness (see: spesmilo#4984 and
spesmilo#5082). Follow ups to fix
that.

a-bezrukov added a commit to zcoinofficial/electrum-xzc that referenced this issue Feb 14, 2019

icodeface added a commit to qtumproject/qtum-electrum that referenced this issue Feb 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.