Fix security issues

.github/workflows/ci.yml

@@ -91,3 +91,33 @@ jobs:
       - name: Run Pest
         working-directory: core
         run: composer test -- --compact
+
+  audit:
+    name: Dependency audit
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v7
+
+      - name: Setup PHP
+        uses: shivammathur/setup-php@v2
+        with:
+          # The audit reads composer.lock and the advisory database; it does not run the code,
+          # so one version is enough. Kept at the lowest supported one for consistency.
+          php-version: '8.3'
+          extensions: ${{ env.PHP_EXTENSIONS }}
+          coverage: none
+          tools: composer:v2
+
+      # --locked audits the lock file directly: no vendor tree has to be installed, and what is
+      # audited is exactly what a site gets. core/ is where the shipped dependencies live.
+      - name: Audit shipped dependencies
+        working-directory: core
+        run: composer audit --locked --no-dev
+
+      # Dev-only tooling (Pest and friends) never reaches a live site, so an advisory there is
+      # worth seeing but must not block a release. The root composer.json holds PHPStan alone and
+      # ships no lock file, so there is nothing to audit there.
+      - name: Audit development dependencies
+        continue-on-error: true
+        working-directory: core
+        run: composer audit --locked

.gitignore

@@ -32,6 +32,9 @@
 /manager/includes/config.inc.php
 /manager/media/style/default/css/styles.min.css
 
+# Installer lock
+/install.session.php
+
 # Composer
 /composer.phar
 

core/functions/helper.php

@@ -29,34 +29,50 @@ function revision(string $path = ''): string
 
 if (!function_exists('createGUID')) {
     /**
-     * create globally unique identifiers (guid)
-     *
-     * @return string
+     * @throws \Random\RandomException
      */
-    function createGUID()
+    function createGUID(int $version = 4, $dashes = false): string
     {
-        mt_srand((float)microtime() * 1000000);
-        $r = mt_rand();
-        $u = uniqid(getmypid() . $r . (float)microtime() * 1000000, 1);
-        return md5($u);
+        $data = random_bytes(16);
+        if ($version === 7) {
+            $milliseconds = (int)floor(microtime(true) * 1000);
+            for ($i = 5; $i >= 0; $i--) {
+                $data[$i] = chr($milliseconds & 0xff);
+                $milliseconds = intdiv($milliseconds, 256);
+            }
+            $data[6] = chr((ord($data[6]) & 0x0f) | 0x70);
+        } else {
+            $data[6] = chr((ord($data[6]) & 0x0f) | 0x40);
+        }
+        $data[8] = chr((ord($data[8]) & 0x3f) | 0x80);
+
+        $hex = bin2hex($data);
+        return !$dashes ? $hex : substr($hex, 0, 8) . '-' . substr($hex, 8, 4) . '-' .
+            substr($hex, 12, 4) . '-' . substr($hex, 16, 4) . '-' . substr($hex, 20, 12);
     }
 }
 
 if (!function_exists('generate_password')) {
     /**
      * Generate password
      *
+     * Used for the password proposed when an account is created, which is then mailed to its
+     * owner - so it has to be unguessable. mt_rand() seeded from microtime() was not: the seed
+     * is the creation time, which anyone receiving such a mail can narrow down to a second.
+     *
      * @param int $length
      * @return string
+     * @throws \Random\RandomException
      */
-    function generate_password($length = 10)
+    function generate_password(int $length = 10): string
     {
+        // No look-alike characters (l/1/I, O/0), because these passwords get read off a screen and typed by hand.
         $allowable_characters = 'abcdefghjkmnpqrstuvxyzABCDEFGHJKLMNPQRSTUVWXYZ23456789';
         $ps_len = strlen($allowable_characters);
-        mt_srand((float)microtime() * 1000000);
-        $pass = "";
+        $length = max(1, $length);
+        $pass = '';
         for ($i = 0; $i < $length; $i++) {
-            $pass .= $allowable_characters[mt_rand(0, $ps_len - 1)];
+            $pass .= $allowable_characters[random_int(0, $ps_len - 1)];
         }
 
         return $pass;

core/src/Core.php

@@ -494,14 +494,10 @@ public function sendRedirect(string $url, int $count_attempts = 0, string $type
         }
 
         // Only allow redirects to the same domain or relative paths to prevent open redirect vulnerability
-        $parsed_url = parse_url($url);
-        if (isset($parsed_url['scheme'])) {
-            if (!in_array(strtolower($parsed_url['scheme']), ['http', 'https']) || $parsed_url['host'] !==
-                parse_url(EVO_SITE_URL)['host']) {
-                $this->getService('ExceptionHandler')->messageQuit(
-                    'External or invalid redirect not allowed: <i>' . htmlspecialchars($url) . '</i>'
-                );
-            }
+        if (!$this->isLocalRedirectTarget($url, EVO_SITE_URL)) {
+            $this->getService('ExceptionHandler')->messageQuit(
+                'External or invalid redirect not allowed: <i>' . htmlspecialchars($url) . '</i>'
+            );
         }
 
         // Fix: Prevent header injection by checking for newlines in all redirect types
@@ -557,6 +553,58 @@ public function sendRedirect(string $url, int $count_attempts = 0, string $type
         exit(0);
     }
 
+    /**
+     * Decide whether a redirect target stays on this site.
+     *
+     * Relative paths pass, absolute URLs pass only when the host is our own. Everything else
+     * is refused - including the two shapes that carry no scheme and therefore used to skip
+     * the check entirely: "//evil.tld" is protocol-relative, and browsers normalise the
+     * backslash variants ("/\evil.tld", "\/evil.tld") to the same thing before following
+     * the Location header.
+     *
+     * @param string $url the target as it would be sent to the browser
+     * @param string $siteUrl EVO_SITE_URL, or any absolute URL naming this site
+     * @return bool
+     * @since 3.5.8
+     */
+    public function isLocalRedirectTarget(string $url, string $siteUrl): bool
+    {
+        // The URL parser used by browsers strips ASCII whitespace/control characters before
+        // resolving a target (and strips tabs and newlines within it). parse_url() does not, so
+        // accepting them would let a value such as " //evil.tld" masquerade as a relative path.
+        if (preg_match('/[\x00-\x20\x7f]/', $url) === 1) {
+            return false;
+        }
+
+        // Browsers read a backslash in the authority position as a slash, so the check has to
+        // read it that way too before deciding whether an authority is present at all.
+        $normalized = str_replace('\\', '/', $url);
+
+        $parsed = parse_url($normalized);
+        if ($parsed === false) {
+            return false;
+        }
+
+        $hasAuthority = isset($parsed['host']) || str_starts_with($normalized, '//');
+
+        if (!isset($parsed['scheme']) && !$hasAuthority) {
+            return true; // a relative path, which can only stay on this site
+        }
+
+        if (isset($parsed['scheme']) && !in_array(strtolower($parsed['scheme']), ['http', 'https'], true)) {
+            return false;
+        }
+
+        $host = isset($parsed['host']) ? (string) $parsed['host'] : '';
+        if ($host === '') {
+            return false; // "//" with nothing behind it, or an authority we could not read
+        }
+
+        $siteHost = parse_url($siteUrl, PHP_URL_HOST);
+
+        return is_string($siteHost) && $siteHost !== '' && strcasecmp($host, $siteHost) === 0;
+    }
+
     /**
      * Forwards request processing to another document id within the current request.
      *

core/src/Support/InstallerCompletion.php

@@ -0,0 +1,78 @@
+<?php
+
+namespace EvolutionCMS\Support;
+
+final class InstallerCompletion
+{
+    public static function writeLock(
+        string $lockFile,
+        string $sessionId,
+        string $ip,
+        int $timestamp,
+        string $token
+    ): bool {
+        if (preg_match('/\A[0-9a-f]{64}\z/D', $token) !== 1) {
+            return false;
+        }
+
+        $content = "<?php\n\$install_session = " . var_export($sessionId, true) . ";\n"
+            . "\$install_ip = " . var_export($ip, true) . ";\n"
+            . "\$install_timestamp = $timestamp;\n"
+            . "\$install_token = '$token';\n";
+
+        return file_put_contents($lockFile, $content, LOCK_EX) !== false;
+    }
+
+    /**
+     * @return array{ip: string, timestamp: int, token: string}|null
+     */
+    public static function readLock(string $lockFile): ?array
+    {
+        if (!is_file($lockFile)) {
+            return null;
+        }
+
+        $install_ip = $install_token = null;
+        $install_timestamp = 0;
+
+        include $lockFile;
+
+        if (
+            !is_string($install_ip)
+            || !is_string($install_token)
+            || preg_match('/\A[0-9a-f]{64}\z/D', $install_token) !== 1
+        ) {
+            return null;
+        }
+
+        return [
+            'ip' => $install_ip,
+            'timestamp' => (int) $install_timestamp,
+            'token' => $install_token,
+        ];
+    }
+
+    /**
+     * @param array{ip: string, timestamp: int, token: string}|null $lock
+     */
+    public static function matches(
+        ?array $lock,
+        string $token,
+        string $ip,
+        int $now,
+        int $maxLifetime
+    ): bool {
+        if ($lock === null || $token === '') {
+            return false;
+        }
+
+        if ($maxLifetime <= 0) {
+            $maxLifetime = 1440;
+        }
+
+        return $lock['timestamp'] > 0
+            && $now <= $lock['timestamp'] + $maxLifetime
+            && $ip === $lock['ip']
+            && hash_equals($lock['token'], $token);
+    }
+}

core/tests/Feature/InstallerWebPathTest.php

@@ -0,0 +1,107 @@
+<?php
+
+use EvolutionCMS\Support\InstallerCompletion;
+use Illuminate\Filesystem\Filesystem;
+use Symfony\Component\Process\Process;
+
+require_once dirname(__DIR__, 2) . '/src/Support/InstallerCompletion.php';
+
+/**
+ * @return array{root: string, lock: string, install: string}
+ */
+function installerWebFixture(): array
+{
+    $root = str_replace('\\', '/', sys_get_temp_dir()) . '/evo-installer-web-' . bin2hex(random_bytes(8));
+    $install = $root . '/install';
+    mkdir($install, 0777, true);
+    file_put_contents($install . '/index.php', '<?php // installer fixture');
+
+    return [
+        'root' => $root,
+        'lock' => $root . '/install.session.php',
+        'install' => $install,
+    ];
+}
+
+/**
+ * Run the real standalone processor with a temporary site root.
+ *
+ * @param array<string, string> $post
+ * @param array<string, string> $cookies
+ */
+function runInstallerRemoval(array $fixture, array $post, array $cookies = [], string $method = 'POST'): Process
+{
+    $processor = str_replace('\\', '/', dirname(__DIR__, 3))
+        . '/manager/processors/remove_installer.processor.php';
+    $runner = $fixture['root'] . '/request.php';
+    $source = "<?php\n"
+        . "define('EVO_BASE_PATH', " . var_export($fixture['root'] . '/', true) . ");\n"
+        . "\$_SERVER['REQUEST_METHOD'] = " . var_export($method, true) . ";\n"
+        . "\$_SERVER['REMOTE_ADDR'] = '203.0.113.10';\n"
+        . "\$_POST = " . var_export($post, true) . ";\n"
+        . "\$_COOKIE = " . var_export($cookies, true) . ";\n"
+        . "require " . var_export($processor, true) . ";\n";
+    file_put_contents($runner, $source);
+
+    $process = new Process([PHP_BINARY, $runner]);
+    $process->run();
+
+    return $process;
+}
+
+function removeInstallerWebFixture(array $fixture): void
+{
+    (new Filesystem())->deleteDirectory($fixture['root']);
+}
+
+test('fresh web install requires successful completion before its installer can be removed', function () {
+    $fixture = installerWebFixture();
+    $token = str_repeat('a', 64);
+
+    try {
+        file_put_contents(
+            $fixture['lock'],
+            "<?php\n\$install_session = 'anonymous-session';\n"
+            . "\$install_ip = '203.0.113.10';\n"
+            . "\$install_timestamp = " . time() . ";\n"
+        );
+
+        $beforeCompletion = runInstallerRemoval($fixture, [
+            'installer_token' => $token,
+            'rminstaller' => '1',
+        ]);
+
+        expect($beforeCompletion->isSuccessful())->toBeTrue()
+            ->and($beforeCompletion->getOutput())->toContain('Not found.')
+            ->and(is_dir($fixture['install']))->toBeTrue();
+
+        expect(InstallerCompletion::writeLock(
+            $fixture['lock'],
+            'anonymous-session',
+            '203.0.113.10',
+            time(),
+            $token
+        ))->toBeTrue();
+
+        $getAttempt = runInstallerRemoval($fixture, [
+            'installer_token' => $token,
+            'rminstaller' => '1',
+        ], [], 'GET');
+
+        expect($getAttempt->isSuccessful())->toBeTrue()
+            ->and($getAttempt->getOutput())->toContain('Not found.')
+            ->and(is_dir($fixture['install']))->toBeTrue();
+
+        $afterCompletion = runInstallerRemoval($fixture, [
+            'installer_token' => $token,
+            'rminstaller' => '1',
+        ]);
+
+        expect($afterCompletion->isSuccessful())->toBeTrue()
+            ->and($afterCompletion->getOutput())->toContain("window.location='../#?a=2'")
+            ->and(is_dir($fixture['install']))->toBeFalse()
+            ->and(is_file($fixture['lock']))->toBeFalse();
+    } finally {
+        removeInstallerWebFixture($fixture);
+    }
+});