commit a275838663a716287110eec7250360a9ab14a670 Author: Damian Johnson <atagar@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.