From 64d97d0787a63b3c646f89237574ac566ed89c50 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Mon, 27 Oct 2014 19:55:56 -0700 Subject: [PATCH 1/4] [ticket/13234] Never allow autologin/remember me to modify the userid This prevents admin relogin with forced user id from overwriting remember me cookies PHPBB3-13234 --- phpBB/includes/session.php | 71 +++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 31 deletions(-) diff --git a/phpBB/includes/session.php b/phpBB/includes/session.php index 4c13a4f558..fcc6745021 100644 --- a/phpBB/includes/session.php +++ b/phpBB/includes/session.php @@ -553,6 +553,45 @@ class session $method = basename(trim($config['auth_method'])); include_once($phpbb_root_path . 'includes/auth/auth_' . $method . '.' . $phpEx); + $method = 'autologin_' . $method; + if (function_exists($method)) + { + $user_data = $method(); + + if ($user_id === false || (isset($user_data['user_id']) && $user_id = $user_data['user_id'])) + { + $this->data = $user_data; + } + + if (sizeof($this->data)) + { + $this->cookie_data['k'] = ''; + $this->cookie_data['u'] = $this->data['user_id']; + } + } + + // If we're presented with an autologin key we'll join against it. + // Else if we've been passed a user_id we'll grab data based on that + if (isset($this->cookie_data['k']) && $this->cookie_data['k'] && $this->cookie_data['u'] && !sizeof($this->data)) + { + $sql = 'SELECT u.* + FROM ' . USERS_TABLE . ' u, ' . SESSIONS_KEYS_TABLE . ' k + WHERE u.user_id = ' . (int) $this->cookie_data['u'] . ' + AND u.user_type IN (' . USER_NORMAL . ', ' . USER_FOUNDER . ") + AND k.user_id = u.user_id + AND k.key_id = '" . $db->sql_escape(md5($this->cookie_data['k'])) . "'"; + $result = $db->sql_query($sql); + $user_data = $db->sql_fetchrow($result); + + if ($user_id === false || (isset($user_data['user_id']) && $user_id = $user_data['user_id'])) + { + $this->data = $user_data; + $bot = false; + } + + $db->sql_freeresult($result); + } + if ($user_id !== false && !sizeof($this->data)) { $this->cookie_data['k'] = ''; @@ -567,36 +606,6 @@ class session $db->sql_freeresult($result); $bot = false; } - else if (!$bot) - { - $method = 'autologin_' . $method; - if (function_exists($method)) - { - $this->data = $method(); - - if (sizeof($this->data)) - { - $this->cookie_data['k'] = ''; - $this->cookie_data['u'] = $this->data['user_id']; - } - } - - // If we're presented with an autologin key we'll join against it. - // Else if we've been passed a user_id we'll grab data based on that - if (isset($this->cookie_data['k']) && $this->cookie_data['k'] && $this->cookie_data['u'] && !sizeof($this->data)) - { - $sql = 'SELECT u.* - FROM ' . USERS_TABLE . ' u, ' . SESSIONS_KEYS_TABLE . ' k - WHERE u.user_id = ' . (int) $this->cookie_data['u'] . ' - AND u.user_type IN (' . USER_NORMAL . ', ' . USER_FOUNDER . ") - AND k.user_id = u.user_id - AND k.key_id = '" . $db->sql_escape(md5($this->cookie_data['k'])) . "'"; - $result = $db->sql_query($sql); - $this->data = $db->sql_fetchrow($result); - $db->sql_freeresult($result); - $bot = false; - } - } // Bot user, if they have a SID in the Request URI we need to get rid of it // otherwise they'll index this page with the SID, duplicate content oh my! @@ -2459,4 +2468,4 @@ class user extends session } } -?> \ No newline at end of file +?> From 3472b6c5bc44b3ca13b6bb3fadc7aef7d7b41f1e Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Mon, 27 Oct 2014 19:57:53 -0700 Subject: [PATCH 2/4] [ticket/13234-2] Never allow autologin/remember me to modify the userid Ascraeus version of 64d97d0787a63b3c646f89237574ac566ed89c50 commit PHPBB3-13234 --- phpBB/phpbb/session.php | 67 +++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 29 deletions(-) diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index cf8ea1877e..c787247b44 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -577,6 +577,43 @@ class session } } + $provider_collection = $phpbb_container->get('auth.provider_collection'); + $provider = $provider_collection->get_provider(); + $this->data = $provider->autologin(); + + if ($user_id !== false && sizeof($this->data) && $this->data['user_id'] != $user_id) + { + $this->data = array(); + } + + if (sizeof($this->data)) + { + $this->cookie_data['k'] = ''; + $this->cookie_data['u'] = $this->data['user_id']; + } + + // If we're presented with an autologin key we'll join against it. + // Else if we've been passed a user_id we'll grab data based on that + if (isset($this->cookie_data['k']) && $this->cookie_data['k'] && $this->cookie_data['u'] && !sizeof($this->data)) + { + $sql = 'SELECT u.* + FROM ' . USERS_TABLE . ' u, ' . SESSIONS_KEYS_TABLE . ' k + WHERE u.user_id = ' . (int) $this->cookie_data['u'] . ' + AND u.user_type IN (' . USER_NORMAL . ', ' . USER_FOUNDER . ") + AND k.user_id = u.user_id + AND k.key_id = '" . $db->sql_escape(md5($this->cookie_data['k'])) . "'"; + $result = $db->sql_query($sql); + $user_data = $db->sql_fetchrow($result); + + if ($user_id === false || $user_id == $user_data['user_id']) + { + $this->data = $user_data; + $bot = false; + } + + $db->sql_freeresult($result); + } + if ($user_id !== false && !sizeof($this->data)) { $this->cookie_data['k'] = ''; @@ -591,34 +628,6 @@ class session $db->sql_freeresult($result); $bot = false; } - else if (!$bot) - { - $provider_collection = $phpbb_container->get('auth.provider_collection'); - $provider = $provider_collection->get_provider(); - $this->data = $provider->autologin(); - - if (sizeof($this->data)) - { - $this->cookie_data['k'] = ''; - $this->cookie_data['u'] = $this->data['user_id']; - } - - // If we're presented with an autologin key we'll join against it. - // Else if we've been passed a user_id we'll grab data based on that - if (isset($this->cookie_data['k']) && $this->cookie_data['k'] && $this->cookie_data['u'] && !sizeof($this->data)) - { - $sql = 'SELECT u.* - FROM ' . USERS_TABLE . ' u, ' . SESSIONS_KEYS_TABLE . ' k - WHERE u.user_id = ' . (int) $this->cookie_data['u'] . ' - AND u.user_type IN (' . USER_NORMAL . ', ' . USER_FOUNDER . ") - AND k.user_id = u.user_id - AND k.key_id = '" . $db->sql_escape(md5($this->cookie_data['k'])) . "'"; - $result = $db->sql_query($sql); - $this->data = $db->sql_fetchrow($result); - $db->sql_freeresult($result); - $bot = false; - } - } // Bot user, if they have a SID in the Request URI we need to get rid of it // otherwise they'll index this page with the SID, duplicate content oh my! @@ -926,7 +935,7 @@ class session } // Reset the data array - $this->data = array(); + $this->data = false; $sql = 'SELECT * FROM ' . USERS_TABLE . ' From 087a5363bba7228f29d6075deb2a099cf9d25630 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Mon, 27 Oct 2014 23:56:20 -0700 Subject: [PATCH 3/4] [ticket/13234-2] Correctly verify that user_id is set in user data array PHPBB3-13234-2 --- phpBB/phpbb/session.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index c787247b44..477e91efd6 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -605,7 +605,7 @@ class session $result = $db->sql_query($sql); $user_data = $db->sql_fetchrow($result); - if ($user_id === false || $user_id == $user_data['user_id']) + if ($user_id === false || (isset($user_data['user_id']) && $user_id == $user_data['user_id'])) { $this->data = $user_data; $bot = false; @@ -935,7 +935,7 @@ class session } // Reset the data array - $this->data = false; + $this->data = array(); $sql = 'SELECT * FROM ' . USERS_TABLE . ' From fcc320e3852215a11b863d0108e16e2be998d5cc Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Tue, 28 Oct 2014 10:14:47 +0100 Subject: [PATCH 4/4] [ticket/13234] Fix conditions and CS PHPBB3-13234 --- phpBB/includes/session.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/session.php b/phpBB/includes/session.php index fcc6745021..5b2c4f0b0d 100644 --- a/phpBB/includes/session.php +++ b/phpBB/includes/session.php @@ -558,7 +558,7 @@ class session { $user_data = $method(); - if ($user_id === false || (isset($user_data['user_id']) && $user_id = $user_data['user_id'])) + if ($user_id === false || (isset($user_data['user_id']) && $user_id == $user_data['user_id'])) { $this->data = $user_data; } @@ -583,7 +583,7 @@ class session $result = $db->sql_query($sql); $user_data = $db->sql_fetchrow($result); - if ($user_id === false || (isset($user_data['user_id']) && $user_id = $user_data['user_id'])) + if ($user_id === false || (isset($user_data['user_id']) && $user_id == $user_data['user_id'])) { $this->data = $user_data; $bot = false;