Fix for "users must login twice" issue 1.13.2

[Benjamin Smith]

Users were complaining about having to try two, three, four, or more times to
be able to login to another system with SSO. In this email is our fix. Please
advise if there's a better way to submit this patch.

Configuration:
We have a web-based product and use sharedance to manage the PHP session
functions data store. In order to preserve the session state, we're hosting
SimpleSAML at an alternate port number (EG: https://website.com:4430) so that
we can read the user browser sessions from our system to authenticate SAML.

Notes from our commit log:
At last, a fix for the auto-logout on the first attempt problem! The
sequence was found to be:

1) User logs into our SIS, gets a sessionID. (EG: 123)

2) User is sent to BH.

3) User gets sent back to SAML instance.

4) SAML looks for $_SESSION['SimpleSAMLphp_SESSION'] at
SessionHandlerPHP.php:168 and fails.

5) SAML creates a new sessionID, causing all the old auth credentials
to be lost/orphaned.

6) Second login (with new PHPSESSID) is done in our system,
and this time #4 works because the deconstructor (which is the only apparent
place sessions get saved in this !@#% thing) was previously called at #5.

This fix is to recycle the sessionID already set if it is set.

[root@xr2 www-saml]# hg diff simplesamlphp/lib/SimpleSAML/SessionHandlerPHP.php
-r 3
diff -r 67fd97537b3d simplesamlphp/lib/SimpleSAML/SessionHandlerPHP.php
--- a/simplesamlphp/lib/SimpleSAML/SessionHandlerPHP.php Fri Dec 11
01:06:26 2015 +0000
+++ b/simplesamlphp/lib/SimpleSAML/SessionHandlerPHP.php Sat Dec 19
00:43:59 2015 +0000
@@ -77,7 +77,8 @@
}

/* Generate new (secure) session id. */
- $sessionId =
SimpleSAML_Utilities::stringToHex(SimpleSAML_Utilities::generateRandomBytes(16));
+ if (!$sessionId = Session_Id())
+ $sessionId =
SimpleSAML_Utilities::stringToHex(SimpleSAML_Utilities::generateRandomBytes(16));
SimpleSAML_Session::createSession($sessionId);

if (session_id() !== '') {

[Jaime Perez Crespo]

Hi,

> On 19 Dec 2015, at 01:54 AM, Benjamin Smith <li...@benjamindsmith.com> wrote:
> Users were complaining about having to try two, three, four, or more times to
> be able to login to another system with SSO. In this email is our fix. Please
> advise if there's a better way to submit this patch.

Since SimpleSAMLphp is hosted in github, the proper way to submit patches is to submit a pull request there.

> Configuration:
> We have a web-based product and use sharedance to manage the PHP session
> functions data store. In order to preserve the session state, we're hosting
> SimpleSAML at an alternate port number (EG: https://website.com:4430) so that
> we can read the user browser sessions from our system to authenticate SAML.

I understand this is a service provider, but you are not explicitly saying so. If that’s the case, using a different port number is wrong. You need the cookies to be available at the same time for both SimpleSAMLphp and the application you are protecting. If you put SimpleSAMLphp in an alternate port number, the cookies will not be available when needed (that is, when the application requests authentication).

> Notes from our commit log:
> At last, a fix for the auto-logout on the first attempt problem! The
> sequence was found to be:
>
> 1) User logs into our SIS, gets a sessionID. (EG: 123)

What is SIS?

> 2) User is sent to BH.

What is BH?

> 3) User gets sent back to SAML instance.

What "SAML instance”? A service provider? The identity provider?

> 4) SAML looks for $_SESSION['SimpleSAMLphp_SESSION'] at
> SessionHandlerPHP.php:168 and fails.

Why does it fail? What’s the URL you are in when SimpleSAMLphp tries to access the session, and what was the URL when the session got set first?

> 5) SAML creates a new sessionID, causing all the old auth credentials
> to be lost/orphaned.
>
> 6) Second login (with new PHPSESSID) is done in our system,
> and this time #4 works because the deconstructor (which is the only apparent
> place sessions get saved in this !@#% thing) was previously called at #5.

Uhm… what?

Yes, sessions are saved in the __destruct() method, because that’s the point in time where you know that you need to save the session (if you need to). Besides, destructors are called *always* when you finish execution on every single object that you have created. So if you had a previous session (SimpleSAMLphp will create one when you hit almost any page, if you don’t have one already), the __destruct() method was called then, and the session was saved properly. Whenever you create or modify a session, the session is saved. Always.

> This fix is to recycle the sessionID already set if it is set.

This is not a fix, this is a bug. You are trying to fix a configuration issue by introducing a bug and a security issue in the code.

First: you are modifying a method called “newSessionId()”, forcing it to reuse an existing session, breaking the contract specified by the documentation. If the method is supposed to return a new session ID, it *must* create a new session ID.
Second: by recycling sessions like this, you are making it impossible for other code using this method to create new sessions when needed. This creates a session fixation vulnerability, since SimpleSAMLphp is then no longer able to create new, fresh sessions if a session already exists.

What you need to do is trace the entire flow, see what happens with cookies (when are they set and where are they sent to), and fix the problems you most likely have with your configuration. PHP sessions work perfectly fine with SimpleSAMLphp 1.13.2.

--
Jaime Pérez
UNINETT / Feide
mail: jaime...@uninett.no
xmpp: ja...@jabber.uninett.no

"Two roads diverged in a wood, and I, I took the one less traveled by, and that has made all the difference."
- Robert Frost