Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fixes lp978896 -- Session fixation security fix
Rotates session tokens on logout, and properly clears sessions
to prevent data leakage.

Change-Id: I52d03576d07b1e023a7730857156d0da1887b1df
  • Loading branch information
Paul McMillan committed May 4, 2012
1 parent 0f952bc commit 041b1c4
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 4 deletions.
2 changes: 1 addition & 1 deletion horizon/exceptions.py
Expand Up @@ -203,7 +203,7 @@ class indicating the type of exception that was encountered will be
if issubclass(exc_type, UNAUTHORIZED):
if ignore:
return NotAuthorized
request.session.clear()
request.user_logout()
if not handled:
LOG.debug("Unauthorized: %s" % exc_value)
# We get some pretty useless error messages back from
Expand Down
9 changes: 9 additions & 0 deletions horizon/middleware.py
Expand Up @@ -49,6 +49,15 @@ def process_request(self, request):
Adds a :class:`~horizon.users.User` object to ``request.user``.
"""
# A quick and dirty way to log users out
def user_logout(request):
if hasattr(request, '_cached_user'):
del request._cached_user
# Use flush instead of clear, so we rotate session keys in
# addition to clearing all the session data
request.session.flush()
request.__class__.user_logout = user_logout

request.__class__.user = users.LazyUser()
request.horizon = {'dashboard': None, 'panel': None}

Expand Down
52 changes: 52 additions & 0 deletions horizon/tests/auth_tests.py
Expand Up @@ -18,6 +18,8 @@
# License for the specific language governing permissions and limitations
# under the License.

import time

from django import http
from django.core.urlresolvers import reverse
from keystoneclient import exceptions as keystone_exceptions
Expand Down Expand Up @@ -220,3 +222,53 @@ def test_logout(self):

self.assertRedirectsNoFollow(res, reverse('splash'))
self.assertNotIn(KEY, self.client.session)

def test_session_fixation(self):
session_ids = []
form_data = {'method': 'Login',
'region': 'http://localhost:5000/v2.0',
'password': self.user.password,
'username': self.user.name}

self.mox.StubOutWithMock(api, 'token_create')
self.mox.StubOutWithMock(api, 'tenant_list_for_token')
self.mox.StubOutWithMock(api, 'token_create_scoped')

aToken = self.tokens.unscoped_token
bToken = self.tokens.scoped_token

api.token_create(IsA(http.HttpRequest), "", self.user.name,
self.user.password).AndReturn(aToken)
api.tenant_list_for_token(IsA(http.HttpRequest),
aToken.id).AndReturn([self.tenants.first()])
api.token_create_scoped(IsA(http.HttpRequest),
self.tenant.id,
aToken.id).AndReturn(bToken)

api.token_create(IsA(http.HttpRequest), "", self.user.name,
self.user.password).AndReturn(aToken)
api.tenant_list_for_token(IsA(http.HttpRequest),
aToken.id).AndReturn([self.tenants.first()])
api.token_create_scoped(IsA(http.HttpRequest),
self.tenant.id,
aToken.id).AndReturn(bToken)
self.mox.ReplayAll()

res = self.client.get(reverse('horizon:auth_login'))
self.assertEqual(res.cookies.get('sessionid'), None)
res = self.client.post(reverse('horizon:auth_login'), form_data)
session_ids.append(res.cookies['sessionid'].value)

self.assertEquals(self.client.session['user_name'],
self.user.name)
self.client.session['foobar'] = 'MY TEST VALUE'
res = self.client.get(reverse('horizon:auth_logout'))
session_ids.append(res.cookies['sessionid'].value)
self.assertEqual(len(self.client.session.items()), 0)
# Sleep for 1 second so the session values are different if
# using the signed_cookies backend.
time.sleep(1)
res = self.client.post(reverse('horizon:auth_login'), form_data)
session_ids.append(res.cookies['sessionid'].value)
# Make sure all 3 session id values are different
self.assertEqual(len(session_ids), len(set(session_ids)))
2 changes: 1 addition & 1 deletion horizon/users.py
Expand Up @@ -59,7 +59,7 @@ def get_user_from_request(request):
# If any of those keys are missing from the session it is
# overwhelmingly likely that we're dealing with an outdated session.
LOG.exception("Error while creating User from session.")
request.session.clear()
request.user_logout()
raise exceptions.NotAuthorized(_("Your session has expired. "
"Please log in again."))

Expand Down
2 changes: 1 addition & 1 deletion horizon/views/auth.py
Expand Up @@ -96,6 +96,6 @@ def switch_tenants(request, tenant_id):

def logout(request):
""" Clears the session and logs the current user out. """
request.session.clear()
request.user_logout()
# FIXME(gabriel): we don't ship a view named splash
return shortcuts.redirect('splash')
12 changes: 11 additions & 1 deletion horizon/views/auth_forms.py
Expand Up @@ -77,6 +77,16 @@ def __init__(self, *args, **kwargs):
self.fields['region'].widget = forms.widgets.HiddenInput()

def handle(self, request, data):
if 'user_name' in request.session:
if request.session['user_name'] != data['username']:
# To avoid reusing another user's session, create a
# new, empty session if the existing session
# corresponds to a different authenticated user.
request.session.flush()
# Always cycle the session key when viewing the login form to
# prevent session fixation
request.session.cycle_key()

# For now we'll allow fallback to OPENSTACK_KEYSTONE_URL if the
# form post doesn't include a region.
endpoint = data.get('region', None) or settings.OPENSTACK_KEYSTONE_URL
Expand Down Expand Up @@ -116,7 +126,7 @@ def handle(self, request, data):
# If we get here we don't want to show a stack trace to the
# user. However, if we fail here, there may be bad session
# data that's been cached already.
request.session.clear()
request.user_logout()
exceptions.handle(request,
message=_("An error occurred authenticating."
" Please try again later."),
Expand Down

0 comments on commit 041b1c4

Please sign in to comment.