From b4744a92d4fa7f6d7ade0ae2d99a2dc0ea94734d Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Fri, 4 Nov 2016 14:23:44 +0000 Subject: [PATCH] Fix SAML2 multi-session vulnerability This resolves an issue where Ipsilon can be requested to initiate logout sessions for all currently open sessions, regardless of currently logged in user. Fixes: CVE-2016-8638 Signed-off-by: Patrick Uiterwijk Reviewed-by: Howard Johnson Reviewed-by: Rob Crittenden --- ipsilon/login/common.py | 9 +++++---- ipsilon/providers/saml2/logout.py | 10 +++++----- ipsilon/providers/saml2/sessions.py | 29 +++++++++++++---------------- ipsilon/providers/saml2idp.py | 2 +- 4 files changed, 24 insertions(+), 26 deletions(-) diff --git a/ipsilon/login/common.py b/ipsilon/login/common.py index 4f8b13d..644408e 100644 --- a/ipsilon/login/common.py +++ b/ipsilon/login/common.py @@ -369,10 +369,11 @@ def __init__(self, *args, **kwargs): def root(self, *args, **kwargs): us = UserSession() - for provider in self.handlers: - self.debug("Calling logout for provider %s" % provider) - obj = self.handlers[provider] - obj() + if us.user is not None: + for provider in self.handlers: + self.debug("Calling logout for provider %s" % provider) + obj = self.handlers[provider] + obj() us.logout(self.user) return self._template('logout.html', title='Logout') diff --git a/ipsilon/providers/saml2/logout.py b/ipsilon/providers/saml2/logout.py index b2ebe0b..e31ce91 100644 --- a/ipsilon/providers/saml2/logout.py +++ b/ipsilon/providers/saml2/logout.py @@ -278,7 +278,7 @@ def logout(self, message, relaystate=None, samlresponse=None): lasso.SAML2_METADATA_BINDING_REDIRECT, ] (logout_mech, session) = saml_sessions.get_next_logout( - logout_mechs=logout_order) + logout_mechs=logout_order, user=us.user) while session: self.debug('Going to log out %s' % session.provider_id) @@ -302,7 +302,7 @@ def logout(self, message, relaystate=None, samlresponse=None): ) saml_sessions.remove_session(session) (logout_mech, session) = saml_sessions.get_next_logout( - logout_mechs=logout_order) + logout_mechs=logout_order, user=us.user) continue try: @@ -316,7 +316,7 @@ def logout(self, message, relaystate=None, samlresponse=None): # log out self.debug('logging out provider id %s' % session.provider_id) indexes = saml_sessions.get_session_id_by_provider_id( - session.provider_id + session.provider_id, us.user ) self.debug('Requesting logout for sessions %s' % (indexes,)) req = logout.get_request() @@ -350,7 +350,7 @@ def logout(self, message, relaystate=None, samlresponse=None): self.error('Provider does not support SOAP') (logout_mech, session) = saml_sessions.get_next_logout( - logout_mechs=logout_order) + logout_mechs=logout_order, user=us.user) # done while @@ -358,7 +358,7 @@ def logout(self, message, relaystate=None, samlresponse=None): # original request using the response we cached earlier. try: - session = saml_sessions.get_initial_logout() + session = saml_sessions.get_initial_logout(us.user) except ValueError: self.debug('SLO get_last_session() unable to find last session') raise cherrypy.HTTPError(400, 'Unable to determine logout state') diff --git a/ipsilon/providers/saml2/sessions.py b/ipsilon/providers/saml2/sessions.py index d3ed7e2..3de455e 100644 --- a/ipsilon/providers/saml2/sessions.py +++ b/ipsilon/providers/saml2/sessions.py @@ -113,7 +113,6 @@ class SAMLSessionFactory(Log): """ def __init__(self, database_url): self._ss = SAML2SessionStore(database_url=database_url) - self.user = None def _data_to_samlsession(self, uuidval, data): """ @@ -143,8 +142,6 @@ def add_session(self, session_id, provider_id, user, login_session, :param request_id: The request ID of the Logout :param supported_logout_mechs: A list of logout protocols supported """ - self.user = user - timeout = cherrypy_config['tools.sessions.timeout'] t = datetime.timedelta(seconds=timeout * 60) expiration_time = datetime.datetime.now() + t @@ -175,11 +172,11 @@ def get_session_by_id(self, session_id): return self._data_to_samlsession(uuidval, data) - def get_session_id_by_provider_id(self, provider_id): + def get_session_id_by_provider_id(self, provider_id, user): """ Return a tuple of logged-in session IDs by provider_id """ - candidates = self._ss.get_user_sessions(self.user) + candidates = self._ss.get_user_sessions(user) session_ids = [] for c in candidates: @@ -228,7 +225,7 @@ def start_logout(self, samlsession, relaystate=None, initial=True): self._ss.update_session(datum) def get_next_logout(self, peek=False, - logout_mechs=None): + logout_mechs=None, user=None): """ Get the next session in the logged-in state and move it to the logging_out state. Return the session that is @@ -246,7 +243,7 @@ def get_next_logout(self, peek=False, Returns a tuple of (mechanism, session) or (None, None) if no more sessions in LOGGED_IN state. """ - candidates = self._ss.get_user_sessions(self.user) + candidates = self._ss.get_user_sessions(user) if logout_mechs is None: logout_mechs = [SAML2_METADATA_BINDING_REDIRECT, ] @@ -260,13 +257,13 @@ def get_next_logout(self, peek=False, return (mech, samlsession) return (None, None) - def get_initial_logout(self): + def get_initial_logout(self, user): """ Get the initial logout request. Raises ValueError if no sessions in INIT_LOGOUT state. """ - candidates = self._ss.get_user_sessions(self.user) + candidates = self._ss.get_user_sessions(user) # FIXME: what does it mean if there are multiple in init? We # just return the first one for now. How do we know @@ -282,11 +279,11 @@ def get_initial_logout(self): def wipe_data(self): self._ss.wipe_data() - def dump(self): + def dump(self, user): """ Dump all sessions to debug log """ - candidates = self._ss.get_user_sessions(self.user) + candidates = self._ss.get_user_sessions(user) count = 0 for c in candidates: @@ -314,13 +311,13 @@ def dump(self): SAML2_METADATA_BINDING_REDIRECT]) # Test finding sessions by provider - ids = factory.get_session_id_by_provider_id(provider2) + ids = factory.get_session_id_by_provider_id(provider2, user='admin') assert(len(ids) == 1) sess3 = factory.add_session('_345678', provider2, "testuser", "", '_3456', [SAML2_METADATA_BINDING_REDIRECT]) - ids = factory.get_session_id_by_provider_id(provider2) + ids = factory.get_session_id_by_provider_id(provider2, user='testuser') assert(len(ids) == 2) # Test finding sessions by session ID @@ -343,13 +340,13 @@ def dump(self): test2 = factory.get_session_by_id('_789012') factory.start_logout(test2, initial=True) - (lmech, test3) = factory.get_next_logout() + (lmech, test3) = factory.get_next_logout(user='admin') assert(test3.session_id == '_345678') - test4 = factory.get_initial_logout() + test4 = factory.get_initial_logout(user='admin') assert(test4.session_id == '_789012') # Even though we've started logout, make sure we can still find # all sessions for a provider. - ids = factory.get_session_id_by_provider_id(provider2) + ids = factory.get_session_id_by_provider_id(provider2, user='admin') assert(len(ids) == 2) diff --git a/ipsilon/providers/saml2idp.py b/ipsilon/providers/saml2idp.py index 9323e0c..c3b6b5a 100644 --- a/ipsilon/providers/saml2idp.py +++ b/ipsilon/providers/saml2idp.py @@ -410,7 +410,7 @@ def idp_initiated_logout(self): saml_sessions = self.sessionfactory # pylint: disable=unused-variable (mech, session) = saml_sessions.get_next_logout( - logout_mechs=[lasso.SAML2_METADATA_BINDING_REDIRECT]) + logout_mechs=[lasso.SAML2_METADATA_BINDING_REDIRECT], user=us.user) if session is None: return