fix(session): fix session security

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,33 +51,137 @@ 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
      *
      * @return 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;
     }
 }

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;
         }
 

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;

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,
+        ]);
     }
 
     /**

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);
     }