diff --git a/src/Core/Auth/Authenticator.php b/src/Core/Auth/Authenticator.php index 250a166c..f15a057a 100644 --- a/src/Core/Auth/Authenticator.php +++ b/src/Core/Auth/Authenticator.php @@ -132,6 +132,7 @@ class Authenticator { $rCrypt = self::hashPassword($rData['password']); $db->query('UPDATE `users` SET `password` = ?, `last_login` = UNIX_TIMESTAMP(), `ip` = ? WHERE `id` = ?;', $rCrypt, $rIP, $rUserInfo['id']); + self::renewSessionId(); $_SESSION['hash'] = $rUserInfo['id']; $_SESSION['ip'] = $rIP; $_SESSION['code'] = AuthRepository::getCurrentCode(); @@ -197,6 +198,7 @@ class Authenticator { $rCrypt = self::hashPassword($rData['password']); $db->query('UPDATE `users` SET `password` = ?, `last_login` = UNIX_TIMESTAMP(), `ip` = ? WHERE `id` = ?;', $rCrypt, $rIP, $rUserInfo['id']); + self::renewSessionId(); $_SESSION['reseller'] = $rUserInfo['id']; $_SESSION['rip'] = $rIP; $_SESSION['rcode'] = AuthRepository::getCurrentCode(); @@ -218,6 +220,20 @@ class Authenticator { return array('status' => STATUS_FAILURE); } + /** + * Move a session that has just signed in onto a fresh id, discarding the old + * one. The id the visitor arrived with is one someone else may know — a cookie + * planted from a sibling subdomain, a shared machine — and keeping it would + * sign them in too (session fixation). + * + * @return void + */ + private static function renewSessionId(): void { + if (session_status() === PHP_SESSION_ACTIVE) { + session_regenerate_id(true); + } + } + /** * Hash a password with a salted, multi-round digest. * diff --git a/src/Public/Views/admin/setup.php b/src/Public/Views/admin/setup.php index 33d8ea30..c324208d 100644 --- a/src/Public/Views/admin/setup.php +++ b/src/Public/Views/admin/setup.php @@ -77,6 +77,7 @@ if (!RequestManager::has('update')): $rQuery = 'INSERT INTO `users`(' . $rPrepare['columns'] . ') VALUES(' . $rPrepare['placeholder'] . ');'; if ($db->query($rQuery, ...$rPrepare['data'])) { + session_regenerate_id(true); // signed in from here: a fresh id, as at login $_SESSION['hash'] = $db->last_insert_id(); $_SESSION['ip'] = NetworkUtils::getUserIP(); $_SESSION['code'] = AuthRepository::getCurrentCode(); diff --git a/src/bootstrap.php b/src/bootstrap.php index cec6d25e..42794b61 100644 --- a/src/bootstrap.php +++ b/src/bootstrap.php @@ -424,7 +424,13 @@ class XC_Bootstrap { if (session_status() === PHP_SESSION_NONE) { $rParams = session_get_cookie_params() ?: []; $rParams['samesite'] = 'Strict'; + // The panel's scripts never read the session cookie, so an XSS must + // not be able to either. + $rParams['httponly'] = true; session_set_cookie_params($rParams); + // Refuse session ids this server never issued, so a visitor cannot + // arrive carrying one an attacker chose. + ini_set('session.use_strict_mode', '1'); session_start(); } diff --git a/tests/Unit/LoginSessionFixationTest.php b/tests/Unit/LoginSessionFixationTest.php new file mode 100644 index 00000000..e57953ad --- /dev/null +++ b/tests/Unit/LoginSessionFixationTest.php @@ -0,0 +1,112 @@ +dbh = true; + } + + public function query($query, $buffered = false) { + $this->rows = []; + if (str_contains($query, 'FROM `users` WHERE `username` = ?')) { + $this->rows = [$this->user]; + } elseif (str_contains($query, 'FROM `access_codes` WHERE `code` = ?')) { + $this->rows = [['id' => 1, 'code' => 'panel', 'groups' => json_encode([$this->user['member_group_id']])]]; + } elseif (str_contains($query, 'COUNT(*) AS `count` FROM `access_codes`')) { + $this->rows = [['count' => 1]]; + } elseif (str_contains($query, 'FROM `users_groups` WHERE `group_id` = ?')) { + $this->rows = [$this->group]; + } + return true; + } + + public function num_rows() { + return count($this->rows); + } + + public function get_row() { + return $this->rows[0] ?? []; + } +} + +/** + * A session id the visitor brought to the login form must not be the one that + * carries the signed-in session: whoever planted it (a sibling-subdomain + * cookie, a shared kiosk) would be signed in too. + * + * Each test runs in its own process: PHP refuses to set a session id once + * output has started, and a runner prints between tests. + */ +#[RunTestsInSeparateProcesses] +class LoginSessionFixationTest extends TestCase { + private LoginScriptedDb $db; + + protected function setUp(): void { + foreach (['STATUS_FAILURE' => 0, 'STATUS_SUCCESS' => 1, 'STATUS_DISABLED' => 5, 'STATUS_NOT_ADMIN' => 6, 'STATUS_INVALID_CAPTCHA' => 12, 'STATUS_INVALID_CODE' => 13, 'STATUS_NOT_RESELLER' => 35] as $rName => $rValue) { + if (!defined($rName)) { + define($rName, $rValue); + } + } + $_SERVER['XC_CODE'] = 'panel'; + $_SERVER['REMOTE_ADDR'] = '192.0.2.10'; + $GLOBALS['rSettings'] = ['recaptcha_enable' => 0, 'save_login_logs' => 0]; + + $this->db = new LoginScriptedDb(); + $this->db->user = ['id' => 4, 'username' => 'boss', 'password' => Authenticator::hashPassword('s3cret', 'fixedsalt', 1000), 'member_group_id' => 1, 'status' => 1]; + $GLOBALS['db'] = $this->db; + DatabaseFactory::set($this->db); + + if (session_status() === PHP_SESSION_ACTIVE) { + session_write_close(); + } + ini_set('session.save_path', sys_get_temp_dir()); + session_id('plantedbyattacker0123456789'); + @session_start(); + } + + protected function tearDown(): void { + @session_destroy(); + } + + public function testAdminLoginIssuesAFreshSessionId(): void { + $this->db->group = ['group_id' => 1, 'is_admin' => 1, 'is_reseller' => 0, 'subresellers' => '']; + + $rRes = Authenticator::login(['username' => 'boss', 'password' => 's3cret'], true); + + $this->assertSame(STATUS_SUCCESS, $rRes['status']); + $this->assertSame(4, $_SESSION['hash']); + $this->assertTrue(session_id() !== 'plantedbyattacker0123456789', 'the planted session id survived the login'); + } + + public function testResellerLoginIssuesAFreshSessionId(): void { + $this->db->group = ['group_id' => 1, 'is_admin' => 0, 'is_reseller' => 1, 'subresellers' => '']; + + $rRes = Authenticator::resellerLogin(['username' => 'boss', 'password' => 's3cret']); + + $this->assertSame(STATUS_SUCCESS, $rRes['status']); + $this->assertSame(4, $_SESSION['reseller']); + $this->assertTrue(session_id() !== 'plantedbyattacker0123456789', 'the planted session id survived the login'); + } + + /** A failed login changes nothing about the session. */ + public function testFailedLoginKeepsTheSession(): void { + $this->db->group = ['group_id' => 1, 'is_admin' => 1, 'is_reseller' => 0, 'subresellers' => '']; + + $rRes = Authenticator::login(['username' => 'boss', 'password' => 'wrong'], true); + + $this->assertSame(STATUS_FAILURE, $rRes['status']); + $this->assertArrayNotHasKey('hash', $_SESSION); + } +}