Skip to content

Commit d904efc

Browse files
committed
Security fixes: FormFlash traversal, salt leak, user-overwrite hardening
- GHSA-hmcx-ch82-3fv2 (high, unauth): FormFlash::__construct now sanitizes session_id/unique_id through a strict allowlist before they reach any filesystem path; invalid values blank out and disable flash for the request, blocking the __form-flash-id arbitrary-directory-create attack. - GHSA-3f29-pqwf-v4j4 (medium): the HMAC key used for CSRF nonce signing and admin rate-limit hashing has moved out of Config into a new Security::getNonceKey() backed by user/config/security-private.php (a .php file, blocked by the default user/*.php htaccess rule and returning empty on direct exec). Existing sites are migrated transparently on first call — the value is preserved, then scrubbed from the live Config and the on-disk security.yaml so sandboxed Twig can no longer leak it via grav.config.get('security.salt'). Auto-gen of security.salt removed from Setup.php; system/config/security.yaml placeholder dropped. - GHSA-rr73-568v-28f8 (high): tightened the new-user uniqueness guard in UserObject::save. strpos(...,'@@') replaced with str_contains (the old form was falsy when the marker sat at position 0, silently bypassing the check) and the instanceof FileStorage gate dropped so the check runs for every FlexStorageInterface backend. - GHSA-58hj-46fw-rcfm: verified covered by the new Twig sandbox (UserCollection::load/save/set are not in the allowlist); regression test pinned in TwigSandboxTest. Three new test files covering the FormFlash, nonce-key, and user-overwrite fixes; .gitignore updated to keep the per-environment private key file and its tmp sibling out of version control. Full unit suite: 629 tests, 2417 assertions.
1 parent 38685ac commit d904efc

13 files changed

Lines changed: 547 additions & 33 deletions

File tree

.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ user/plugins/*
2626
user/themes/*
2727
!user/themes/.*
2828
user/**/config/security.yaml
29+
user/**/config/security-private.php
30+
user/**/config/security-private.php.tmp
2931

3032
# Environments
3133
.env

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@
99
* Smarter dangerous-Twig filter — fixes false positives like `{{ page.header.user.mail }}` getting blocked because they happened to contain a dangerous function name.
1010
* Soft-fail on sandbox violations — the rest of the page still renders, visitors see a small placeholder, admins see a pointer to the log entry.
1111
* Escape hatch — the sandbox can be disabled from the admin UI (or YAML) if a site genuinely needs the old, unrestricted behaviour.
12+
1. [](#bugfix)
13+
* [security] Fixed unauthenticated path traversal in `FormFlash` (GHSA-hmcx-ch82-3fv2): `session_id` and `unique_id` now pass through a strict allowlist before being used to build on-disk paths, preventing arbitrary directory creation via the `__form-flash-id` parameter.
14+
* [security] Fixed salt disclosure via sandboxed Twig (GHSA-3f29-pqwf-v4j4): the HMAC key used for CSRF nonces and admin rate-limit hashing has moved out of Config into `user/config/security-private.php` (blocked from web access by the default `user/*.php` htaccess rule), so `{{ grav.config.get('security.salt') }}` no longer leaks it. Existing sites are migrated automatically on first request — existing nonces and sessions survive the upgrade.
15+
* [security] Hardened the new-user uniqueness guard in `UserObject::save` (GHSA-rr73-568v-28f8): `strpos(..., '@@')` replaced with `str_contains` (the old form was falsy when the transient-key marker was at position 0, silently bypassing the check) and the check now runs for every `FlexStorageInterface` backend rather than only `FileStorage`. A low-privileged user with `admin.users.create` can no longer disrupt a super-admin account by submitting the admin's username through the "add user" form.
1216

1317
# v2.0.0-beta.1
1418
## 04/16/2026

system/config/security.yaml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -298,4 +298,7 @@ twig_sandbox:
298298
# Wildcard on stdClass for {{ page.header.<any_yaml_key> }}
299299
- class: 'stdClass'
300300
methods: '*'
301-
salt: SbmgUJQ62MqNc0
301+
# NOTE: `salt:` was removed in v2.0.0-beta.2 (GHSA-3f29-pqwf-v4j4). The equivalent
302+
# HMAC key is now stored in `user/config/security-private.php`, outside the Config
303+
# tree, so sandboxed Twig cannot read it via `grav.config.get(...)`. On upgrade,
304+
# any legacy `salt:` in `user/config/security.yaml` is migrated automatically.

system/src/Grav/Common/Config/Setup.php

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
use BadMethodCallException;
1313
use Grav\Common\File\CompiledYamlFile;
1414
use Grav\Common\Data\Data;
15-
use Grav\Common\Utils;
1615
use InvalidArgumentException;
1716
use Pimple\Container;
1817
use Psr\Http\Message\ServerRequestInterface;
@@ -407,21 +406,9 @@ protected function check(UniformResourceLocator $locator)
407406
$this->initializeLocator($locator);
408407
}
409408

410-
// Create security.yaml salt if it doesn't exist into existing configuration environment if possible.
411-
$securityFile = Utils::basename(static::$securityFile);
412-
$securityFolder = substr(static::$securityFile, 0, -\strlen($securityFile));
413-
$securityFolder = $locator->findResource($securityFolder, true) ?: $locator->findResource($securityFolder, true, true);
414-
$filename = "{$securityFolder}/{$securityFile}";
415-
416-
$security_file = CompiledYamlFile::instance($filename);
417-
$security_content = (array)$security_file->content();
418-
419-
if (!isset($security_content['salt'])) {
420-
$security_content = array_merge($security_content, ['salt' => Utils::generateRandomString(14)]);
421-
$security_file->content($security_content);
422-
$security_file->save();
423-
$security_file->free();
424-
}
409+
// Legacy `security.salt` auto-gen was removed in v2.0 (GHSA-3f29-pqwf-v4j4);
410+
// Security::getNonceKey() now manages the equivalent value in a private
411+
// PHP file outside the Config tree so sandboxed Twig cannot read it.
425412
} catch (RuntimeException $e) {
426413
throw new RuntimeException(sprintf('Grav failed to initialize: %s', $e->getMessage()), 500, $e);
427414
}

system/src/Grav/Common/Flex/Types/Users/UserObject.php

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
use Grav\Framework\Filesystem\Filesystem;
3838
use Grav\Framework\Flex\Flex;
3939
use Grav\Framework\Flex\FlexDirectory;
40-
use Grav\Framework\Flex\Storage\FileStorage;
4140
use Grav\Framework\Flex\Traits\FlexMediaTrait;
4241
use Grav\Framework\Flex\Traits\FlexRelationshipsTrait;
4342
use Grav\Framework\Form\FormFlashFile;
@@ -599,20 +598,24 @@ public function save()
599598
{
600599
// TODO: We may want to handle this in the storage layer in the future.
601600
$key = $this->getStorageKey();
602-
$isNewUser = !$key || strpos($key, '@@');
601+
// `@@` is Flex's marker for an in-memory (not-yet-persisted) storage key.
602+
// `str_contains` is the correct predicate here — `strpos` returns 0 when
603+
// the marker is at position 0 (e.g. `@@hash`), which is falsy and would
604+
// have skipped the uniqueness check below.
605+
$isNewUser = $key === '' || str_contains($key, '@@');
603606

604607
if ($isNewUser) {
605-
$storage = $this->getFlexDirectory()->getStorage();
606-
if ($storage instanceof FileStorage) {
607-
$newKey = $this->getKey();
608-
609-
// Check if a user with this username already exists (prevent overwriting)
610-
if ($storage->hasKey($newKey)) {
611-
throw new RuntimeException('User account with this username already exists');
612-
}
608+
$newKey = $this->getKey();
613609

614-
$this->setStorageKey($newKey);
610+
// Prevent overwriting an existing account when a low-privileged user
611+
// creates a new user with an already-taken username (GHSA-rr73-568v-28f8).
612+
// Applies to every storage implementation, not just FileStorage.
613+
$storage = $this->getFlexDirectory()->getStorage();
614+
if ($storage->hasKey($newKey)) {
615+
throw new RuntimeException('User account with this username already exists');
615616
}
617+
618+
$this->setStorageKey($newKey);
616619
}
617620

618621
$password = $this->getProperty('password') ?? $this->getProperty('password1');

system/src/Grav/Common/Security.php

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@
1515
use Grav\Common\Page\Pages;
1616
use Grav\Common\Twig\Sandbox\GravSecurityPolicy;
1717
use Rhukster\DomSanitizer\DOMSanitizer;
18+
use RocketTheme\Toolbox\File\YamlFile;
19+
use RocketTheme\Toolbox\ResourceLocator\UniformResourceLocator;
20+
use RuntimeException;
1821
use Twig\Sandbox\SecurityPolicyInterface;
1922
use function chr;
2023
use function count;
@@ -808,4 +811,95 @@ private static function splitMethodNames($methods, bool $lowercase = true): arra
808811
}
809812
return $clean;
810813
}
814+
815+
/** @var string|null in-process cache for the nonce key */
816+
private static ?string $nonceKey = null;
817+
818+
/**
819+
* Per-site HMAC key used for CSRF nonce signing, admin rate-limit key hashing,
820+
* and (when configured) session-name derivation. Backed by a local PHP file
821+
* outside the Config tree, so sandboxed Twig cannot reach it via
822+
* `grav.config.get('security.salt')` or `Config::toArray()` (GHSA-3f29-pqwf-v4j4).
823+
*
824+
* Migration: if the legacy `security.salt` key is present in the loaded Config
825+
* (i.e. from an older install's `user/config/security.yaml`), its value is
826+
* copied into the private file on first call and scrubbed from both the live
827+
* Config and the on-disk YAML. Existing CSRF nonces and sessions survive the
828+
* upgrade because the key value is preserved.
829+
*
830+
* To rotate the key manually, delete `user/config/security-private.php`; the
831+
* next request generates a fresh 64-char random value. Rotation invalidates
832+
* in-flight CSRF nonces and — if `system.session.uniqueness` is set to
833+
* `security` — existing sessions.
834+
*/
835+
public static function getNonceKey(): string
836+
{
837+
if (self::$nonceKey !== null) {
838+
return self::$nonceKey;
839+
}
840+
841+
$grav = Grav::instance();
842+
/** @var UniformResourceLocator $locator */
843+
$locator = $grav['locator'];
844+
$configFolder = $locator->findResource('config://', true) ?: $locator->findResource('config://', true, true);
845+
$privateFile = "{$configFolder}/security-private.php";
846+
847+
if (is_file($privateFile)) {
848+
$value = @include $privateFile;
849+
if (is_string($value) && $value !== '') {
850+
return self::$nonceKey = $value;
851+
}
852+
// Corrupt/empty file — fall through to regenerate.
853+
}
854+
855+
// One-time migration out of Config for sites upgrading from <= v2.0.0-beta.2.
856+
/** @var Config $config */
857+
$config = $grav['config'];
858+
$legacy = $config->get('security.salt');
859+
if (is_string($legacy) && $legacy !== '') {
860+
self::writeNonceKey($privateFile, $legacy);
861+
$config->set('security.salt', null);
862+
863+
$securityYaml = "{$configFolder}/security.yaml";
864+
if (is_file($securityYaml)) {
865+
$file = YamlFile::instance($securityYaml);
866+
$content = (array) $file->content();
867+
if (array_key_exists('salt', $content)) {
868+
unset($content['salt']);
869+
$file->content($content);
870+
$file->save();
871+
$file->free();
872+
}
873+
}
874+
875+
return self::$nonceKey = $legacy;
876+
}
877+
878+
$generated = bin2hex(random_bytes(32));
879+
self::writeNonceKey($privateFile, $generated);
880+
881+
return self::$nonceKey = $generated;
882+
}
883+
884+
private static function writeNonceKey(string $path, string $value): void
885+
{
886+
$escaped = var_export($value, true);
887+
$contents = "<?php\n\n// Auto-generated private secret. Do NOT commit to version control.\n// Used for CSRF nonce signing and admin rate-limit hashing. Regenerate by\n// deleting this file; the next request will write a new value.\n\nreturn {$escaped};\n";
888+
889+
$dir = dirname($path);
890+
if (!is_dir($dir)) {
891+
Folder::create($dir);
892+
}
893+
894+
// Atomic write: stage to a temp file, fsync via rename.
895+
$tmp = $path . '.tmp';
896+
if (@file_put_contents($tmp, $contents, LOCK_EX) === false) {
897+
throw new RuntimeException('Failed to write nonce key file');
898+
}
899+
@chmod($tmp, 0600);
900+
if (!@rename($tmp, $path)) {
901+
@unlink($tmp);
902+
throw new RuntimeException('Failed to commit nonce key file');
903+
}
904+
}
811905
}

system/src/Grav/Common/Service/SessionServiceProvider.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
use Grav\Common\Config\Config;
1313
use Grav\Common\Debugger;
14+
use Grav\Common\Security;
1415
use Grav\Common\Session;
1516
use Grav\Common\Uri;
1617
use Grav\Common\Utils;
@@ -86,7 +87,7 @@ public function register(Container $container)
8687
}
8788

8889
$session_prefix = $c['inflector']->hyphenize($config->get('system.session.name', 'grav-site'));
89-
$session_uniqueness = $config->get('system.session.uniqueness', 'path') === 'path' ? substr(md5(GRAV_ROOT), 0, 7) : md5((string) $config->get('security.salt'));
90+
$session_uniqueness = $config->get('system.session.uniqueness', 'path') === 'path' ? substr(md5(GRAV_ROOT), 0, 7) : md5(Security::getNonceKey());
9091

9192
$session_name = $session_prefix . '-' . $session_uniqueness;
9293

system/src/Grav/Common/Utils.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1405,7 +1405,7 @@ private static function generateNonceString($action, $previousTick = false)
14051405
$i--;
14061406
}
14071407

1408-
return ($i . '|' . $action . '|' . $username . '|' . $token . '|' . $grav['config']->get('security.salt'));
1408+
return ($i . '|' . $action . '|' . $username . '|' . $token . '|' . Security::getNonceKey());
14091409
}
14101410

14111411
/**

system/src/Grav/Framework/Form/FormFlash.php

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,14 @@ public function __construct($config)
7575
$config = array_filter($config, static fn($val) => $val !== null);
7676
}
7777

78-
$this->id = $config['id'] ?? '';
79-
$this->sessionId = $config['session_id'] ?? '';
80-
$this->uniqueId = $config['unique_id'] ?? '';
78+
// Identifiers are used to build filesystem paths (tmp://forms/<sessionId>/<uniqueId>)
79+
// and can reach this constructor from request input (e.g. __form-flash-id).
80+
// Reject anything outside a strict alphanumeric+[,_-] allowlist to prevent path
81+
// traversal into arbitrary directories. Invalid values blank out, which disables
82+
// flash storage for the request rather than failing hard.
83+
$this->id = self::sanitizeId($config['id'] ?? '');
84+
$this->sessionId = self::sanitizeId($config['session_id'] ?? '');
85+
$this->uniqueId = self::sanitizeId($config['unique_id'] ?? '');
8186

8287
$this->setUser($config['user'] ?? null);
8388

@@ -489,6 +494,22 @@ public function getTmpDir(): string
489494
return $this->folder && $this->uniqueId ? "{$this->folder}/{$this->uniqueId}" : '';
490495
}
491496

497+
/**
498+
* Gate for identifiers used in filesystem paths. Accepts the character
499+
* set produced by PHP session IDs and Grav's form unique-id generators
500+
* (alphanumerics, comma, underscore, hyphen). Anything else — including
501+
* empty/non-string values — collapses to an empty string, which causes
502+
* save()/delete()/getTmpDir() to become no-ops.
503+
*/
504+
private static function sanitizeId($id): string
505+
{
506+
if (!is_string($id) || $id === '') {
507+
return '';
508+
}
509+
510+
return preg_match('/^[A-Za-z0-9,_-]{1,64}$/', $id) ? $id : '';
511+
}
512+
492513
/**
493514
* @return ?YamlFile
494515
*/
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
<?php
2+
3+
use Codeception\Util\Fixtures;
4+
use Grav\Common\Grav;
5+
use Grav\Framework\Form\FormFlash;
6+
7+
/**
8+
* Class FormFlashSecurityTest
9+
*
10+
* Covers: GHSA-hmcx-ch82-3fv2 (unauthenticated path traversal / arbitrary
11+
* directory creation via FormFlash session_id / unique_id).
12+
*
13+
* Naming convention: test{Method}_{GHSA_ID}_{description}
14+
*/
15+
class FormFlashSecurityTest extends \PHPUnit\Framework\TestCase
16+
{
17+
/** @var Grav */
18+
protected $grav;
19+
20+
protected function setUp(): void
21+
{
22+
parent::setUp();
23+
$grav = Fixtures::get('grav');
24+
$this->grav = $grav();
25+
}
26+
27+
// =========================================================================
28+
// GHSA-hmcx-ch82-3fv2: Unauthenticated path traversal in FormFlash
29+
// =========================================================================
30+
31+
/**
32+
* @dataProvider providerGHSAhmcx_TraversalIds
33+
*/
34+
public function testConstruct_GHSAhmcx_RejectsTraversalSessionId(string $id, string $description): void
35+
{
36+
$flash = new FormFlash([
37+
'session_id' => $id,
38+
'unique_id' => 'abcdef1234567890',
39+
'form_name' => 'test',
40+
]);
41+
42+
$this->assertSame('', $flash->getSessionId(), $description);
43+
$this->assertSame('', $flash->getTmpDir(), "tmp dir must be empty when session id is rejected: {$description}");
44+
}
45+
46+
/**
47+
* @dataProvider providerGHSAhmcx_TraversalIds
48+
*/
49+
public function testConstruct_GHSAhmcx_RejectsTraversalUniqueId(string $id, string $description): void
50+
{
51+
$flash = new FormFlash([
52+
'session_id' => 'abcdef1234567890',
53+
'unique_id' => $id,
54+
'form_name' => 'test',
55+
]);
56+
57+
$this->assertSame('', $flash->getUniqueId(), $description);
58+
$this->assertSame('', $flash->getTmpDir(), "tmp dir must be empty when unique id is rejected: {$description}");
59+
}
60+
61+
/**
62+
* @dataProvider providerGHSAhmcx_ValidIds
63+
*/
64+
public function testConstruct_GHSAhmcx_AcceptsValidIds(string $id, string $description): void
65+
{
66+
$flash = new FormFlash([
67+
'session_id' => $id,
68+
'unique_id' => $id,
69+
'form_name' => 'test',
70+
]);
71+
72+
$this->assertSame($id, $flash->getSessionId(), $description);
73+
$this->assertSame($id, $flash->getUniqueId(), $description);
74+
}
75+
76+
public static function providerGHSAhmcx_TraversalIds(): array
77+
{
78+
return [
79+
['../../user/config/proof_dir', 'parent-dir traversal (advisory PoC)'],
80+
['..', 'bare parent-dir'],
81+
['foo/../bar', 'embedded traversal'],
82+
['foo/bar', 'embedded forward slash'],
83+
['foo\\bar', 'embedded backslash'],
84+
['foo:bar', 'colon (stream prefix)'],
85+
['foo.bar', 'embedded dot'],
86+
['tmp://forms/abc', 'explicit stream url'],
87+
['%2e%2e%2f', 'url-encoded traversal'],
88+
[str_repeat('a', 65), 'overlong (>64 chars)'],
89+
["abc\x00def", 'null byte'],
90+
["abc\ndef", 'newline'],
91+
[' abc', 'leading space'],
92+
];
93+
}
94+
95+
public static function providerGHSAhmcx_ValidIds(): array
96+
{
97+
return [
98+
['abc123', 'alphanumeric'],
99+
['ABCdef123', 'mixed case'],
100+
['session-id-with-dashes', 'hyphens'],
101+
['session_id_with_underscores', 'underscores'],
102+
['session,id,with,commas', 'commas (PHP 5-bit session id)'],
103+
[str_repeat('a', 64), 'max length (64 chars)'],
104+
];
105+
}
106+
}

0 commit comments

Comments
 (0)