From 4056af12443d44bd5289c7c6f40cb334a3e99b38 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jaime=20Pe=CC=81rez?= <jaime.perez@uninett.no>
Date: Sat, 2 Jul 2016 18:48:45 +0200
Subject: [PATCH] bugfix: Stop SimpleSAML_SessionHandler::newSessionId() from
initializing the session.
Historically, SimpleSAML_SessionHandler::newSessionId() has also created the session, sending the cookies to the browser. This is problematic both because given the name of the method one would not assume such behaviour, and also because even for transient sessions the handler would then try to set cookies. When we are using a transient session, it is likely to be because we cannot set cookies or because there was a temporary error when loading the session. If we try to set the cookies even for transient sessions, we could either get an error because cookies cannot be set, or overwrite the previous session cookies with transient ones, trashing a legitimate session in case a temporary error occurs.
As a side effect, this can also cause behaviours like the one described in issue #413. There's no point in trying to set the cookies when it's not possible, so we shouldn't even try, and save us the errors.
To fix this, we made SimpleSAML_SessionHandler::setCookie() abstract, forcing each extending class to implement it. The former implementation is moved to SimpleSAML_SessionHandlerCookie, and the SimpleSAML_SessionHandlerPHP gets a new method that starts the session, effectively sending the cookie. SimpleSAML_Session would then be responsible to call the setCookie() method of the session handler when creating a regular session, and skip it when creating a transient one. This introduces a bug, since SimpleSAML_Session was trying to set the auth token cookie calling the same setCookie() method in the session handler. We fixed that by using SimpleSAML\Utils\HTTP::setCookie() instead, in 8756835bacc7057734aba7fe349b534e63261253.
This resolves #413.
---
lib/SimpleSAML/Session.php | 34 +++++++------
lib/SimpleSAML/SessionHandler.php | 38 +++++---------
lib/SimpleSAML/SessionHandlerCookie.php | 27 +++++++++-
lib/SimpleSAML/SessionHandlerPHP.php | 66 ++++++++++++++++---------
4 files changed, 101 insertions(+), 64 deletions(-)
diff --git a/lib/SimpleSAML/Session.php b/lib/SimpleSAML/Session.php
index 2690be792..5373158c2 100644
--- a/lib/SimpleSAML/Session.php
+++ b/lib/SimpleSAML/Session.php
@@ -160,10 +160,11 @@ class SimpleSAML_Session
if ($this->sessionId === null) {
$this->sessionId = $sh->newSessionId();
}
-
} else { // regular session
$sh = SimpleSAML_SessionHandler::getSessionHandler();
$this->sessionId = $sh->newSessionId();
+ $sh->setCookie($sh->getSessionCookieName(), $this->sessionId, $sh->getCookieParams());
+
$this->trackid = bin2hex(openssl_random_pseudo_bytes(5));
SimpleSAML\Logger::setTrackId($this->trackid);
@@ -197,19 +198,15 @@ class SimpleSAML_Session
$session = null;
try {
$session = self::getSession();
-
} catch (Exception $e) {
- // for some reason, we were unable to initialize this session, use a transient session instead
- self::useTransientSession();
-
+ /*
+ * For some reason, we were unable to initialize this session. Note that this error might be temporary, and
+ * it's possible that we can recover from it in subsequent requests, so we should not try to create a new
+ * session here. Therefore, use just a transient session and throw the exception for someone else to handle
+ * it.
+ */
SimpleSAML\Logger::error('Error loading session: '.$e->getMessage());
- if ($e instanceof SimpleSAML_Error_Exception) {
- $cause = $e->getCause();
- if ($cause instanceof Exception) {
- throw $cause;
- }
- }
- throw $e;
+ self::useTransientSession($e);
}
// if getSession() found it, use it
@@ -227,8 +224,17 @@ class SimpleSAML_Session
return self::$instance;
}
- // create a new session
- return self::load(new SimpleSAML_Session());
+ // try to create a new session
+ try {
+ self::load(new SimpleSAML_Session());
+ } catch (\SimpleSAML\Error\CannotSetCookie $e) {
+ // can't create a regular session because we can't set cookies. Use transient.
+ SimpleSAML\Logger::error('Error creating session: '.$e->getMessage());
+ self::useTransientSession();
+ }
+
+ // we must have a session now, either regular or transient
+ return self::$instance;
}
/**
diff --git a/lib/SimpleSAML/SessionHandler.php b/lib/SimpleSAML/SessionHandler.php
index 23d826f53..cfd830760 100644
--- a/lib/SimpleSAML/SessionHandler.php
+++ b/lib/SimpleSAML/SessionHandler.php
@@ -54,7 +54,7 @@ abstract class SimpleSAML_SessionHandler
/**
- * Create and set new session id.
+ * Create a new session id.
*
* @return string The new session id.
*/
@@ -95,6 +95,18 @@ abstract class SimpleSAML_SessionHandler
abstract public function loadSession($sessionId = null);
+ /**
+ * Set a session cookie.
+ *
+ * @param string $sessionName The name of the session.
+ * @param string|null $sessionID The session ID to use. Set to null to delete the cookie.
+ * @param array|null $cookieParams Additional parameters to use for the session cookie.
+ *
+ * @throws \SimpleSAML\Error\CannotSetCookie If we can't set the cookie.
+ */
+ abstract public function setCookie($sessionName, $sessionID, array $cookieParams = null);
+
+
/**
* Initialize the session handler.
*
@@ -125,7 +137,6 @@ abstract class SimpleSAML_SessionHandler
*/
public function hasSessionCookie()
{
-
return true;
}
@@ -138,7 +149,6 @@ abstract class SimpleSAML_SessionHandler
*/
public function getCookieParams()
{
-
$config = SimpleSAML_Configuration::getInstance();
return array(
@@ -149,26 +159,4 @@ abstract class SimpleSAML_SessionHandler
'httponly' => true,
);
}
-
-
- /**
- * Set a session cookie.
- *
- * @param string $name The name of the session cookie.
- * @param string|null $value The value of the cookie. Set to null to delete the cookie.
- * @param array|null $params Additional params to use for the session cookie.
- */
- public function setCookie($name, $value, array $params = null)
- {
- assert('is_string($name)');
- assert('is_string($value) || is_null($value)');
-
- if ($params !== null) {
- $params = array_merge($this->getCookieParams(), $params);
- } else {
- $params = $this->getCookieParams();
- }
-
- \SimpleSAML\Utils\HTTP::setCookie($name, $value, $params);
- }
}
diff --git a/lib/SimpleSAML/SessionHandlerCookie.php b/lib/SimpleSAML/SessionHandlerCookie.php
index c8409a8d7..e5c02bff6 100644
--- a/lib/SimpleSAML/SessionHandlerCookie.php
+++ b/lib/SimpleSAML/SessionHandlerCookie.php
@@ -45,7 +45,7 @@ abstract class SimpleSAML_SessionHandlerCookie extends SimpleSAML_SessionHandler
/**
- * Create and set new session id.
+ * Create a new session id.
*
* @return string The new session id.
*/
@@ -53,7 +53,6 @@ abstract class SimpleSAML_SessionHandlerCookie extends SimpleSAML_SessionHandler
{
$this->session_id = self::createSessionID();
SimpleSAML_Session::createSession($this->session_id);
- $this->setCookie($this->cookie_name, $this->session_id);
return $this->session_id;
}
@@ -142,4 +141,28 @@ abstract class SimpleSAML_SessionHandlerCookie extends SimpleSAML_SessionHandler
{
return array_key_exists($this->cookie_name, $_COOKIE);
}
+
+
+ /**
+ * Set a session cookie.
+ *
+ * @param string $sessionName The name of the session.
+ * @param string|null $sessionID The session ID to use. Set to null to delete the cookie.
+ * @param array|null $cookieParams Additional parameters to use for the session cookie.
+ *
+ * @throws \SimpleSAML\Error\CannotSetCookie If we can't set the cookie.
+ */
+ public function setCookie($sessionName, $sessionID, array $cookieParams = null)
+ {
+ assert('is_string($sessionName)');
+ assert('is_string($sessionID) || is_null($sessionID)');
+
+ if ($cookieParams !== null) {
+ $params = array_merge($this->getCookieParams(), $cookieParams);
+ } else {
+ $params = $this->getCookieParams();
+ }
+
+ \SimpleSAML\Utils\HTTP::setCookie($sessionName, $sessionID, $params, true);
+ }
}
diff --git a/lib/SimpleSAML/SessionHandlerPHP.php b/lib/SimpleSAML/SessionHandlerPHP.php
index 6f952b39e..8f6ee835f 100644
--- a/lib/SimpleSAML/SessionHandlerPHP.php
+++ b/lib/SimpleSAML/SessionHandlerPHP.php
@@ -147,38 +147,17 @@ class SimpleSAML_SessionHandlerPHP extends SimpleSAML_SessionHandler
/**
- * Create and set new session id.
+ * Create a new session id.
*
* @return string The new session id.
- *
- * @throws SimpleSAML_Error_Exception If the cookie is marked as secure but we are not using HTTPS, or the headers
- * were already sent and therefore we cannot set the cookie.
*/
public function newSessionId()
{
- $session_cookie_params = session_get_cookie_params();
-
- if ($session_cookie_params['secure'] && !\SimpleSAML\Utils\HTTP::isHTTPS()) {
- throw new SimpleSAML_Error_Exception('Session start with secure cookie not allowed on http.');
- }
-
- if (headers_sent()) {
- throw new SimpleSAML_Error_Exception('Cannot create new session - headers already sent.');
- }
-
// generate new (secure) session id
$sessionId = bin2hex(openssl_random_pseudo_bytes(16));
SimpleSAML_Session::createSession($sessionId);
- if (session_id() !== '') {
- // session already started, close it
- session_write_close();
- }
-
- session_id($sessionId);
- $this->sessionStart();
-
- return session_id();
+ return $sessionId;
}
@@ -321,4 +300,45 @@ class SimpleSAML_SessionHandlerPHP extends SimpleSAML_SessionHandler
return $ret;
}
+
+
+ /**
+ * Set a session cookie.
+ *
+ * @param string $sessionName The name of the session.
+ * @param string|null $sessionID The session ID to use. Set to null to delete the cookie.
+ * @param array|null $cookieParams Additional parameters to use for the session cookie.
+ *
+ * @throws \SimpleSAML\Error\CannotSetCookie If we can't set the cookie.
+ */
+ public function setCookie($sessionName, $sessionID, array $cookieParams = null)
+ {
+ if ($cookieParams === null) {
+ $cookieParams = session_get_cookie_params();
+ }
+
+ if ($cookieParams['secure'] && !\SimpleSAML\Utils\HTTP::isHTTPS()) {
+ throw new SimpleSAML\Error\CannotSetCookie('Secure cookies not allowed on http.');
+ }
+
+ if (headers_sent()) {
+ throw new SimpleSAML\Error\CannotSetCookie('Headers already sent.');
+ }
+
+ session_set_cookie_params(
+ $cookieParams['lifetime'],
+ $cookieParams['path'],
+ $cookieParams['domain'],
+ $cookieParams['secure'],
+ $cookieParams['httponly']
+ );
+
+ if (session_id() !== '') {
+ // session already started, close it
+ session_write_close();
+ }
+
+ session_id($sessionID);
+ $this->sessionStart();
+ }
}
--
GitLab