From 03f6055802771663321bd6685e8d944150673f61 Mon Sep 17 00:00:00 2001 From: Franck DAKIA Date: Thu, 4 Jun 2026 23:54:44 +0000 Subject: [PATCH] fix(session): fix session security --- src/Security/Crypto.php | 139 +++++++++++++++++++-- src/Security/Tokenize.php | 3 +- src/Session/Adapters/FilesystemAdapter.php | 4 +- src/Session/Session.php | 49 ++++++-- src/Support/helpers.php | 14 ++- 5 files changed, 179 insertions(+), 30 deletions(-) diff --git a/src/Security/Crypto.php b/src/Security/Crypto.php index 5d19f9ca..27a2dd25 100644 --- a/src/Security/Crypto.php +++ b/src/Security/Crypto.php @@ -4,7 +4,7 @@ namespace Bow\Security; -use Bow\Support\Str; +use RuntimeException; class Crypto { @@ -22,6 +22,19 @@ class Crypto */ private static string $cipher = 'AES-256-CBC'; + /** + * Header tagging the authenticated (random-IV + HMAC) payload format. + * + * The ':' is not part of the base64 alphabet, so a value carrying this + * prefix can never be confused with a legacy (base64-only) ciphertext. + */ + private const HEADER = 'BOW2:'; + + /** + * The authentication tag length in bytes (HMAC-SHA256). + */ + private const MAC_LENGTH = 32; + /** * Set the key * @@ -38,22 +51,46 @@ public static function setKey(string $key, ?string $cipher = null): void } /** - * Encrypt data + * Encrypt data. + * + * Produces an authenticated payload: a fresh random IV is used for every + * call (so identical plaintexts yield different ciphertexts) and an + * encrypt-then-MAC HMAC-SHA256 tag protects against tampering. * * @param string $data - * @return string|bool + * @return string */ - public static function encrypt(string $data): string|bool + public static function encrypt(string $data): string { - $iv_size = openssl_cipher_iv_length(static::$cipher); + $key = static::resolveKey(); + + $iv_size = (int) openssl_cipher_iv_length(static::$cipher); + $iv = random_bytes($iv_size); + + $cipher_text = openssl_encrypt( + $data, + static::$cipher, + static::deriveKey('enc', $key), + OPENSSL_RAW_DATA, + $iv + ); + + if ($cipher_text === false) { + throw new RuntimeException('Unable to encrypt the given data.'); + } - $iv = Str::slice(sha1(static::$key), 0, $iv_size); + $mac = hash_hmac('sha256', $iv . $cipher_text, static::deriveKey('auth', $key), true); - return openssl_encrypt($data, static::$cipher, static::$key, 0, $iv); + return self::HEADER . base64_encode($iv . $mac . $cipher_text); } /** - * decrypt + * Decrypt data. + * + * Authenticated payloads are verified before decryption and fail closed + * (return false) on a bad tag, truncation or wrong key. Values produced by + * the previous unauthenticated format are still readable for backward + * compatibility. * * @param string $data * @@ -61,10 +98,90 @@ public static function encrypt(string $data): string|bool */ public static function decrypt(string $data): string|bool { - $iv_size = openssl_cipher_iv_length(static::$cipher); + $key = static::resolveKey(); + + if (!str_starts_with($data, self::HEADER)) { + return static::decryptLegacy($data, $key); + } + + $raw = base64_decode(substr($data, strlen(self::HEADER)), true); + + if ($raw === false) { + return false; + } + + $iv_size = (int) openssl_cipher_iv_length(static::$cipher); + + if (strlen($raw) <= $iv_size + self::MAC_LENGTH) { + return false; + } + + $iv = substr($raw, 0, $iv_size); + $mac = substr($raw, $iv_size, self::MAC_LENGTH); + $cipher_text = substr($raw, $iv_size + self::MAC_LENGTH); + + $calculated = hash_hmac('sha256', $iv . $cipher_text, static::deriveKey('auth', $key), true); + + // Reject tampered or wrong-key payloads before touching the cipher. + if (!hash_equals($calculated, $mac)) { + return false; + } + + return openssl_decrypt( + $cipher_text, + static::$cipher, + static::deriveKey('enc', $key), + OPENSSL_RAW_DATA, + $iv + ); + } + + /** + * Decrypt a value produced by the legacy (static IV, unauthenticated) + * format. Kept only so data encrypted before the upgrade keeps working. + * + * @param string $data + * @param string $key + * @return string|bool + */ + private static function decryptLegacy(string $data, string $key): string|bool + { + $iv_size = (int) openssl_cipher_iv_length(static::$cipher); + + $iv = substr(sha1($key), 0, $iv_size); + + return openssl_decrypt($data, static::$cipher, $key, 0, $iv); + } - $iv = Str::slice(sha1(static::$key), 0, $iv_size); + /** + * Derive a purpose-specific 256-bit subkey from the configured key. + * + * Separating the encryption key from the authentication key (domain + * separation) is required for encrypt-then-MAC to be sound, and normalises + * an arbitrary-length configured key to a fixed strong key. + * + * @param string $context + * @param string $key + * @return string + */ + private static function deriveKey(string $context, string $key): string + { + return hash_hmac('sha256', 'BowCrypto|v2|' . $context, $key, true); + } + + /** + * Resolve the configured key or fail loudly when it is missing. + * + * @return string + */ + private static function resolveKey(): string + { + if (static::$key === null || static::$key === '') { + throw new RuntimeException( + 'The application security key is not set. Define security.key before using Crypto.' + ); + } - return openssl_decrypt($data, static::$cipher, static::$key, 0, $iv); + return static::$key; } } diff --git a/src/Security/Tokenize.php b/src/Security/Tokenize.php index ca67d079..a7b9b873 100644 --- a/src/Security/Tokenize.php +++ b/src/Security/Tokenize.php @@ -94,7 +94,8 @@ public static function verify(string $token, bool $strict = false): bool $csrf = Session::getInstance()->get('__bow.csrf'); - if ($token !== $csrf['token']) { + // Constant-time comparison to avoid leaking the token via timing. + if (!hash_equals((string) $csrf['token'], $token)) { return false; } diff --git a/src/Session/Adapters/FilesystemAdapter.php b/src/Session/Adapters/FilesystemAdapter.php index 0db41053..9860a0c4 100644 --- a/src/Session/Adapters/FilesystemAdapter.php +++ b/src/Session/Adapters/FilesystemAdapter.php @@ -90,7 +90,9 @@ public function gc(int $maxlifetime): int|false public function open(string $path, string $name): bool { if (!is_dir($this->save_path)) { - mkdir($this->save_path); + // 0700: session files contain authentication state and must not be + // readable by other users on a shared host. + mkdir($this->save_path, 0700, true); } return true; diff --git a/src/Session/Session.php b/src/Session/Session.php index c4d8eff5..19348927 100644 --- a/src/Session/Session.php +++ b/src/Session/Session.php @@ -6,7 +6,6 @@ use BadMethodCallException; use Bow\Contracts\CollectionInterface; -use Bow\Security\Crypto; use Bow\Session\Adapters\ArrayAdapter; use Bow\Session\Adapters\DatabaseAdapter; use Bow\Session\Adapters\FilesystemAdapter; @@ -75,7 +74,11 @@ private function __construct(array $config) 'path' => '/', 'domain' => null, 'secure' => false, - 'httponly' => false, + // Keep the session cookie out of reach of JavaScript by default + // (mitigates session theft via XSS) and constrain cross-site + // sending (mitigates CSRF). + 'httponly' => true, + 'samesite' => 'Lax', 'save_path' => null, ], $config @@ -115,6 +118,14 @@ public static function getInstance(): ?Session */ public function regenerate(): void { + $this->start(); + + // Rotate the underlying session ID and delete the previous record so a + // fixated/leaked ID can no longer be reused, then clear the values. + if (PHP_SESSION_ACTIVE === session_status()) { + session_regenerate_id(true); + } + $this->flush(); $this->start(); } @@ -162,11 +173,17 @@ public function start(): bool */ private function initializeDriver(): void { + // Reject uninitialized session IDs (anti session-fixation) and never + // accept the session ID from the URL/query string. Must be set before + // session_start(). + @ini_set('session.use_strict_mode', '1'); + @ini_set('session.use_only_cookies', '1'); + // We Apply session cookie name @session_name($this->config['name']); if (!isset($_COOKIE[$this->config['name']])) { - @session_id(hash("sha256", $this->generateId())); + @session_id($this->generateId()); } // We create get driver @@ -204,13 +221,17 @@ private function initializeDriver(): void } /** - * Generate session ID + * Generate a cryptographically secure session ID. + * + * Uses the CSPRNG (random_bytes) instead of time-based primitives such as + * uniqid()/microtime(), which are predictable and would let an attacker + * guess or fixate session identifiers. * * @return string */ private function generateId(): string { - return Crypto::encrypt(uniqid(microtime())); + return bin2hex(random_bytes(32)); } /** @@ -223,14 +244,16 @@ private function setCookieParameters(): void $domain = $this->config['domain'] ?? null; $secure = (bool)$this->config["secure"]; $httponly = (bool)$this->config["httponly"]; - - session_set_cookie_params( - (int)$this->config["lifetime"], - $this->config["path"], - $domain, - $secure, - $httponly - ); + $samesite = $this->config["samesite"] ?? 'Lax'; + + session_set_cookie_params([ + 'lifetime' => (int)$this->config["lifetime"], + 'path' => $this->config["path"], + 'domain' => $domain, + 'secure' => $secure, + 'httponly' => $httponly, + 'samesite' => $samesite, + ]); } /** diff --git a/src/Support/helpers.php b/src/Support/helpers.php index 0225cccd..4126b117 100644 --- a/src/Support/helpers.php +++ b/src/Support/helpers.php @@ -665,7 +665,10 @@ function collect(array $data = []): Collection if (!function_exists('encrypt')) { /** - * Encrypt data + * Encrypt data using the application security key. + * + * Returns an authenticated payload (random IV + HMAC), so encrypting the + * same value twice yields different ciphertexts. * * @param string $data * @return string @@ -678,12 +681,15 @@ function encrypt(string $data): string if (!function_exists('decrypt')) { /** - * Decrypt data + * Decrypt a value previously produced by encrypt(). + * + * Fails closed: returns false when the payload has been tampered with or + * was encrypted with a different key. * * @param string $data - * @return string + * @return string|bool */ - function decrypt(string $data): string + function decrypt(string $data): string|bool { return Crypto::decrypt($data); }