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 <[email protected]>
Reviewed-by: Howard Johnson <[email protected]>
Reviewed-by: Rob Crittenden <[email protected]>

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')

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,15 +350,15 @@ 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
 
         # All sessions should be logged out now. Respond to the
         # 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')

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",
                                 "<Login/>", '_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)

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