From 0227030b8d35947fd1811f1b41bd1c9a58fc5b60 Mon Sep 17 00:00:00 2001 From: Franck DAKIA Date: Sun, 4 Oct 2026 06:26:35 +0000 Subject: [PATCH] Security hardening across query builder, storage, auth, mail and crypto - Database\QueryBuilder: bind whereBetween/whereNotBetween range values (SQL injection) and validate column/table identifiers in where*, join, select, aggregate, distinct, increment and insert/update keys through assertSafeIdentifier (wildcards allowed only where legitimate). - Storage\DiskFilesystemService::path(): confine resolved paths to the base directory and reject traversal (arbitrary read/write/delete). - Mail\Envelop: strip CR/LF/NUL from header-bound values (header/SMTP injection, CWE-93). - Middleware\CsrfMiddleware: reject empty tokens and compare with hash_equals (null==null bypass and timing). - Auth\SessionGuard + Session::regenerate(): rotate the session id on login while preserving data (session fixation). - Security\Crypto::decrypt(): fail closed on non-authenticated legacy (static-IV) input unless explicitly opted in via allowLegacy(). - Http: sanitize download filename; basename() the explicit upload target. Adds regression tests for the above. --- src/Auth/Guards/SessionGuard.php | 8 ++ src/Database/QueryBuilder.php | 78 +++++++++++++++---- src/Http/Response.php | 9 ++- src/Http/UploadedFile.php | 4 + src/Mail/Envelop.php | 40 +++++++++- src/Middleware/CsrfMiddleware.php | 14 +++- src/Security/Crypto.php | 26 +++++++ src/Session/Session.php | 7 +- src/Storage/Service/DiskFilesystemService.php | 34 +++++++- tests/Database/Query/QueryBuilderTest.php | 31 ++++++++ tests/Filesystem/DiskFilesystemTest.php | 21 +++++ tests/Hashing/SecurityTest.php | 31 ++++++++ tests/Mail/EnvelopHeaderInjectionTest.php | 62 +++++++++++++++ 13 files changed, 336 insertions(+), 29 deletions(-) create mode 100644 tests/Mail/EnvelopHeaderInjectionTest.php diff --git a/src/Auth/Guards/SessionGuard.php b/src/Auth/Guards/SessionGuard.php index 29b357f6..35bf29f8 100644 --- a/src/Auth/Guards/SessionGuard.php +++ b/src/Auth/Guards/SessionGuard.php @@ -63,6 +63,10 @@ public function attempts(array $credentials, bool $remember = false): bool $password = $credentials[$fields['password']]; if (Hash::check($password, $user->{$fields['password']})) { + // Rotate the session ID on privilege elevation (anti session + // fixation). regenerate() preserves data, so the stored user below + // lives under the new ID. + $this->getSession()->regenerate(); $this->getSession()->put($this->session_key, $user); if ($remember) { @@ -127,6 +131,10 @@ public function guest(): bool */ public function login(Authentication $user, bool $remember = false): bool { + // Rotate the session ID on privilege elevation (anti session fixation). + // regenerate() preserves data, so the stored user below lives under the + // new ID. + $this->getSession()->regenerate(); $this->getSession()->add($this->session_key, $user); if ($remember) { diff --git a/src/Database/QueryBuilder.php b/src/Database/QueryBuilder.php index b352478f..8d375205 100644 --- a/src/Database/QueryBuilder.php +++ b/src/Database/QueryBuilder.php @@ -352,6 +352,8 @@ public function where( ); } + $column = $this->assertSafeIdentifier($column, 'where'); + if ($value instanceof QueryBuilder) { $indicator = "(" . $value->toSql() . ")"; } else { @@ -429,11 +431,17 @@ private static function isComparisonOperator(mixed $comparator): bool * @return string * @throws QueryBuilderException */ - private static function assertSafeIdentifier(string $identifier, string $clause): string + private function assertSafeIdentifier(string $identifier, string $clause, bool $allowWildcard = false): string { $trimmed = trim($identifier); - if (!preg_match('/^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$/', $trimmed)) { + // Aggregates and SELECT lists may legitimately use "*" (and + // "table.*"); WHERE/JOIN/ORDER BY/GROUP BY identifiers may not. + $pattern = $allowWildcard + ? '/^(\*|[A-Za-z_][A-Za-z0-9_]*(\.(\*|[A-Za-z_][A-Za-z0-9_]*))?)$/' + : '/^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$/'; + + if (!preg_match($pattern, $trimmed)) { throw new QueryBuilderException( "Unsafe identifier passed to {$clause}: [{$identifier}]. " . "Only a plain or table-qualified column name is allowed." @@ -565,6 +573,8 @@ public function setTable(string $table): QueryBuilder */ public function whereNull(string $column): QueryBuilder { + $column = $this->assertSafeIdentifier($column, 'whereNull'); + if (is_null($this->where)) { $this->where = $column . ' is null'; } else { @@ -584,6 +594,8 @@ public function whereNull(string $column): QueryBuilder */ public function whereNotNull(string $column): QueryBuilder { + $column = $this->assertSafeIdentifier($column, 'whereNotNull'); + if (is_null($this->where)) { $this->where = $column . ' is not null'; } else { @@ -602,15 +614,18 @@ public function whereNotNull(string $column): QueryBuilder */ public function whereNotBetween(string $column, array $range): QueryBuilder { + $column = $this->assertSafeIdentifier($column, 'whereNotBetween'); $range = (array) $range; - $between = implode(' and ', $range); if (is_null($this->where)) { - $this->where = $column . ' not between ' . $between; + $this->where = $column . ' not between ? and ?'; } else { - $this->where .= ' and ' . $column . ' not between ' . $between; + $this->where .= ' and ' . $column . ' not between ? and ?'; } + $this->where_data_binding[] = $range[0]; + $this->where_data_binding[] = $range[1]; + return $this; } @@ -625,15 +640,18 @@ public function whereNotBetween(string $column, array $range): QueryBuilder */ public function whereBetween(string $column, array $range): QueryBuilder { + $column = $this->assertSafeIdentifier($column, 'whereBetween'); $range = (array) $range; - $between = implode(' and ', $range); if (is_null($this->where)) { - $this->where = $column . ' between ' . $between; + $this->where = $column . ' between ? and ?'; } else { - $this->where .= ' and ' . $column . ' between ' . $between; + $this->where .= ' and ' . $column . ' between ? and ?'; } + $this->where_data_binding[] = $range[0]; + $this->where_data_binding[] = $range[1]; + return $this; } @@ -675,6 +693,8 @@ public function whereNotIn(string $column, array $range) $in = (string) $range; } + $column = $this->assertSafeIdentifier($column, 'whereNotIn'); + if (is_null($this->where)) { $this->where = $column . ' not in (' . $in . ')'; } else { @@ -708,6 +728,8 @@ public function whereIn(string $column, array $range): QueryBuilder $in = (string) $range; } + $column = $this->assertSafeIdentifier($column, 'whereIn'); + if (is_null($this->where)) { $this->where = $column . ' in (' . $in . ')'; } else { @@ -732,7 +754,7 @@ public function join( mixed $comparator = '=', ?string $second = null ): QueryBuilder { - $table = $this->getPrefix() . $table; + $table = $this->getPrefix() . $this->assertSafeIdentifier($table, 'join'); if (is_null($this->join)) { $this->join = ''; @@ -746,6 +768,9 @@ public function join( $comparator = '='; } + $first = $this->assertSafeIdentifier($first, 'join'); + $second = $this->assertSafeIdentifier($second, 'join'); + // Building the join query $this->join .= 'inner join ' . $table . ' on ' . $first . ' ' . $comparator . ' ' . $second; @@ -791,7 +816,7 @@ public function leftJoin( mixed $comparator = '=', ?string $second = null ): QueryBuilder { - $table = $this->getPrefix() . $table; + $table = $this->getPrefix() . $this->assertSafeIdentifier($table, 'join'); if (is_null($this->join)) { $this->join = ''; @@ -805,6 +830,9 @@ public function leftJoin( $comparator = '='; } + $first = $this->assertSafeIdentifier($first, 'join'); + $second = $this->assertSafeIdentifier($second, 'join'); + // Building the join query $this->join .= 'left join ' . $table . ' on ' . $first . ' ' . $comparator . ' ' . $second . ' '; @@ -827,7 +855,7 @@ public function rightJoin( mixed $comparator = '=', ?string $second = null ): QueryBuilder { - $table = $this->getPrefix() . $table; + $table = $this->getPrefix() . $this->assertSafeIdentifier($table, 'join'); if (is_null($this->join)) { $this->join = ''; @@ -841,6 +869,9 @@ public function rightJoin( $comparator = '='; } + $first = $this->assertSafeIdentifier($first, 'join'); + $second = $this->assertSafeIdentifier($second, 'join'); + $this->join .= 'right join ' . $table . ' on ' . $first . ' ' . $comparator . ' ' . $second; return $this; @@ -925,7 +956,7 @@ public function group(string $column) public function groupBy(string $column): QueryBuilder { if (is_null($this->group)) { - $this->group = static::assertSafeIdentifier($column, 'groupBy'); + $this->group = $this->assertSafeIdentifier($column, 'groupBy'); } return $this; @@ -952,7 +983,7 @@ public function having( $comparator = '='; } - $column = static::assertSafeIdentifier($column, 'having'); + $column = $this->assertSafeIdentifier($column, 'having'); // Bind the value with a placeholder, exactly like where(). A subquery // is inlined; any scalar is parameterised so it can never be injected. @@ -985,7 +1016,7 @@ public function orderBy(string $column, string $type = 'asc'): QueryBuilder $type = 'asc'; } - $column = static::assertSafeIdentifier($column, 'orderBy'); + $column = $this->assertSafeIdentifier($column, 'orderBy'); if (is_null($this->order)) { $this->order = 'order by ' . $column . ' ' . strtolower($type); @@ -1016,6 +1047,7 @@ public function max(string $column): int|float */ private function aggregate($aggregate, $column): mixed { + $column = $this->assertSafeIdentifier($column, 'aggregate', true); $sql = 'select ' . $aggregate . '(' . $column . ') from ' . $this->table; // Adding the join clause @@ -1344,6 +1376,8 @@ public function select(array $select = ['*']) foreach ($select as $key => $value) { if ($value instanceof QueryBuilder) { $select[$key] = '(' . $value->toSql() . ')'; + } else { + $select[$key] = $this->assertSafeIdentifier((string) $value, 'select', true); } } @@ -1386,8 +1420,14 @@ public function jump(int $offset = 0): QueryBuilder */ public function update(array $data = []): int { + $columns = array_keys($data); + + foreach ($columns as $column) { + $this->assertSafeIdentifier((string) $column, 'update'); + } + $sql = 'update ' . $this->table . ' set '; - $sql .= implode(' = ?, ', array_keys($data)) . ' = ?'; + $sql .= implode(' = ?, ', $columns) . ' = ?'; if (!is_null($this->where)) { $sql .= ' where ' . $this->where; @@ -1481,6 +1521,7 @@ public function decrement(string $column, int $step = 1): int */ private function incrementAction(string $column, int $step = 1, string $direction = '+') { + $column = $this->assertSafeIdentifier($column, 'incrementAction'); $sql = 'update ' . $this->table . ' set ' . $column . ' = ' . $column . ' ' . $direction . ' ' . $step; if (!is_null($this->where)) { @@ -1505,6 +1546,8 @@ private function incrementAction(string $column, int $step = 1, string $directio */ public function distinct(string $column) { + $column = $this->assertSafeIdentifier($column, 'distinct', true); + if (!is_null($this->select)) { $this->select .= ", distinct $column"; } else { @@ -1619,6 +1662,11 @@ public function insert(array $values): int private function insertOne(array $values): int { $fields = array_keys($values); + + foreach ($fields as $field) { + $this->assertSafeIdentifier((string) $field, 'insertOne'); + } + $column = implode(', ', $fields); $sql = 'insert into ' . $this->table . '(' . $column . ') values'; diff --git a/src/Http/Response.php b/src/Http/Response.php index cbff3beb..344b59ff 100644 --- a/src/Http/Response.php +++ b/src/Http/Response.php @@ -108,6 +108,9 @@ public function getCode(): int /** * Download the given file as an argument * + * Note: $file is expected to be a trusted, caller-controlled path. Never + * pass unsanitized user input as $file, as it is read directly from disk. + * * @param string $file * @param ?string $filename * @param array $headers @@ -124,9 +127,13 @@ public function download( $filename = basename($file); } + // Sanitize the filename used in the Content-Disposition header to prevent + // header injection and path disclosure (strip directory, CR/LF and quotes). + $filename = str_replace(["\r", "\n", '"'], '', basename($filename)); + $disposition = $headers["disposition"] ?? 'attachment'; - $this->withHeader('Content-Disposition', $disposition . '; filename=' . $filename); + $this->withHeader('Content-Disposition', $disposition . '; filename="' . $filename . '"'); $this->withHeader('Content-Type', $type); $file_size = filesize($file); diff --git a/src/Http/UploadedFile.php b/src/Http/UploadedFile.php index bfd4ad51..9b9e9903 100644 --- a/src/Http/UploadedFile.php +++ b/src/Http/UploadedFile.php @@ -140,6 +140,10 @@ public function moveTo(string $to, ?string $filename = null): bool if (is_null($filename)) { $filename = $this->getHashName(); + } else { + // Strip any directory component from an explicit filename to prevent + // path traversal outside of the destination directory. + $filename = basename($filename); } if (!is_dir($to)) { diff --git a/src/Mail/Envelop.php b/src/Mail/Envelop.php index ff543de2..72c2422b 100644 --- a/src/Mail/Envelop.php +++ b/src/Mail/Envelop.php @@ -137,11 +137,25 @@ protected function setBoundary(string $boundary): void */ public function withHeader(string $key, string $value): Envelop { + $key = $this->sanitizeHeaderValue($key); + $value = $this->sanitizeHeaderValue($value); + $this->headers[] = "$key: $value"; return $this; } + /** + * Strip CR/LF/NUL from a header-bound value to prevent header injection + * + * @param string $value + * @return string + */ + private function sanitizeHeaderValue(string $value): string + { + return str_replace(["\r", "\n", "\0"], '', $value); + } + /** * Define the receiver * @@ -151,10 +165,10 @@ public function withHeader(string $key, string $value): Envelop */ public function to(string|array $to): Envelop { - $recipients = (array)$to; + $recipients = (array) $to; - foreach ($recipients as $to) { - $this->to[] = $this->formatEmail($to); + foreach ($recipients as $item) { + $this->to[] = $this->formatEmail($item); } return $this; @@ -236,7 +250,7 @@ public function compileHeaders(): string */ public function subject(string $subject): Envelop { - $this->subject = $subject; + $this->subject = $this->sanitizeHeaderValue($subject); return $this; } @@ -250,6 +264,9 @@ public function subject(string $subject): Envelop */ public function from(string $from, ?string $name = null): Envelop { + $from = $this->sanitizeHeaderValue($from); + $name = ($name !== null) ? $this->sanitizeHeaderValue($name) : null; + $this->from = ($name !== null) ? (ucwords($name) . " <{$from}>") : $from; return $this; @@ -305,6 +322,9 @@ public function text(string $text): Envelop */ public function addBcc(string $mail, ?string $name = null): Envelop { + $mail = $this->sanitizeHeaderValue($mail); + $name = ($name !== null) ? $this->sanitizeHeaderValue($name) : null; + $mail = ($name !== null) ? (ucwords($name) . " <{$mail}>") : $mail; $this->headers[] = "Bcc: $mail"; @@ -335,6 +355,9 @@ public function bcc(string $mail, ?string $name = null): Envelop */ public function addCc(string $mail, ?string $name = null): Envelop { + $mail = $this->sanitizeHeaderValue($mail); + $name = ($name !== null) ? $this->sanitizeHeaderValue($name) : null; + $mail = ($name !== null) ? (ucwords($name) . " <{$mail}>") : $mail; $this->headers[] = "Cc: $mail"; @@ -364,6 +387,9 @@ public function cc(string $mail, ?string $name = null): Envelop */ public function addReplyTo(string $mail, ?string $name = null): Envelop { + $mail = $this->sanitizeHeaderValue($mail); + $name = ($name !== null) ? $this->sanitizeHeaderValue($name) : null; + $mail = ($name !== null) ? (ucwords($name) . " <{$mail}>") : $mail; $this->headers[] = "Replay-To: $mail"; @@ -393,6 +419,9 @@ public function replyTo(string $mail, ?string $name = null): Envelop */ public function addReturnPath(string $mail, ?string $name = null): Envelop { + $mail = $this->sanitizeHeaderValue($mail); + $name = ($name !== null) ? $this->sanitizeHeaderValue($name) : null; + $mail = ($name !== null) ? (ucwords($name) . " <{$mail}>") : $mail; $this->headers[] = "Return-Path: $mail"; @@ -410,6 +439,9 @@ public function addReturnPath(string $mail, ?string $name = null): Envelop */ public function returnPath(string $mail, ?string $name = null): Envelop { + $mail = $this->sanitizeHeaderValue($mail); + $name = ($name !== null) ? $this->sanitizeHeaderValue($name) : null; + $mail = ($name !== null) ? (ucwords($name) . " <{$mail}>") : $mail; $this->headers[] = "Return-Path: $mail"; diff --git a/src/Middleware/CsrfMiddleware.php b/src/Middleware/CsrfMiddleware.php index 7d32f54d..16acde17 100644 --- a/src/Middleware/CsrfMiddleware.php +++ b/src/Middleware/CsrfMiddleware.php @@ -25,8 +25,14 @@ public function process(Request $request, callable $next, array $args = []): mix } } + $session = (string) $request->session()->get('_token'); + if ($request->isAjax()) { - if ($request->getHeader('x-csrf-token') === session('_token')) { + $provided = (string) $request->getHeader('x-csrf-token'); + + // Reject empties then constant-time compare, so an unset session + // token can no longer be matched by an absent/empty header. + if ($session !== '' && hash_equals($session, $provided)) { return $next($request); } @@ -37,7 +43,11 @@ public function process(Request $request, callable $next, array $args = []): mix ); } - if ($request->get('_token') == $request->session()->get('_token')) { + $provided = (string) $request->get('_token'); + + // Reject empties then constant-time compare, so an unset session token + // can no longer be matched by an absent/empty form token. + if ($session !== '' && hash_equals($session, $provided)) { return $next($request); } diff --git a/src/Security/Crypto.php b/src/Security/Crypto.php index 04ba6be5..3f9023d2 100644 --- a/src/Security/Crypto.php +++ b/src/Security/Crypto.php @@ -22,6 +22,15 @@ class Crypto */ private static string $cipher = 'AES-256-CBC'; + /** + * Whether decrypt() may fall back to the unauthenticated legacy format + * (static IV, no MAC) for values lacking the BOW2: header. Disabled by + * default so decrypt() fails closed; opt in only to read old ciphertexts. + * + * @var bool + */ + private static bool $allow_legacy = false; + /** * Header tagging the authenticated (random-IV + HMAC) payload format. * @@ -56,6 +65,17 @@ public static function setKey(string $key, ?string $cipher = null): void } } + /** + * Allow or forbid decrypt() from falling back to the unauthenticated legacy + * format. Off by default; enable only while migrating old ciphertexts. + * + * @param bool $allow + */ + public static function allowLegacy(bool $allow = true): void + { + static::$allow_legacy = $allow; + } + /** * Encrypt data. * @@ -107,6 +127,12 @@ public static function decrypt(string $data): string|bool $key = static::resolveKey(); if (!str_starts_with($data, self::HEADER)) { + // Fail closed on non-authenticated input unless the legacy format + // has been explicitly re-enabled for migration. + if (!static::$allow_legacy) { + return false; + } + return static::decryptLegacy($data, $key); } diff --git a/src/Session/Session.php b/src/Session/Session.php index 19348927..15b085cf 100644 --- a/src/Session/Session.php +++ b/src/Session/Session.php @@ -121,13 +121,12 @@ 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. + // fixated/leaked ID can no longer be reused. session_regenerate_id(true) + // deletes the old file while PRESERVING $_SESSION, so authenticated data + // survives the rotation (do NOT flush/destroy here). if (PHP_SESSION_ACTIVE === session_status()) { session_regenerate_id(true); } - - $this->flush(); - $this->start(); } /** diff --git a/src/Storage/Service/DiskFilesystemService.php b/src/Storage/Service/DiskFilesystemService.php index 361549a0..c61bb8f6 100644 --- a/src/Storage/Service/DiskFilesystemService.php +++ b/src/Storage/Service/DiskFilesystemService.php @@ -6,6 +6,7 @@ use Bow\Http\UploadedFile; use Bow\Storage\Contracts\FilesystemInterface; +use Bow\Storage\Exception\ResourceException; use RuntimeException; class DiskFilesystemService implements FilesystemInterface @@ -108,11 +109,38 @@ public function put(string $file, string $content): bool */ public function path(string $file): string { - if (preg_match('#^' . $this->base_directory . '#', $file)) { - return $file; + $base = rtrim($this->base_directory, DIRECTORY_SEPARATOR); + + // Join the file to the base directory unless it is already an absolute + // path located inside the base directory (plain prefix check, not regex). + if ($file === $base || str_starts_with($file, $base . DIRECTORY_SEPARATOR)) { + $path = $file; + } else { + $path = $base . DIRECTORY_SEPARATOR . ltrim($file, '/'); + } + + // Reject any parent-directory traversal segment before touching the disk. + if (in_array('..', explode('/', str_replace('\\', '/', $path)), true)) { + throw new ResourceException( + sprintf('The path "%s" is outside of the base directory.', $file) + ); + } + + // Resolve the target (or its parent, for files that do not exist yet) and + // make sure the result stays confined to the base directory, guarding + // against symlink escapes. + $resolved = realpath($path) ?: realpath(dirname($path)); + + if ($resolved !== false + && $resolved !== $base + && !str_starts_with($resolved . DIRECTORY_SEPARATOR, $base . DIRECTORY_SEPARATOR) + ) { + throw new ResourceException( + sprintf('The path "%s" is outside of the base directory.', $file) + ); } - return rtrim($this->base_directory, '/') . '/' . ltrim($file, '/'); + return $path; } /** diff --git a/tests/Database/Query/QueryBuilderTest.php b/tests/Database/Query/QueryBuilderTest.php index b5231406..8f6f8d6b 100644 --- a/tests/Database/Query/QueryBuilderTest.php +++ b/tests/Database/Query/QueryBuilderTest.php @@ -359,6 +359,37 @@ public function test_shared_lock_flag_resets_after_to_sql(string $name) $this->assertStringNotContainsString('lock in share mode', $sql); } + /** + * SQL-injection guard: a malicious column name must be rejected. + * + * Uses a local in-memory SQLite PDO so it needs no external connection. + */ + public function test_where_rejects_unsafe_column_identifier() + { + $builder = new QueryBuilder('users', new \PDO('sqlite::memory:')); + + $this->expectException(QueryBuilderException::class); + + $builder->where('1=1 UNION SELECT password FROM admins -- ', '=', 'x'); + } + + /** + * SQL-injection guard: whereBetween must bind both bounds as placeholders + * instead of concatenating them into the statement. + */ + public function test_where_between_binds_both_bounds() + { + $builder = new QueryBuilder('users', new \PDO('sqlite::memory:')); + $builder->whereBetween('price', ['0', '100 OR 1=1 -- ']); + + $this->assertStringContainsString('price between ? and ?', $builder->toSql()); + + $property = (new \ReflectionObject($builder))->getProperty('where_data_binding'); + $property->setAccessible(true); + + $this->assertSame(['0', '100 OR 1=1 -- '], $property->getValue($builder)); + } + /** * @return array */ diff --git a/tests/Filesystem/DiskFilesystemTest.php b/tests/Filesystem/DiskFilesystemTest.php index a973cc0c..da1291f0 100644 --- a/tests/Filesystem/DiskFilesystemTest.php +++ b/tests/Filesystem/DiskFilesystemTest.php @@ -3,6 +3,7 @@ namespace Bow\Tests\Filesystem; use Bow\Http\UploadedFile; +use Bow\Storage\Exception\ResourceException; use Bow\Storage\Service\DiskFilesystemService; use Bow\Storage\Storage; use Bow\Tests\Config\TestingConfiguration; @@ -82,6 +83,26 @@ public function test_get_path_by_passed_the_right_path() $this->assertEquals($this->storage->path($path), $path); } + public function test_path_rejects_traversal() + { + $this->expectException(ResourceException::class); + + $this->storage->path('../../x'); + } + + public function test_get_does_not_leak_outside_base_directory() + { + $secret = dirname($this->storage->getBaseDirectory()) . '/secret_regression.txt'; + file_put_contents($secret, 'TOP-SECRET'); + + try { + $this->expectException(ResourceException::class); + $this->storage->get('../secret_regression.txt'); + } finally { + @unlink($secret); + } + } + public function test_is_directory() { $this->storage->makeDirectory("is_directory"); diff --git a/tests/Hashing/SecurityTest.php b/tests/Hashing/SecurityTest.php index e544bf0d..06d75410 100644 --- a/tests/Hashing/SecurityTest.php +++ b/tests/Hashing/SecurityTest.php @@ -28,4 +28,35 @@ public function test_should_check_hash_value() $this->assertTrue(Hash::check('bow', $hashed)); } + + public function test_decrypt_fails_closed_on_non_authenticated_input() + { + $key = file_get_contents(__DIR__ . '/stubs/.key'); + Crypto::setkey($key, 'AES-256-CBC'); + Crypto::allowLegacy(false); + + // A legacy (static-IV, unauthenticated) ciphertext with no BOW2: header. + $cipher = 'AES-256-CBC'; + $iv = substr(sha1($key), 0, (int) openssl_cipher_iv_length($cipher)); + $legacy = openssl_encrypt('secret', $cipher, $key, 0, $iv); + + $this->assertFalse(Crypto::decrypt($legacy)); + $this->assertFalse(Crypto::decrypt('garbage')); + } + + public function test_decrypt_reads_legacy_only_when_opted_in() + { + $key = file_get_contents(__DIR__ . '/stubs/.key'); + Crypto::setkey($key, 'AES-256-CBC'); + + $cipher = 'AES-256-CBC'; + $iv = substr(sha1($key), 0, (int) openssl_cipher_iv_length($cipher)); + $legacy = openssl_encrypt('secret', $cipher, $key, 0, $iv); + + Crypto::allowLegacy(true); + $this->assertEquals('secret', Crypto::decrypt($legacy)); + + Crypto::allowLegacy(false); + $this->assertFalse(Crypto::decrypt($legacy)); + } } diff --git a/tests/Mail/EnvelopHeaderInjectionTest.php b/tests/Mail/EnvelopHeaderInjectionTest.php new file mode 100644 index 00000000..9211715f --- /dev/null +++ b/tests/Mail/EnvelopHeaderInjectionTest.php @@ -0,0 +1,62 @@ +subject("Hi\r\nBcc: victim@evil.com"); + + $subject = $envelop->getSubject(); + + $this->assertStringNotContainsString("\r", $subject); + $this->assertStringNotContainsString("\n", $subject); + $this->assertSame("HiBcc: victim@evil.com", $subject); + } + + public function test_with_header_strips_crlf_from_key_and_value() + { + $envelop = new Envelop(false); + $envelop->withHeader("X-Test\r\nBcc: x@evil", "v\r\nCc: y@evil"); + + $compiled = $envelop->compileHeaders(); + + $this->assertStringNotContainsString("\nBcc:", $compiled); + $this->assertStringNotContainsString("\nCc:", $compiled); + + foreach ($envelop->getHeaders() as $header) { + $this->assertStringNotContainsString("\r", $header); + $this->assertStringNotContainsString("\n", $header); + } + } + + public function test_from_strips_crlf() + { + $envelop = new Envelop(false); + $envelop->from("bob@example.com\r\nBcc: x@evil", "Bob\r\nName"); + + $from = $envelop->getFrom(); + + $this->assertStringNotContainsString("\r", $from); + $this->assertStringNotContainsString("\n", $from); + } + + public function test_copy_and_reply_headers_strip_crlf() + { + $envelop = new Envelop(false); + $envelop->addBcc("bcc@example.com\r\nSubject: hacked"); + $envelop->addCc("cc@example.com\r\nSubject: hacked"); + $envelop->addReplyTo("reply@example.com\r\nSubject: hacked"); + $envelop->addReturnPath("return@example.com\r\nSubject: hacked"); + + foreach ($envelop->getHeaders() as $header) { + $this->assertStringNotContainsString("\r", $header); + $this->assertStringNotContainsString("\n", $header); + } + } +}