Fix security bug regarding long lived threads

securedrop_client/logic.py

@@ -239,6 +239,8 @@ def start_message_thread(self):
             self.message_sync.moveToThread(self.message_thread)
             self.message_thread.started.connect(self.message_sync.run)
             self.message_thread.start()
+        else:  # Already running from last login
+            self.message_sync.api = self.api
 
     def start_reply_thread(self):
         """
@@ -250,6 +252,8 @@ def start_reply_thread(self):
             self.reply_sync.moveToThread(self.reply_thread)
             self.reply_thread.started.connect(self.reply_sync.run)
             self.reply_thread.start()
+        else:  # Already running from last login
+            self.reply_sync.api = self.api
 
     def timeout_cleanup(self, thread_id, user_callback):
         """
@@ -466,7 +470,8 @@ def logout(self):
         state.
         """
         self.api = None
-        # self.stop_message_thread()
+        self.message_sync.api = None
+        self.reply_sync.api = None
         self.gui.logout()
 
     def set_status(self, message, duration=5000):

securedrop_client/message_sync.py

@@ -80,10 +80,11 @@ def run(self, loop=True):
                     # Need to set filename on non-Qubes platforms
                     sdk_submission.filename = db_submission.filename
 
-                    self.fetch_the_thing(sdk_submission,
-                                         db_submission,
-                                         self.api.download_submission,
-                                         storage.mark_file_as_downloaded)
+                    if self.api:
+                        self.fetch_the_thing(sdk_submission,
+                                             db_submission,
+                                             self.api.download_submission,
+                                             storage.mark_file_as_downloaded)
 
                 except Exception as e:
                     logger.critical(
@@ -121,10 +122,11 @@ def run(self, loop=True):
                     # Need to set filename on non-Qubes platforms
                     sdk_reply.filename = db_reply.filename
 
-                    self.fetch_the_thing(sdk_reply,
-                                         db_reply,
-                                         self.api.download_reply,
-                                         storage.mark_reply_as_downloaded)
+                    if self.api:
+                        self.fetch_the_thing(sdk_reply,
+                                             db_reply,
+                                             self.api.download_reply,
+                                             storage.mark_reply_as_downloaded)
 
                 except Exception as e:
                     logger.critical(

tests/test_logic.py

@@ -99,6 +99,23 @@ def test_Client_start_message_thread(safe_tmpdir):
         cl.message_thread.start.assert_called_once_with()
 
 
+def test_Client_start_message_thread_if_already_running(safe_tmpdir):
+    """
+    Ensure that when starting the message thread, we don't start another thread
+    if it's already running. Instead, we just authenticate the existing thread.
+    """
+    mock_gui = mock.MagicMock()
+    mock_session = mock.MagicMock()
+    cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir))
+    cl.api = 'api object'
+    cl.message_sync = mock.MagicMock()
+    cl.message_thread = mock.MagicMock()
+    cl.message_thread.api = None
+    cl.start_message_thread()
+    cl.message_sync.api = cl.api
+    cl.message_thread.start.assert_not_called()
+
+
 def test_Client_start_reply_thread(safe_tmpdir):
     """
     When starting reply-fetching thread, make sure we do a few things.
@@ -116,6 +133,23 @@ def test_Client_start_reply_thread(safe_tmpdir):
         cl.reply_thread.start.assert_called_once_with()
 
 
+def test_Client_start_reply_thread_if_already_running(safe_tmpdir):
+    """
+    Ensure that when starting the reply thread, we don't start another thread
+    if it's already running. Instead, we just authenticate the existing thread.
+    """
+    mock_gui = mock.MagicMock()
+    mock_session = mock.MagicMock()
+    cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir))
+    cl.api = 'api object'
+    cl.reply_sync = mock.MagicMock()
+    cl.reply_thread = mock.MagicMock()
+    cl.reply_thread.api = None
+    cl.start_reply_thread()
+    cl.reply_sync.api = cl.api
+    cl.reply_thread.start.assert_not_called()
+
+
 def test_Client_call_api(safe_tmpdir):
     """
     A new thread and APICallRunner is created / setup.
@@ -676,13 +710,20 @@ def test_Client_on_update_star_failed(safe_tmpdir):
 def test_Client_logout(safe_tmpdir):
     """
     The API is reset to None and the UI is set to logged out state.
+    The message and reply threads should also have the
     """
     mock_gui = mock.MagicMock()
     mock_session = mock.MagicMock()
     cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir))
     cl.api = mock.MagicMock()
+    cl.message_sync = mock.MagicMock()
+    cl.reply_sync = mock.MagicMock()
+    cl.message_sync.api = mock.MagicMock()
+    cl.reply_sync.api = mock.MagicMock()
     cl.logout()
     assert cl.api is None
+    assert cl.message_sync.api is None
+    assert cl.reply_sync.api is None
     cl.gui.logout.assert_called_once_with()