Skip to content

Commit 396cec4

Browse files
papacpapac
authored
Merge pull request #408 from bowphp/refactor/code-base
fix(session): fix session security
2 parents 619dd63 + 03f6055 commit 396cec4

5 files changed

Lines changed: 179 additions & 30 deletions

File tree

‎src/Security/Crypto.php‎

Lines changed: 128 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
namespace Bow\Security;
66

7-
use Bow\Support\Str;
7+
use RuntimeException;
88

99
class Crypto
1010
{
@@ -22,6 +22,19 @@ class Crypto
2222
*/
2323
private static string $cipher = 'AES-256-CBC';
2424

25+
/**
26+
* Header tagging the authenticated (random-IV + HMAC) payload format.
27+
*
28+
* The ':' is not part of the base64 alphabet, so a value carrying this
29+
* prefix can never be confused with a legacy (base64-only) ciphertext.
30+
*/
31+
private const HEADER = 'BOW2:';
32+
33+
/**
34+
* The authentication tag length in bytes (HMAC-SHA256).
35+
*/
36+
private const MAC_LENGTH = 32;
37+
2538
/**
2639
* Set the key
2740
*
@@ -38,33 +51,137 @@ public static function setKey(string $key, ?string $cipher = null): void
3851
}
3952

4053
/**
41-
* Encrypt data
54+
* Encrypt data.
55+
*
56+
* Produces an authenticated payload: a fresh random IV is used for every
57+
* call (so identical plaintexts yield different ciphertexts) and an
58+
* encrypt-then-MAC HMAC-SHA256 tag protects against tampering.
4259
*
4360
* @param string $data
44-
* @return string|bool
61+
* @return string
4562
*/
46-
public static function encrypt(string $data): string|bool
63+
public static function encrypt(string $data): string
4764
{
48-
$iv_size = openssl_cipher_iv_length(static::$cipher);
65+
$key = static::resolveKey();
66+
67+
$iv_size = (int) openssl_cipher_iv_length(static::$cipher);
68+
$iv = random_bytes($iv_size);
69+
70+
$cipher_text = openssl_encrypt(
71+
$data,
72+
static::$cipher,
73+
static::deriveKey('enc', $key),
74+
OPENSSL_RAW_DATA,
75+
$iv
76+
);
77+
78+
if ($cipher_text === false) {
79+
throw new RuntimeException('Unable to encrypt the given data.');
80+
}
4981

50-
$iv = Str::slice(sha1(static::$key), 0, $iv_size);
82+
$mac = hash_hmac('sha256', $iv . $cipher_text, static::deriveKey('auth', $key), true);
5183

52-
return openssl_encrypt($data, static::$cipher, static::$key, 0, $iv);
84+
return self::HEADER . base64_encode($iv . $mac . $cipher_text);
5385
}
5486

5587
/**
56-
* decrypt
88+
* Decrypt data.
89+
*
90+
* Authenticated payloads are verified before decryption and fail closed
91+
* (return false) on a bad tag, truncation or wrong key. Values produced by
92+
* the previous unauthenticated format are still readable for backward
93+
* compatibility.
5794
*
5895
* @param string $data
5996
*
6097
* @return string|bool
6198
*/
6299
public static function decrypt(string $data): string|bool
63100
{
64-
$iv_size = openssl_cipher_iv_length(static::$cipher);
101+
$key = static::resolveKey();
102+
103+
if (!str_starts_with($data, self::HEADER)) {
104+
return static::decryptLegacy($data, $key);
105+
}
106+
107+
$raw = base64_decode(substr($data, strlen(self::HEADER)), true);
108+
109+
if ($raw === false) {
110+
return false;
111+
}
112+
113+
$iv_size = (int) openssl_cipher_iv_length(static::$cipher);
114+
115+
if (strlen($raw) <= $iv_size + self::MAC_LENGTH) {
116+
return false;
117+
}
118+
119+
$iv = substr($raw, 0, $iv_size);
120+
$mac = substr($raw, $iv_size, self::MAC_LENGTH);
121+
$cipher_text = substr($raw, $iv_size + self::MAC_LENGTH);
122+
123+
$calculated = hash_hmac('sha256', $iv . $cipher_text, static::deriveKey('auth', $key), true);
124+
125+
// Reject tampered or wrong-key payloads before touching the cipher.
126+
if (!hash_equals($calculated, $mac)) {
127+
return false;
128+
}
129+
130+
return openssl_decrypt(
131+
$cipher_text,
132+
static::$cipher,
133+
static::deriveKey('enc', $key),
134+
OPENSSL_RAW_DATA,
135+
$iv
136+
);
137+
}
138+
139+
/**
140+
* Decrypt a value produced by the legacy (static IV, unauthenticated)
141+
* format. Kept only so data encrypted before the upgrade keeps working.
142+
*
143+
* @param string $data
144+
* @param string $key
145+
* @return string|bool
146+
*/
147+
private static function decryptLegacy(string $data, string $key): string|bool
148+
{
149+
$iv_size = (int) openssl_cipher_iv_length(static::$cipher);
150+
151+
$iv = substr(sha1($key), 0, $iv_size);
152+
153+
return openssl_decrypt($data, static::$cipher, $key, 0, $iv);
154+
}
65155

66-
$iv = Str::slice(sha1(static::$key), 0, $iv_size);
156+
/**
157+
* Derive a purpose-specific 256-bit subkey from the configured key.
158+
*
159+
* Separating the encryption key from the authentication key (domain
160+
* separation) is required for encrypt-then-MAC to be sound, and normalises
161+
* an arbitrary-length configured key to a fixed strong key.
162+
*
163+
* @param string $context
164+
* @param string $key
165+
* @return string
166+
*/
167+
private static function deriveKey(string $context, string $key): string
168+
{
169+
return hash_hmac('sha256', 'BowCrypto|v2|' . $context, $key, true);
170+
}
171+
172+
/**
173+
* Resolve the configured key or fail loudly when it is missing.
174+
*
175+
* @return string
176+
*/
177+
private static function resolveKey(): string
178+
{
179+
if (static::$key === null || static::$key === '') {
180+
throw new RuntimeException(
181+
'The application security key is not set. Define security.key before using Crypto.'
182+
);
183+
}
67184

68-
return openssl_decrypt($data, static::$cipher, static::$key, 0, $iv);
185+
return static::$key;
69186
}
70187
}

‎src/Security/Tokenize.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,8 @@ public static function verify(string $token, bool $strict = false): bool
9494

9595
$csrf = Session::getInstance()->get('__bow.csrf');
9696

97-
if ($token !== $csrf['token']) {
97+
// Constant-time comparison to avoid leaking the token via timing.
98+
if (!hash_equals((string) $csrf['token'], $token)) {
9899
return false;
99100
}
100101

‎src/Session/Adapters/FilesystemAdapter.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,9 @@ public function gc(int $maxlifetime): int|false
9090
public function open(string $path, string $name): bool
9191
{
9292
if (!is_dir($this->save_path)) {
93-
mkdir($this->save_path);
93+
// 0700: session files contain authentication state and must not be
94+
// readable by other users on a shared host.
95+
mkdir($this->save_path, 0700, true);
9496
}
9597

9698
return true;

‎src/Session/Session.php‎

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66

77
use BadMethodCallException;
88
use Bow\Contracts\CollectionInterface;
9-
use Bow\Security\Crypto;
109
use Bow\Session\Adapters\ArrayAdapter;
1110
use Bow\Session\Adapters\DatabaseAdapter;
1211
use Bow\Session\Adapters\FilesystemAdapter;
@@ -75,7 +74,11 @@ private function __construct(array $config)
7574
'path' => '/',
7675
'domain' => null,
7776
'secure' => false,
78-
'httponly' => false,
77+
// Keep the session cookie out of reach of JavaScript by default
78+
// (mitigates session theft via XSS) and constrain cross-site
79+
// sending (mitigates CSRF).
80+
'httponly' => true,
81+
'samesite' => 'Lax',
7982
'save_path' => null,
8083
],
8184
$config
@@ -115,6 +118,14 @@ public static function getInstance(): ?Session
115118
*/
116119
public function regenerate(): void
117120
{
121+
$this->start();
122+
123+
// Rotate the underlying session ID and delete the previous record so a
124+
// fixated/leaked ID can no longer be reused, then clear the values.
125+
if (PHP_SESSION_ACTIVE === session_status()) {
126+
session_regenerate_id(true);
127+
}
128+
118129
$this->flush();
119130
$this->start();
120131
}
@@ -162,11 +173,17 @@ public function start(): bool
162173
*/
163174
private function initializeDriver(): void
164175
{
176+
// Reject uninitialized session IDs (anti session-fixation) and never
177+
// accept the session ID from the URL/query string. Must be set before
178+
// session_start().
179+
@ini_set('session.use_strict_mode', '1');
180+
@ini_set('session.use_only_cookies', '1');
181+
165182
// We Apply session cookie name
166183
@session_name($this->config['name']);
167184

168185
if (!isset($_COOKIE[$this->config['name']])) {
169-
@session_id(hash("sha256", $this->generateId()));
186+
@session_id($this->generateId());
170187
}
171188

172189
// We create get driver
@@ -204,13 +221,17 @@ private function initializeDriver(): void
204221
}
205222

206223
/**
207-
* Generate session ID
224+
* Generate a cryptographically secure session ID.
225+
*
226+
* Uses the CSPRNG (random_bytes) instead of time-based primitives such as
227+
* uniqid()/microtime(), which are predictable and would let an attacker
228+
* guess or fixate session identifiers.
208229
*
209230
* @return string
210231
*/
211232
private function generateId(): string
212233
{
213-
return Crypto::encrypt(uniqid(microtime()));
234+
return bin2hex(random_bytes(32));
214235
}
215236

216237
/**
@@ -223,14 +244,16 @@ private function setCookieParameters(): void
223244
$domain = $this->config['domain'] ?? null;
224245
$secure = (bool)$this->config["secure"];
225246
$httponly = (bool)$this->config["httponly"];
226-
227-
session_set_cookie_params(
228-
(int)$this->config["lifetime"],
229-
$this->config["path"],
230-
$domain,
231-
$secure,
232-
$httponly
233-
);
247+
$samesite = $this->config["samesite"] ?? 'Lax';
248+
249+
session_set_cookie_params([
250+
'lifetime' => (int)$this->config["lifetime"],
251+
'path' => $this->config["path"],
252+
'domain' => $domain,
253+
'secure' => $secure,
254+
'httponly' => $httponly,
255+
'samesite' => $samesite,
256+
]);
234257
}
235258

236259
/**

‎src/Support/helpers.php‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -665,7 +665,10 @@ function collect(array $data = []): Collection
665665

666666
if (!function_exists('encrypt')) {
667667
/**
668-
* Encrypt data
668+
* Encrypt data using the application security key.
669+
*
670+
* Returns an authenticated payload (random IV + HMAC), so encrypting the
671+
* same value twice yields different ciphertexts.
669672
*
670673
* @param string $data
671674
* @return string
@@ -678,12 +681,15 @@ function encrypt(string $data): string
678681

679682
if (!function_exists('decrypt')) {
680683
/**
681-
* Decrypt data
684+
* Decrypt a value previously produced by encrypt().
685+
*
686+
* Fails closed: returns false when the payload has been tampered with or
687+
* was encrypted with a different key.
682688
*
683689
* @param string $data
684-
* @return string
690+
* @return string|bool
685691
*/
686-
function decrypt(string $data): string
692+
function decrypt(string $data): string|bool
687693
{
688694
return Crypto::decrypt($data);
689695
}

0 commit comments

Comments
 (0)