From 5307ce565fbedb9cc27cbe7c757b41a00438d37c Mon Sep 17 00:00:00 2001 From: Preston Holmes Date: Sun, 27 Jul 2014 21:54:29 -0700 Subject: [PATCH] Fixed #23066 -- Modified RemoteUserMiddleware to logout on REMOTE_USER change. This is a security fix. Disclosure following shortly. --- django/contrib/auth/middleware.py | 28 +++++++++++++------ django/contrib/auth/tests/test_remote_user.py | 18 ++++++++++++ docs/releases/1.4.14.txt | 9 ++++++ docs/releases/1.5.9.txt | 9 ++++++ docs/releases/1.6.6.txt | 9 ++++++ 5 files changed, 65 insertions(+), 8 deletions(-) diff --git a/django/contrib/auth/middleware.py b/django/contrib/auth/middleware.py index b2f392262a..b34da60299 100644 --- a/django/contrib/auth/middleware.py +++ b/django/contrib/auth/middleware.py @@ -76,14 +76,7 @@ class RemoteUserMiddleware(object): # authenticated remote-user, or return (leaving request.user set to # AnonymousUser by the AuthenticationMiddleware). if request.user.is_authenticated(): - try: - stored_backend = load_backend(request.session.get( - auth.BACKEND_SESSION_KEY, '')) - if isinstance(stored_backend, RemoteUserBackend): - auth.logout(request) - except ImportError: - # backend failed to load - auth.logout(request) + self._remove_invalid_user(request) return # If the user is already authenticated and that user is the user we are # getting passed in the headers, then the correct user is already @@ -91,6 +84,11 @@ class RemoteUserMiddleware(object): if request.user.is_authenticated(): if request.user.get_username() == self.clean_username(username, request): return + else: + # An authenticated user is associated with the request, but + # it does not match the authorized user in the header. + self._remove_invalid_user(request) + # We are seeing this user for the first time in this session, attempt # to authenticate the user. user = auth.authenticate(remote_user=username) @@ -112,3 +110,17 @@ class RemoteUserMiddleware(object): except AttributeError: # Backend has no clean_username method. pass return username + + def _remove_invalid_user(self, request): + """ + Removes the current authenticated user in the request which is invalid + but only if the user is authenticated via the RemoteUserBackend. + """ + try: + stored_backend = load_backend(request.session.get(auth.BACKEND_SESSION_KEY, '')) + except ImportError: + # backend failed to load + auth.logout(request) + else: + if isinstance(stored_backend, RemoteUserBackend): + auth.logout(request) diff --git a/django/contrib/auth/tests/test_remote_user.py b/django/contrib/auth/tests/test_remote_user.py index 2ccfd6e6fa..790e5d0d53 100644 --- a/django/contrib/auth/tests/test_remote_user.py +++ b/django/contrib/auth/tests/test_remote_user.py @@ -125,6 +125,24 @@ class RemoteUserTest(TestCase): response = self.client.get('/remote_user/') self.assertEqual(response.context['user'].username, 'modeluser') + def test_user_switch_forces_new_login(self): + """ + Tests that if the username in the header changes between requests + that the original user is logged out + """ + User.objects.create(username='knownuser') + # Known user authenticates + response = self.client.get('/remote_user/', + **{self.header: self.known_user}) + self.assertEqual(response.context['user'].username, 'knownuser') + # During the session, the REMOTE_USER changes to a different user. + response = self.client.get('/remote_user/', + **{self.header: "newnewuser"}) + # Ensure that the current user is not the prior remote_user + # In backends that create a new user, username is "newnewuser" + # In backends that do not create new users, it is '' (anonymous user) + self.assertNotEqual(response.context['user'].username, 'knownuser') + def tearDown(self): """Restores settings to avoid breaking other tests.""" settings.MIDDLEWARE_CLASSES = self.curr_middleware diff --git a/docs/releases/1.4.14.txt b/docs/releases/1.4.14.txt index 6c140ee6dc..811c3f67ea 100644 --- a/docs/releases/1.4.14.txt +++ b/docs/releases/1.4.14.txt @@ -38,3 +38,12 @@ if a file with the uploaded name already exists. underscore plus a random 7 character alphanumeric string (e.g. ``"_x3a1gho"``), rather than iterating through an underscore followed by a number (e.g. ``"_1"``, ``"_2"``, etc.). + +``RemoteUserMiddleware`` session hijacking +========================================== + +When using the :class:`~django.contrib.auth.middleware.RemoteUserMiddleware` +and the ``RemoteUserBackend``, a change to the ``REMOTE_USER`` header between +requests without an intervening logout could result in the prior user's session +being co-opted by the subsequent user. The middleware now logs the user out on +a failed login attempt. diff --git a/docs/releases/1.5.9.txt b/docs/releases/1.5.9.txt index 232cbb0767..5070f0eca1 100644 --- a/docs/releases/1.5.9.txt +++ b/docs/releases/1.5.9.txt @@ -38,3 +38,12 @@ if a file with the uploaded name already exists. underscore plus a random 7 character alphanumeric string (e.g. ``"_x3a1gho"``), rather than iterating through an underscore followed by a number (e.g. ``"_1"``, ``"_2"``, etc.). + +``RemoteUserMiddleware`` session hijacking +========================================== + +When using the :class:`~django.contrib.auth.middleware.RemoteUserMiddleware` +and the ``RemoteUserBackend``, a change to the ``REMOTE_USER`` header between +requests without an intervening logout could result in the prior user's session +being co-opted by the subsequent user. The middleware now logs the user out on +a failed login attempt. diff --git a/docs/releases/1.6.6.txt b/docs/releases/1.6.6.txt index c2ebdb9efb..e52ea6b23f 100644 --- a/docs/releases/1.6.6.txt +++ b/docs/releases/1.6.6.txt @@ -39,6 +39,15 @@ underscore plus a random 7 character alphanumeric string (e.g. ``"_x3a1gho"``), rather than iterating through an underscore followed by a number (e.g. ``"_1"``, ``"_2"``, etc.). +``RemoteUserMiddleware`` session hijacking +========================================== + +When using the :class:`~django.contrib.auth.middleware.RemoteUserMiddleware` +and the ``RemoteUserBackend``, a change to the ``REMOTE_USER`` header between +requests without an intervening logout could result in the prior user's session +being co-opted by the subsequent user. The middleware now logs the user out on +a failed login attempt. + Bugfixes ========