Skip to content

Commit

Permalink
[1.7.x] Fixed #23066 -- Modified RemoteUserMiddleware to logout on RE…
Browse files Browse the repository at this point in the history
…MOTE_USER change.

This is a security fix. Disclosure following shortly.
  • Loading branch information
ptone authored and timgraham committed Aug 20, 2014
1 parent 3123f84 commit 1a45d05
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 8 deletions.
28 changes: 20 additions & 8 deletions django/contrib/auth/middleware.py
Expand Up @@ -76,21 +76,19 @@ def process_request(self, request):
# 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
# persisted in the session and we don't need to continue.
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)
Expand All @@ -112,3 +110,17 @@ def clean_username(self, username, request):
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)
18 changes: 18 additions & 0 deletions django/contrib/auth/tests/test_remote_user.py
Expand Up @@ -125,6 +125,24 @@ def test_header_disappears(self):
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
Expand Down
9 changes: 9 additions & 0 deletions docs/releases/1.4.14.txt
Expand Up @@ -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.
9 changes: 9 additions & 0 deletions docs/releases/1.5.9.txt
Expand Up @@ -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.
9 changes: 9 additions & 0 deletions docs/releases/1.6.6.txt
Expand Up @@ -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
========

Expand Down

0 comments on commit 1a45d05

Please sign in to comment.