Security fixes: cache + serialization integrity, git-clone shell escape

rhukster · rhukster · commit c66dfeb5ff67 · 2026-04-23T23:32:35.000+01:00
Closes the rest of the Tier-1 unauth/authz advisories from the 2026-04 batch: - GHSA-gwfr-jfjf-92vv: Framework\Cache\Adapter\FileCache now HMAC-signs every payload (sha256, key from Security::getNonceKey()) and verifies on read. Tampered, forged, or pre-upgrade files are treated as cache misses and unlinked instead of being unserialized. New on-disk format v2\n<expires>\n<key>\n<hmac>\n<serialized>; existing caches rebuild transparently. (Adapter isn't currently in Grav's main cache path — Symfony's FilesystemAdapter is — but the class is reachable to plugin authors so the hardening is defensive.) - GHSA-vj3m-2g9h-vm4p (5-part advisory): * #1 Scheduler\JobQueue: serialized_job blob now carries a sibling serialized_job_hmac field; reconstructJob refuses to unserialize an item whose HMAC missing/mismatches and falls through to the safe structured-fields rebuild. Closes the Job::exec → call_user_func_array direct RCE gadget chain. * #2 FileCache: same fix as GHSA-gwfr above. * #3 Session::getFlashObject: payload is now wrapped in "v2|<hmac>|<serialized>"; legacy/forged envelopes return null instead of triggering unserialize. * #4 InstallCommand git clone: branch/url/path coming from user/.dependencies are now escapeshellarg'd, with a "--" separator before url/path to block option-injection (e.g. --upload-pack=evil in path). * #5 cleanDangerousTwig: twig_array_reduce (advisory call-out) plus twig_array_some/twig_array_every added to CALLABLE_DANGEROUS_NAMES. Two new test files (FileCacheSecurityTest, UnserializeIntegritySecurityTest) covering 13 cases between them; CleanDangerousTwigTest extended with the new twig_array_* entries. Full unit suite: 645 tests, 2447 assertions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,8 @@
     * [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.
     * [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.
     * [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.
+    * [security] Added HMAC integrity to `Framework\Cache\Adapter\FileCache` (GHSA-gwfr-jfjf-92vv): every cache payload is signed with `Security::getNonceKey()` on write and verified on read; tampered, attacker-planted, or pre-upgrade files are treated as cache misses and removed instead of being unserialized. The on-disk format is now versioned (`v2\n<expires>\n<key>\n<hmac>\n<serialized>`); existing caches rebuild transparently on first read.
+    * [security] Closed GHSA-vj3m-2g9h-vm4p (5-part advisory): (#1) `JobQueue` now HMAC-signs the `serialized_job` blob — a tampered queue file can no longer smuggle a forged `Job` for direct RCE via `Job::exec → call_user_func_array`; legitimate items still execute via the structured-fields fallback. (#3) `Session::getFlashObject` now wraps its payload in a versioned HMAC envelope, refusing to unserialize legacy/forged values. (#4) `InstallCommand` git-clone arguments (`branch`, `url`, `path` from `user/.dependencies`) now go through `escapeshellarg`, with a `--` separator before url/path to block option-injection. (#5) `twig_array_reduce` (plus `twig_array_some`/`twig_array_every`) added to `cleanDangerousTwig`'s callable blocklist alongside the existing `twig_array_map`/`filter`. (#2) was the same FileCache issue addressed by GHSA-gwfr above.
 
 # v2.0.0-beta.1
 ## 04/16/2026
diff --git a/system/src/Grav/Common/Scheduler/JobQueue.php b/system/src/Grav/Common/Scheduler/JobQueue.php
@@ -9,6 +9,7 @@
 
 namespace Grav\Common\Scheduler;
 
+use Grav\Common\Security;
 use RocketTheme\Toolbox\File\JsonFile;
 use RuntimeException;
 
@@ -91,8 +92,13 @@ public function push(Job $job, string $priority = self::PRIORITY_NORMAL): string
             'metadata' => [],
         ];
         
-        // Always serialize the job to preserve its full state
-        $queueItem['serialized_job'] = base64_encode(serialize($job));
+        // Always serialize the job to preserve its full state. The blob is HMAC-signed
+        // with the per-site nonce key so that a tampered queue file cannot be used to
+        // smuggle a forged Job (which would gain RCE via Job::exec → call_user_func_array).
+        // GHSA-vj3m-2g9h-vm4p (#1).
+        $serialized = serialize($job);
+        $queueItem['serialized_job'] = base64_encode($serialized);
+        $queueItem['serialized_job_hmac'] = hash_hmac('sha256', $serialized, Security::getNonceKey());
         
         $this->writeQueueItem($queueItem, 'pending');
         
@@ -459,26 +465,37 @@ protected function getItemsInDirectory(string $directory): array
      */
     protected function reconstructJob(array $item): ?Job
     {
-        if (isset($item['serialized_job'])) {
-            // Unserialize the job
-            try {
-                $job = unserialize(base64_decode((string) $item['serialized_job']));
-                if ($job instanceof Job) {
-                    return $job;
+        // GHSA-vj3m-2g9h-vm4p (#1): refuse to unserialize a queue item whose
+        // HMAC is missing or doesn't match — a tampered or attacker-planted
+        // queue file could otherwise inject a forged Job for direct RCE.
+        if (isset($item['serialized_job'], $item['serialized_job_hmac'])) {
+            $serialized = base64_decode((string) $item['serialized_job'], true);
+            if ($serialized !== false) {
+                $expected = hash_hmac('sha256', $serialized, Security::getNonceKey());
+                if (hash_equals((string) $item['serialized_job_hmac'], $expected)) {
+                    try {
+                        $job = unserialize($serialized, ['allowed_classes' => true]);
+                        if ($job instanceof Job) {
+                            return $job;
+                        }
+                    } catch (\Exception) {
+                        return null;
+                    }
                 }
-            } catch (\Exception) {
-                // Failed to unserialize
-                return null;
             }
+            // HMAC missing/mismatched/decode failed — fall through to the
+            // structured-fields rebuild below so legitimate queue items
+            // written before this fix still run, but without trusting their
+            // serialized state.
         }
-        
+
         // Create a new job from command
         if (isset($item['command'])) {
             $args = $item['arguments'] ?? [];
             $job = new Job($item['command'], $args, $item['job_id']);
             return $job;
         }
-        
+
         return null;
     }
     
diff --git a/system/src/Grav/Common/Security.php b/system/src/Grav/Common/Security.php
@@ -283,8 +283,12 @@ public static function getXssDefaults(): array
      * Property access (e.g. {{ page.header }}) is allowed; calls (header(...), obj.header(...), |header) are blocked.
      */
     private const CALLABLE_DANGEROUS_NAMES = [
-        // Twig internals
-        'twig_array_map', 'twig_array_filter', 'call_user_func', 'call_user_func_array',
+        // Twig internals — every callback-taking helper. GHSA-vj3m-2g9h-vm4p (#5)
+        // called out the missing `twig_array_reduce`; adding the other Twig 3
+        // callback predicates (some/every) at the same time as defense-in-depth.
+        'twig_array_map', 'twig_array_filter', 'twig_array_reduce',
+        'twig_array_some', 'twig_array_every',
+        'call_user_func', 'call_user_func_array',
         'forward_static_call', 'forward_static_call_array',
         // Twig environment manipulation
         'registerUndefinedFunctionCallback', 'registerUndefinedFilterCallback',
diff --git a/system/src/Grav/Common/Session.php b/system/src/Grav/Common/Session.php
@@ -97,7 +97,11 @@ public function started()
      */
     public function setFlashObject($name, mixed $object)
     {
-        $this->__set($name, serialize($object));
+        // GHSA-vj3m-2g9h-vm4p (#3): wrap the serialized payload with an HMAC so a
+        // tampered session file can't smuggle in arbitrary class instantiation.
+        $serialized = serialize($object);
+        $hmac = hash_hmac('sha256', $serialized, Security::getNonceKey());
+        $this->__set($name, "v2|{$hmac}|" . $serialized);
 
         return $this;
     }
@@ -110,9 +114,28 @@ public function setFlashObject($name, mixed $object)
      */
     public function getFlashObject($name)
     {
-        $serialized = $this->__get($name);
-
-        $object = is_string($serialized) ? unserialize($serialized, ['allowed_classes' => true]) : $serialized;
+        $stored = $this->__get($name);
+
+        $object = null;
+        if (is_string($stored) && str_starts_with($stored, 'v2|')) {
+            // 3-field format: v2|<hmac>|<serialized>. The serialized payload may
+            // itself contain `|`, so split with limit=3.
+            $parts = explode('|', $stored, 3);
+            if (count($parts) === 3) {
+                [, $expectedHmac, $serialized] = $parts;
+                $actualHmac = hash_hmac('sha256', $serialized, Security::getNonceKey());
+                if (hash_equals($expectedHmac, $actualHmac)) {
+                    try {
+                        $object = unserialize($serialized, ['allowed_classes' => true]);
+                    } catch (\Throwable) {
+                        $object = null;
+                    }
+                }
+            }
+        } elseif (!is_string($stored)) {
+            $object = $stored;
+        }
+        // Legacy unsigned strings or HMAC mismatches fall through with $object = null.
 
         $this->__unset($name);
 
diff --git a/system/src/Grav/Console/Cli/InstallCommand.php b/system/src/Grav/Console/Cli/InstallCommand.php
@@ -147,7 +147,19 @@ private function gitclone(): int
         foreach ($this->config['git'] as $repo => $data) {
             $path = $this->destination . DS . $data['path'];
             if (!file_exists($path)) {
-                exec('cd ' . escapeshellarg($this->destination) . ' && git clone -b ' . $data['branch'] . ' --depth 1 ' . $data['url'] . ' ' . $data['path'], $output, $return);
+                // GHSA-vj3m-2g9h-vm4p (#4): branch/url/path come from user/.dependencies
+                // and must be shell-escaped before reaching exec() — otherwise a planted
+                // .dependencies file gains command injection when an admin runs install.
+                // The bare `--` blocks option-injection in url/path positions
+                // (e.g. a `path` value like `--upload-pack=evil`).
+                $cmd = sprintf(
+                    'cd %s && git clone -b %s --depth 1 -- %s %s',
+                    escapeshellarg($this->destination),
+                    escapeshellarg((string) $data['branch']),
+                    escapeshellarg((string) $data['url']),
+                    escapeshellarg((string) $data['path'])
+                );
+                exec($cmd, $output, $return);
 
                 if (!$return) {
                     $io->writeln('<green>SUCCESS</green> cloned <magenta>' . $data['url'] . '</magenta> -> <cyan>' . $path . '</cyan>');
diff --git a/system/src/Grav/Framework/Cache/Adapter/FileCache.php b/system/src/Grav/Framework/Cache/Adapter/FileCache.php
@@ -11,6 +11,7 @@
 
 use ErrorException;
 use FilesystemIterator;
+use Grav\Common\Security;
 use Grav\Framework\Cache\AbstractCache;
 use Grav\Framework\Cache\Exception\CacheException;
 use Grav\Framework\Cache\Exception\InvalidArgumentException;
@@ -24,10 +25,20 @@
  *
  * Defaults to 1 year TTL. Does not support unlimited TTL.
  *
+ * Cache files are HMAC-signed (sha256, key from Security::getNonceKey()) so that a
+ * tampered or attacker-planted file is rejected as a cache miss instead of
+ * being unserialized — closes GHSA-gwfr-jfjf-92vv. The on-disk format is:
+ *
+ *     v2\n<expires>\n<key>\n<hmac-hex>\n<serialized>
+ *
+ * Pre-v2 files (no version line) are treated as cache misses and rebuilt.
+ *
  * @package Grav\Framework\Cache
  */
 class FileCache extends AbstractCache
 {
+    private const FORMAT_VERSION = 'v2';
+
     /** @var string */
     private $directory;
     /** @var string|null */
@@ -63,20 +74,38 @@ public function doGet($key, $miss)
             return $miss;
         }
 
-        if ($now >= (int) $expiresAt = fgets($h)) {
+        $version = rtrim((string)fgets($h));
+        if ($version !== self::FORMAT_VERSION) {
+            // Pre-v2 file (or junk). Drop it; the caller will repopulate.
             fclose($h);
             @unlink($file);
-        } else {
-            $i = rawurldecode(rtrim((string)fgets($h)));
-            $value = stream_get_contents($h) ?: '';
+            return $miss;
+        }
+
+        if ($now >= (int) $expiresAt = fgets($h)) {
             fclose($h);
+            @unlink($file);
+            return $miss;
+        }
 
-            if ($i === $key) {
-                return unserialize($value, ['allowed_classes' => true]);
-            }
+        $i = rawurldecode(rtrim((string)fgets($h)));
+        $expectedHmac = rtrim((string)fgets($h));
+        $value = stream_get_contents($h) ?: '';
+        fclose($h);
+
+        if ($i !== $key) {
+            return $miss;
         }
 
-        return $miss;
+        $actualHmac = hash_hmac('sha256', $value, Security::getNonceKey());
+        if (!hash_equals($expectedHmac, $actualHmac)) {
+            // Tampered or stale-secret payload — refuse to unserialize and
+            // delete the file so it gets rebuilt cleanly next time.
+            @unlink($file);
+            return $miss;
+        }
+
+        return unserialize($value, ['allowed_classes' => true]);
     }
 
     /**
@@ -86,12 +115,16 @@ public function doGet($key, $miss)
     public function doSet($key, $value, $ttl)
     {
         $expiresAt = time() + (int)$ttl;
+        $serialized = serialize($value);
+        $hmac = hash_hmac('sha256', $serialized, Security::getNonceKey());
+
+        $payload = self::FORMAT_VERSION . "\n"
+            . $expiresAt . "\n"
+            . rawurlencode($key) . "\n"
+            . $hmac . "\n"
+            . $serialized;
 
-        $result = $this->write(
-            $this->getFile($key, true),
-            $expiresAt . "\n" . rawurlencode($key) . "\n" . serialize($value),
-            $expiresAt
-        );
+        $result = $this->write($this->getFile($key, true), $payload, $expiresAt);
 
         if (!$result && !is_writable($this->directory)) {
             throw new CacheException(sprintf('Cache directory is not writable (%s)', $this->directory));
diff --git a/tests/unit/Grav/Common/Security/CleanDangerousTwigTest.php b/tests/unit/Grav/Common/Security/CleanDangerousTwigTest.php
@@ -366,6 +366,11 @@ public static function providerCallbackFunctions(): array
             ['{{ array_map("system", ["id"]) }}', 'array_map with callback'],
             ['{{ array_filter(arr, "system") }}', 'array_filter with callback'],
             ['{{ usort(arr, "system") }}', 'usort with callback'],
+            // GHSA-vj3m-2g9h-vm4p (#5): twig_array_reduce was missing from the
+            // blocklist alongside its already-listed twig_array_map/filter siblings.
+            ['{{ twig_array_reduce(arr, "system", "") }}', 'twig_array_reduce GHSA-vj3m'],
+            ['{{ twig_array_some(arr, "system") }}', 'twig_array_some'],
+            ['{{ twig_array_every(arr, "system") }}', 'twig_array_every'],
         ];
     }
 
diff --git a/tests/unit/Grav/Common/Security/FileCacheSecurityTest.php b/tests/unit/Grav/Common/Security/FileCacheSecurityTest.php
diff --git a/tests/unit/Grav/Common/Security/UnserializeIntegritySecurityTest.php b/tests/unit/Grav/Common/Security/UnserializeIntegritySecurityTest.php
