This can only happen when you share a session_id.
Under normal, proper site operations (^), that can only happen when you are issueing/using the same session_id while that session_id is already given.
Chances of that are very very small but not zero.
The change is increased when:
1) you have a huge backlog of used session records in your database (which is common in default OC) or file system. Remember, bots get a new session id on every request so that db table can fill pretty fast if not cleaned properly.
2) hackers who send requests with random/edited session cookies in the hope one is active, very rare, with virtually no chance but very difficult to counter (*).
3) Users who never close their browser and thus retain their session_id indefinitely which now may have been issued to someone else or some "remember me" implementation where the session cookie is no longer a session cookie but has a defined lifetime (*).
(*) many sites (incl OC) simply use the session_id presented in the cookie even if that session_id was not issued by the server nor whether that session is actually active on the server. As long as it is a valid format, that session_id is used.
(^) excludes unstable/experimental one-page checkout implementations
Item 1. you can counter by:
a) making sure your session garbage collection works properly which is the preferred method with the least performance impact.
b) checking whether a new session_id is already active before issueing/using it which adds extra effort, be it a minute one.
a)
For those sites we use the database for session storage better replace the file system/library/session/db.php
db = $registry->get('db');
$this->expire = ini_get('session.gc_maxlifetime') ?? 3600;
// session garbage collection using php.ini, defaults to 1% probability
$gc_probability = ini_get('session.gc_probability') ?? 1;
$gc_divisor = ini_get('session.gc_divisor') ?? 100;
if ((rand() % $gc_divisor) < $gc_probability) $this->gc();
}
public function read($session_id) {
$query = $this->db->query("SELECT data FROM " . DB_PREFIX . "session WHERE session_id = '" . $this->db->escape($session_id) . "'");
if ($query->num_rows) return json_decode($query->row['data'], true);
else return false;
}
public function write($session_id, $data) {
if ($session_id) $this->db->query("REPLACE INTO " . DB_PREFIX . "session SET session_id = '" . $this->db->escape($session_id) . "', data = '" . $this->db->escape(json_encode($data)) . "', expire = '" . $this->db->escape(date('Y-m-d H:i:s', time() + $this->expire)) . "'");
return true;
}
public function destroy($session_id) {
$this->db->query("DELETE FROM " . DB_PREFIX . "session WHERE session_id = '" . $this->db->escape($session_id) . "'");
return true;
}
public function gc() {
$dt = date('Y-m-d H:i:s',((int)time()));
$this->db->query("DELETE FROM " . DB_PREFIX . "session WHERE expire < '".$dt."'");
}
public function session_active ($session_id) {
$query = $this->db->query("SELECT session_id FROM " . DB_PREFIX . "session WHERE session_id = '" . $this->db->escape($session_id) . "'");
if (!$query->num_rows) return false;
return true;
}
}
That should take better care of the session garbage collection, the default gc() function is crap as it compares a timestamp with a date-time value apart from the fact that it is never called. The file storage class already has a __destruct() function and works fine.
b)
we changed the start function in system/library/session.php to:
public function start($session_id = false, $no_clash = true) {
if ($session_id) {
// determine if session id is valid format
if (!preg_match('/^[a-zA-Z0-9,\-]{22,52}$/', $session_id)) {
error_log('Error: invalid session ID given:'.$session_id.' by '.$_SERVER['REMOTE_ADDR'].', issueing a new one...');
$session_id = false;
}
}
if (!$session_id) {
// issue new session id
$session_id_active = true;
while ($session_id_active) {
// generate a new session id
if (function_exists('random_bytes')) $session_id = substr(bin2hex(random_bytes(32)), 0, 32);
else $session_id = substr(bin2hex(openssl_random_pseudo_bytes(32)), 0, 32);
// exit loop if no_clash is false or session id not active
if (!$no_clash) $session_id_active = false;
elseif (!$this->adaptor->session_active($session_id)) $session_id_active = false;
}
}
$this->session_id = $session_id;
$this->data = $this->adaptor->read($session_id);
return $session_id;
}
We added to system/library/session/db.php
public function session_active ($session_id) {
// determine if session_id is present in the session table
$this->db->query("SELECT session_id FROM " . DB_PREFIX . "session WHERE session_id = '" . $this->db->escape($session_id) . "'");
if (!$query->num_rows) return false;
return true;
}
We added to system/library/session/file.php
public function session_active ($session_id) {
// determine if session_id is present in the file system
$file = DIR_SESSION . 'sess_' . basename($session_id);
clearstatcache(true, $file);
if (!file_exists($file)) return false;
return true;
}
These function will check whether a newly generated session_id is already active server-side and generate a new one if it is.
Both for db and file stored sessions. You can toggle that check with the $no_clash parameter.
In addition, as stated (*), OC will use any session_id given to it via the cookie as long as it has the right format, which is not good.
We check whether the given session_id actually is active server-side or that given session_id is ignored.
catalog/controller/startup/session.php
change:
if (isset($_COOKIE[$this->config->get('session_name')])) {
$session_id = $_COOKIE[$this->config->get('session_name')];
} else {
$session_id = '';
}
$this->session->start($session_id);
to:
$session_id = '';
if (!empty($_COOKIE[$this->config->get('session_name')])) {
if (SESSION_VERIFY) {
if ($this->config->get('session_engine') == 'db') {
$query = $this->db->query("SELECT session_id FROM " . DB_PREFIX . "session WHERE session_id = '" . $this->db->escape($_COOKIE[$this->config->get('session_name')]) . "'");
if ($query->num_rows) $session_id = $_COOKIE[$this->config->get('session_name')];
} else {
$session_file = DIR_SESSION.'sess_'.$_COOKIE[$this->config->get('session_name')];
if (is_file($session_file)) $session_id = $_COOKIE[$this->config->get('session_name')];
}
} else {
$session_id = $_COOKIE[$this->config->get('session_name')];
}
}
$this->session->start($session_id,SESSION_NOCLASH);
system/framework.php
change:
if (isset($_COOKIE[$config->get('session_name')])) {
$session_id = $_COOKIE[$config->get('session_name')];
} else {
$session_id = '';
}
$this->session->start($session_id);
to:
$session_id = false;
if (isset($_COOKIE[$config->get('session_name')])) {
if (SESSION_VERIFY) {
if ($config->get('session_engine') == 'db') {
$query = $db->query("SELECT session_id FROM " . DB_PREFIX . "session WHERE session_id = '" . $db->escape($_COOKIE[$config->get('session_name')]) . "'");
if ($query->num_rows) $session_id = $_COOKIE[$config->get('session_name')];
} else {
$session_file = DIR_SESSION.'sess_'.$_COOKIE[$config->get('session_name')];
if (is_file($session_file)) $session_id = $_COOKIE[$config->get('session_name')];
}
} else {
$session_id = $_COOKIE[$config->get('session_name')];
}
}
$this->session->start($session_id,SESSION_NOCLASH);
change:
// Database
if ($config->get('db_autostart')) {
$registry->set('db', new DB($config->get('db_engine'), $config->get('db_hostname'), $config->get('db_username'), $config->get('db_password'), $config->get('db_database'), $config->get('db_port')));
}
to:
// Database
if ($config->get('db_autostart')) {
$db = new DB($config->get('db_engine'), $config->get('db_hostname'), $config->get('db_username'), $config->get('db_password'), $config->get('db_database'), $config->get('db_port'));
$registry->set('db', $db);
}
to be able to access the database from here.
(Turning verify on in framework and session controller makes the format check redundant as an invalid formatted session id would never be found in the database or file system)
define the switches in config
config.php
define('SESSION_VERIFY', true); // verify if the given session id exists server side
define('SESSION_NOCLASH', false); // prevent issueing new session id which already exists
As said, under proper operations, the chance of clashing sessions is very small but not zero and when it does happen, the impact/embarassment is rather large.
So make your own choice on whether the yield is worth the extra effort.
as mentioned, bots do not retain sessions and as such they invoke a new session generation on each request.
That is why the vast majority of your sessions in your session table or file system are for bots (you thought you had a lot of customers).
Since bots have no use for them beyond the lifetime of the script, those sessions can be removed immediately after script end.
(in the olden days it was common practice to not issue sessions to bots in the first place but that is not a good practice as the script itself and the content it produces relies on variables stored and used in the session during execution)
We do this by identifying public bots (those identifying themselves as such via the user agent, assuming everybody knows how to do that) and direct the session class to delete the session upon final session close (script end).
In catalog/controller/startup/startup.php we identify if the request comes from a public bot.
If it is, we set a session variable:
$this->data['force_remove_session'] = false;
if ($bot) $this->data['force_remove_session'] = true;
in system/library/session.php we change the shutdown function.
change:
public function close() {
$this->adaptor->write($this->session_id, $this->data);
}
to:
public function close() {
$this->adaptor->write($this->session_id, $this->data);
if (isset($this->data['force_remove_session']) && $this->data['force_remove_session']) {
$this->adaptor->destroy($this->session_id);
}
}
That way any session given to a bot is deleted after the script for that request ends and that will reduce the session table/directory to a large extend not waiting for the normal session garbage collection to kick in.
you can of course also just increase the character count of the session id from 32 to say 50.
That also reduces the chance of clashes but it still is not zero (just like the lottery) and as such it is more like an arms-race.
