[tor-commits] [stem/master] Authentication failures could cause incorrect response

[atagar at torproject.org]

commit a275838663a716287110eec7250360a9ab14a670
Author: Damian Johnson <atagar at torproject.org>
Date:   Wed Jun 21 10:08:37 2017 -0700

    Authentication failures could cause incorrect response
    
    Oops. To best ensure we performed post-authentication activities I hacked it
    directly into the BaseController, but these didn't take into account that
    authentication could fail. The post-auth requests then failed because tor
    closes the socket.
    
    Caught thanks to daftaupe on...
    
      https://trac.torproject.org/projects/tor/ticket/22679
    
    While troubleshooting this I ran into another bug that might be an issue with
    tor. When authentication failed we called 'QUIT'. Tor will sometimes respond
    with 'closing connection' and other times just hang. Reached out to Nick to see
    if this is something we'd care to address, but on Stem's side now simply
    omitting a QUIT call if unauthenticated.
---
 docs/change_log.rst                     |  1 +
 stem/connection.py                      |  3 +++
 stem/control.py                         | 14 ++------------
 test/integ/connection/authentication.py | 19 +++++++++++++++++++
 4 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/docs/change_log.rst b/docs/change_log.rst
index 06341b4..96690ac 100644
--- a/docs/change_log.rst
+++ b/docs/change_log.rst
@@ -46,6 +46,7 @@ The following are only available within Stem's `git repository
  * **Controller**
 
   * :func:`~stem.process.launch_tor` raised a ValueError if invoked when outside the main thread
+  * Failure to authenticate could raise an improper response or hang (:trac:`22679`)
   * Renamed :class:`~stem.response.events.ConnectionBandwidthEvent` type attribute to conn_type to avoid conflict with parent class (:trac:`21774`)
   * Added the GUARD_WAIT :data:`~stem.CircStatus` (:spec:`6446210`)
   * Unable to use cookie auth when path includes wide characters (chinese, japanese, etc)
diff --git a/stem/connection.py b/stem/connection.py
index a3f27a0..db3f218 100644
--- a/stem/connection.py
+++ b/stem/connection.py
@@ -584,6 +584,9 @@ def authenticate(controller, password = None, chroot_path = None, protocolinfo_r
         else:
           authenticate_cookie(controller, cookie_path, False)
 
+      if isinstance(controller, stem.control.BaseController):
+        controller._post_authentication()
+
       return  # success!
     except OpenAuthRejected as exc:
       auth_exceptions.append(exc)
diff --git a/stem/control.py b/stem/control.py
index 65e3401..8c01d94 100644
--- a/stem/control.py
+++ b/stem/control.py
@@ -633,13 +633,6 @@ class BaseController(object):
         if isinstance(response, stem.ControllerError):
           raise response
         else:
-          # I really, really don't like putting hooks into this method, but
-          # this is the most reliable method I can think of for taking actions
-          # immediately after successfully authenticating to a connection.
-
-          if message.upper().startswith('AUTHENTICATE'):
-            self._post_authentication()
-
           return response
       except stem.SocketClosed:
         # If the recv() thread caused the SocketClosed then we could still be
@@ -693,10 +686,7 @@ class BaseController(object):
       and **False** otherwise
     """
 
-    if self.is_alive():
-      return self._is_authenticated
-
-    return False
+    return self._is_authenticated if self.is_alive() else False
 
   def connect(self):
     """
@@ -1051,7 +1041,7 @@ class Controller(BaseController):
 
   def close(self):
     # making a best-effort attempt to quit before detaching the socket
-    if self.is_alive():
+    if self.is_authenticated():
       try:
         self.msg('QUIT')
       except:
diff --git a/test/integ/connection/authentication.py b/test/integ/connection/authentication.py
index 3a2fcb0..a8c0c82 100644
--- a/test/integ/connection/authentication.py
+++ b/test/integ/connection/authentication.py
@@ -268,6 +268,25 @@ class TestAuthenticate(unittest.TestCase):
         self.assertRaises(exc_type, self._check_auth, auth_type, auth_value)
 
   @test.require.controller
+  def test_wrong_password_with_controller(self):
+    """
+    We ran into a race condition where providing the wrong password to the
+    Controller caused inconsistent responses. Checking for that...
+
+    https://trac.torproject.org/projects/tor/ticket/22679
+    """
+
+    runner = test.runner.get_runner()
+
+    if test.runner.Torrc.PASSWORD not in runner.get_options():
+      self.skipTest('(requires password auth)')
+      return
+
+    for i in range(10):
+      with runner.get_tor_controller(False) as controller:
+        self.assertRaises(stem.connection.IncorrectPassword, controller.authenticate, 'wrong_password')
+
+  @test.require.controller
   def test_authenticate_cookie(self):
     """
     Tests the authenticate_cookie function.