storage: read/write sanity checks

related: #4110
supersedes: #4528

electrum/gui/kivy/main_window.py

@@ -7,10 +7,10 @@ import traceback
 from decimal import Decimal
 import threading
 import asyncio
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Optional
 
 from electrum.bitcoin import TYPE_ADDRESS
-from electrum.storage import WalletStorage
+from electrum.storage import WalletStorage, StorageReadWriteError
 from electrum.wallet import Wallet, InternalAddressCorruption
 from electrum.util import profiler, InvalidPassword, send_exception_to_crash_reporter
 from electrum.plugin import run_hook
@@ -79,7 +79,10 @@ from .uix.dialogs.lightning_open_channel import LightningOpenChannelDialog
 from .uix.dialogs.lightning_channels import LightningChannelsDialog
 
 if TYPE_CHECKING:
+    from . import ElectrumGui
     from electrum.simple_config import SimpleConfig
+    from electrum.wallet import Abstract_Wallet
+    from electrum.plugin import Plugins
 
 
 class ElectrumWindow(App):
@@ -309,7 +312,7 @@ class ElectrumWindow(App):
         self.nfcscanner = None
         self.tabs = None
         self.is_exit = False
-        self.wallet = None
+        self.wallet = None  # type: Optional[Abstract_Wallet]
         self.pause_time = 0
         self.asyncio_loop = asyncio.get_event_loop()
 
@@ -330,8 +333,8 @@ class ElectrumWindow(App):
             self.proxy_config = net_params.proxy if net_params.proxy else {}
             self.update_proxy_str(self.proxy_config)
 
-        self.plugins = kwargs.get('plugins', [])
-        self.gui_object = kwargs.get('gui_object', None)
+        self.plugins = kwargs.get('plugins', None)  # type: Plugins
+        self.gui_object = kwargs.get('gui_object', None)  # type: ElectrumGui
         self.daemon = self.gui_object.daemon
         self.fx = self.daemon.fx
 
@@ -548,6 +551,7 @@ class ElectrumWindow(App):
             self.network.register_callback(self.on_channel, ['channel'])
             self.network.register_callback(self.on_payment_status, ['payment_status'])
         # load wallet
+        # FIXME if this raises, the whole app quits, without any user feedback or exc reporting
         self.load_wallet_by_name(self.electrum_config.get_wallet_path(use_gui_last_wallet=True))
         # URI passed in config
         uri = self.electrum_config.get('url')
@@ -1072,7 +1076,6 @@ class ElectrumWindow(App):
 
     def __delete_wallet(self, pw):
         wallet_path = self.get_wallet_path()
-        dirname = os.path.dirname(wallet_path)
         basename = os.path.basename(wallet_path)
         if self.wallet.has_password():
             try:

electrum/gui/kivy/uix/dialogs/wallets.py

@@ -6,6 +6,7 @@ from kivy.properties import ObjectProperty
 from kivy.lang import Builder
 
 from electrum.util import base_units
+from electrum.storage import StorageReadWriteError
 
 from ...i18n import _
 from .label_dialog import LabelDialog
@@ -54,12 +55,20 @@ Builder.load_string('''
 class WalletDialog(Factory.Popup):
 
     def new_wallet(self, app, dirname):
-        def cb(text):
-            if text:
-                app.load_wallet_by_name(os.path.join(dirname, text))
+        def cb(filename):
+            if not filename:
+                return
+            # FIXME? "filename" might contain ".." (etc) and hence sketchy path traversals are possible
+            try:
+                app.load_wallet_by_name(os.path.join(dirname, filename))
+            except StorageReadWriteError:
+                app.show_error(_("R/W error accessing path"))
         d = LabelDialog(_('Enter wallet name'), '', cb)
         d.open()
 
     def open_wallet(self, app):
-        app.load_wallet_by_name(self.ids.wallet_selector.selection[0])
+        try:
+            app.load_wallet_by_name(self.ids.wallet_selector.selection[0])
+        except StorageReadWriteError:
+            app.show_error(_("R/W error accessing path"))
 

electrum/gui/qt/installwizard.py

@@ -15,7 +15,7 @@ from PyQt5.QtWidgets import (QWidget, QDialog, QLabel, QHBoxLayout, QMessageBox,
                              QGridLayout, QSlider, QScrollArea, QApplication)
 
 from electrum.wallet import Wallet, Abstract_Wallet
-from electrum.storage import WalletStorage
+from electrum.storage import WalletStorage, StorageReadWriteError
 from electrum.util import UserCancelled, InvalidPassword, WalletFileException
 from electrum.base_wizard import BaseWizard, HWD_SETUP_DECRYPT_WALLET, GoBack
 from electrum.i18n import _
@@ -193,8 +193,11 @@ class InstallWizard(QDialog, MessageBoxMixin, BaseWizard):
         vbox.addLayout(hbox2)
         self.set_layout(vbox, title=_('Electrum wallet'))
 
-        temp_storage = WalletStorage(path, manual_upgrades=True)
-        wallet_folder = os.path.dirname(temp_storage.path)
+        try:
+            temp_storage = WalletStorage(path, manual_upgrades=True)
+        except StorageReadWriteError:
+            temp_storage = None  # type: Optional[WalletStorage]
+        wallet_folder = os.path.dirname(path)
 
         def on_choose():
             path, __ = QFileDialog.getOpenFileName(self, "Select your wallet file", wallet_folder)
@@ -202,19 +205,21 @@ class InstallWizard(QDialog, MessageBoxMixin, BaseWizard):
                 self.name_e.setText(path)
 
         def on_filename(filename):
+            # FIXME? "filename" might contain ".." (etc) and hence sketchy path traversals are possible
             nonlocal temp_storage
+            temp_storage = None
             path = os.path.join(wallet_folder, filename)
             wallet_from_memory = get_wallet_from_daemon(path)
             try:
                 if wallet_from_memory:
-                    temp_storage = wallet_from_memory.storage
+                    temp_storage = wallet_from_memory.storage  # type: Optional[WalletStorage]
                 else:
                     temp_storage = WalletStorage(path, manual_upgrades=True)
-                self.next_button.setEnabled(True)
-            except BaseException:
+            except StorageReadWriteError:
+                pass
+            except Exception:
                 self.logger.exception('')
-                temp_storage = None
-                self.next_button.setEnabled(False)
+            self.next_button.setEnabled(temp_storage is not None)
             user_needs_to_enter_password = False
             if temp_storage:
                 if not temp_storage.file_exists():
@@ -246,12 +251,12 @@ class InstallWizard(QDialog, MessageBoxMixin, BaseWizard):
 
         button.clicked.connect(on_choose)
         self.name_e.textChanged.connect(on_filename)
-        n = os.path.basename(temp_storage.path)
-        self.name_e.setText(n)
+        self.name_e.setText(os.path.basename(path))
 
         while True:
             if self.loop.exec_() != 2:  # 2 = next
                 raise UserCancelled
+            assert temp_storage
             if temp_storage.file_exists() and not temp_storage.is_encrypted():
                 break
             if not temp_storage.file_exists():

electrum/storage.py

@@ -50,6 +50,9 @@ class StorageEncryptionVersion(IntEnum):
     XPUB_PASSWORD = 2
 
 
+class StorageReadWriteError(Exception): pass
+
+
 class WalletStorage(Logger):
 
     def __init__(self, path, *, manual_upgrades=False):
@@ -61,7 +64,7 @@ class WalletStorage(Logger):
         DB_Class = JsonDB
         self.logger.info(f"wallet path {self.path}")
         self.pubkey = None
-        # TODO we should test r/w permissions here (whether file exists or not)
+        self._test_read_write_permissions(self.path)
         if self.file_exists():
             with open(self.path, "r", encoding='utf-8') as f:
                 self.raw = f.read()
@@ -74,6 +77,27 @@ class WalletStorage(Logger):
             # avoid new wallets getting 'upgraded'
             self.db = DB_Class('', manual_upgrades=False)
 
+    @classmethod
+    def _test_read_write_permissions(cls, path):
+        # note: There might already be a file at 'path'.
+        #       Make sure we do NOT overwrite/corrupt that!
+        temp_path = "%s.tmptest.%s" % (path, os.getpid())
+        echo = "fs r/w test"
+        try:
+            # test READ permissions for actual path
+            if os.path.exists(path):
+                with open(path, "r", encoding='utf-8') as f:
+                    f.read(1)  # read 1 byte
+            # test R/W sanity for "similar" path
+            with open(temp_path, "w", encoding='utf-8') as f:
+                f.write(echo)
+            with open(temp_path, "r", encoding='utf-8') as f:
+                echo2 = f.read()
+            os.remove(temp_path)
+        except Exception as e:
+            raise StorageReadWriteError(e) from e
+        if echo != echo2:
+            raise StorageReadWriteError('echo sanity-check failed')
 
     def load_plugins(self):
         wallet_type = self.db.get('wallet_type')