diff --git a/app/Services/ThemeHooksValidator.php b/app/Services/ThemeHooksValidator.php index 73ab3e65d..2557524ef 100644 --- a/app/Services/ThemeHooksValidator.php +++ b/app/Services/ThemeHooksValidator.php @@ -10,18 +10,44 @@ class ThemeHooksValidator * Dangerous PHP functions that are blocked in hooks.php. */ private const DANGEROUS_FUNCTIONS = [ + // OS command & process execution 'system', 'exec', 'passthru', 'shell_exec', 'popen', 'proc_open', - 'pcntl_exec', 'assert', 'create_function', 'eval', - // BUG FIX: call_user_func / call_user_func_array bypass (Severity: Critical) + 'pcntl_exec', 'pcntl_fork', 'pcntl_alarm', 'pcntl_signal', 'pcntl_wait', + 'pcntl_waitpid', 'pcntl_wexitstatus', 'dl', 'assert', 'create_function', 'eval', + // Dynamic callback & indirect invocation 'call_user_func', 'call_user_func_array', + 'forward_static_call', 'forward_static_call_array', + 'register_shutdown_function', 'register_tick_function', + 'preg_replace_callback', 'preg_replace_callback_array', 'mb_ereg_replace_callback', + 'array_map', 'array_filter', 'array_reduce', 'array_walk', 'array_walk_recursive', + 'unserialize', + // File system & I/O 'file_put_contents', 'file_get_contents', 'fopen', 'fwrite', 'fputs', 'unlink', 'mkdir', 'rmdir', 'rename', 'copy', 'chmod', 'chown', - 'symlink', 'link', 'tmpfile', 'move_uploaded_file', + 'symlink', 'link', 'tmpfile', 'move_uploaded_file', 'touch', + 'readfile', 'file', 'fpassthru', 'highlight_file', 'show_source', + 'fileperms', 'fileowner', 'filegroup', 'chgrp', 'lchown', 'lchgrp', + 'glob', 'scandir', 'opendir', 'readdir', 'dir', + // Environment, configuration & variable manipulation 'extract', 'parse_str', 'putenv', 'ini_set', 'ini_alter', 'header', 'setcookie', 'define', 'defined', - 'base64_decode', 'urldecode', + // Encoding / Obfuscation helpers + 'base64_decode', 'urldecode', 'hex2bin', + 'gzinflate', 'gzuncompress', 'gzdecode', 'str_rot13', 'convert_uudecode', + ]; + + /** + * Dangerous PHP language construct tokens that are blocked in hooks.php. + */ + private const DANGEROUS_LANGUAGE_TOKENS = [ + T_EVAL, + T_INCLUDE, + T_INCLUDE_ONCE, + T_REQUIRE, + T_REQUIRE_ONCE, + T_HALT_COMPILER, ]; /** @@ -32,7 +58,7 @@ class ThemeHooksValidator ]; /** - * Scan ZIP archive for files with dangerous PHP extensions. + * Scan ZIP archive for files with dangerous PHP extensions and malicious blade templates. * Must be called BEFORE extractTo(). * * @throws \RuntimeException if dangerous files are detected. @@ -44,8 +70,26 @@ public function scanZipForPhp(\ZipArchive $zip): void for ($i = 0; $i < $zip->numFiles; $i++) { $filename = $zip->getNameIndex($i); - // Skip Blade template files — they are safe Laravel templates + // Skip directories + if (str_ends_with($filename, '/')) { + continue; + } + + // Validate Blade template files if (str_ends_with($filename, '.blade.php')) { + // Blade templates must be inside resources/views/ + if (! str_contains($filename, 'resources/views/')) { + $dangerousFiles[] = "{$filename} (Blade template must reside in resources/views/)"; + continue; + } + + // Scan Blade content for PHP/Blade execution blocks containing backtick operator + $bladeContent = $zip->getFromIndex($i); + if ($bladeContent !== false && preg_match('/(<\?php|@php|\{\{|\{!!)[^>}]*`/', $bladeContent)) { + $dangerousFiles[] = "{$filename} (Backtick execution operator detected in PHP/Blade directive)"; + continue; + } + continue; } @@ -144,12 +188,28 @@ public function loadHooks(string $themePath): void } /** - * Validate hooks.php source code using token_get_all(). + * Validate hooks.php source code using multi-layer token and string analysis. * * Returns ['valid' => bool, 'reason' => string, 'details' => array] */ public function validateSource(string $source, string $themeName): array { + // === LAYER 1: Raw character scan === + // Reject backtick execution operators immediately + if (str_contains($source, '`')) { + return [ + 'valid' => false, + 'reason' => 'Backtick execution operator is not allowed', + 'details' => [ + [ + 'function' => 'backtick_execution', + 'line' => 1, + ], + ], + ]; + } + + // === LAYER 2: Token-based AST analysis === $tokens = @token_get_all($source); if (! is_array($tokens)) { return ['valid' => false, 'reason' => 'hooks.php cannot be parsed', 'details' => []]; @@ -161,70 +221,151 @@ public function validateSource(string $source, string $themeName): array $hasBadCharacter = false; for ($i = 0; $i < $count; $i++) { - if (! is_array($tokens[$i])) { + $token = $tokens[$i]; + + // Handle single-character literal tokens (e.g. '`', '$', '(', '}', ']', etc.) + if (! is_array($token)) { + if ($token === '`') { + $dangerousCalls[] = [ + 'function' => 'backtick_execution', + 'line' => 1, + ]; + } elseif ($token === '$') { + // Check for variable variables like $$var + $nextIdx = $this->getNextMeaningfulTokenIndex($tokens, $i, $count); + if ($nextIdx < $count && is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_VARIABLE) { + $dangerousCalls[] = [ + 'function' => 'variable_variable:' . $tokens[$nextIdx][1], + 'line' => $tokens[$nextIdx][2], + ]; + } + } elseif ($token === '}' || $token === ']') { + // Detect: ${"system"}(), ${'system'}(), ${$a}() → '}(' + // Detect: $_GET["cmd"](), $a['fn']() → '](' + // This catches bypass payloads A, B, C, D from the security review. + $nextIdx = $this->getNextMeaningfulTokenIndex($tokens, $i, $count); + if ($nextIdx < $count && ! is_array($tokens[$nextIdx]) && $tokens[$nextIdx] === '(') { + $dangerousCalls[] = [ + 'function' => $token === '}' ? 'curly_expression_function_call' : 'array_subscript_function_call', + 'line' => 1, + ]; + } + } continue; } + $tokenId = $token[0]; + $tokenText = $token[1]; + $tokenLine = $token[2]; + + // Double check for backtick inside any token text + if (str_contains($tokenText, '`')) { + $dangerousCalls[] = [ + 'function' => 'backtick_execution', + 'line' => $tokenLine, + ]; + } + // Detect syntax errors via T_BAD_CHARACTER - if ($tokens[$i][0] === T_BAD_CHARACTER) { + if ($tokenId === T_BAD_CHARACTER) { $hasBadCharacter = true; } // Reject inline HTML — hooks.php should be pure PHP - if ($tokens[$i][0] === T_INLINE_HTML) { - $content = trim((string) $tokens[$i][1]); + if ($tokenId === T_INLINE_HTML) { + $content = trim((string) $tokenText); if ($content !== '') { $hasInlineHtml = true; } } - // Detect eval() — T_EVAL is a language construct token, not T_STRING - if ($tokens[$i][0] === T_EVAL) { + // Detect dangerous language constructs (eval, include, require, etc.) + if (in_array($tokenId, self::DANGEROUS_LANGUAGE_TOKENS, true)) { $dangerousCalls[] = [ - 'function' => 'eval', - 'line' => $tokens[$i][2], + 'function' => token_name($tokenId), + 'line' => $tokenLine, ]; continue; } - // Look for function calls: T_STRING followed by '(' - if ($tokens[$i][0] === T_STRING) { - $funcName = strtolower((string) $tokens[$i][1]); + // Detect dynamic class instantiation: new $var() + if ($tokenId === T_NEW) { + $nextIdx = $this->getNextMeaningfulTokenIndex($tokens, $i, $count); + if ($nextIdx < $count && is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_VARIABLE) { + $dangerousCalls[] = [ + 'function' => 'dynamic_instantiation:' . $tokens[$nextIdx][1], + 'line' => $tokenLine, + ]; + } + continue; + } - // Check if the next non-whitespace token is '(' - $nextIdx = $i + 1; - while ($nextIdx < $count && is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_WHITESPACE) { - $nextIdx++; + // Detect dynamic method/property calls: $obj->$method or $obj->{$method} + $isNullsafe = defined('T_NULLSAFE_OBJECT_OPERATOR') && $tokenId === T_NULLSAFE_OBJECT_OPERATOR; + if ($tokenId === T_OBJECT_OPERATOR || $isNullsafe) { + $nextIdx = $this->getNextMeaningfulTokenIndex($tokens, $i, $count); + if ($nextIdx < $count) { + if (is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_VARIABLE) { + $dangerousCalls[] = [ + 'function' => 'dynamic_member_access:' . $tokens[$nextIdx][1], + 'line' => $tokenLine, + ]; + } elseif (! is_array($tokens[$nextIdx]) && $tokens[$nextIdx] === '{') { + $dangerousCalls[] = [ + 'function' => 'dynamic_member_expression', + 'line' => $tokenLine, + ]; + } } + continue; + } + + // Detect dynamic static calls: Class::$method() or $class::$method() + if ($tokenId === T_PAAMAYIM_NEKUDOTAYIM) { + $nextIdx = $this->getNextMeaningfulTokenIndex($tokens, $i, $count); + if ($nextIdx < $count) { + if (is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_VARIABLE) { + $dangerousCalls[] = [ + 'function' => 'dynamic_static_call:' . $tokens[$nextIdx][1], + 'line' => $tokenLine, + ]; + } elseif (! is_array($tokens[$nextIdx]) && $tokens[$nextIdx] === '$') { + $dangerousCalls[] = [ + 'function' => 'dynamic_static_expression', + 'line' => $tokenLine, + ]; + } + } + continue; + } + + // Look for function calls: T_STRING followed by '(' + if ($tokenId === T_STRING) { + $funcName = strtolower((string) $tokenText); + $nextIdx = $this->getNextMeaningfulTokenIndex($tokens, $i, $count); if ($nextIdx < $count && ! is_array($tokens[$nextIdx]) && $tokens[$nextIdx] === '(') { // Skip if this T_STRING is part of a function/method definition - $prevIdx = $i - 1; - while ($prevIdx >= 0 && is_array($tokens[$prevIdx]) && $tokens[$prevIdx][0] === T_WHITESPACE) { - $prevIdx--; - } + $prevIdx = $this->getPrevMeaningfulTokenIndex($tokens, $i); $isDefinition = ($prevIdx >= 0 && is_array($tokens[$prevIdx]) && $tokens[$prevIdx][0] === T_FUNCTION); if (! $isDefinition && in_array($funcName, self::DANGEROUS_FUNCTIONS, true)) { $dangerousCalls[] = [ 'function' => $funcName, - 'line' => $tokens[$i][2], + 'line' => $tokenLine, ]; } } } - // BUG FIX (High): Detect variable function calls, e.g. $f('id') or $obj->method(). - if ($tokens[$i][0] === T_VARIABLE) { - $nextIdx = $i + 1; - while ($nextIdx < $count && is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_WHITESPACE) { - $nextIdx++; - } + // Detect variable function calls, e.g. $f('id') + if ($tokenId === T_VARIABLE) { + $nextIdx = $this->getNextMeaningfulTokenIndex($tokens, $i, $count); if ($nextIdx < $count && ! is_array($tokens[$nextIdx]) && $tokens[$nextIdx] === '(') { $dangerousCalls[] = [ - 'function' => 'variable_function_call:' . $tokens[$i][1], - 'line' => $tokens[$i][2], + 'function' => 'variable_function_call:' . $tokenText, + 'line' => $tokenLine, ]; } } @@ -256,4 +397,38 @@ public function validateSource(string $source, string $themeName): array return ['valid' => true, 'reason' => '', 'details' => []]; } + + /** + * Get index of next non-whitespace, non-comment token. + */ + private function getNextMeaningfulTokenIndex(array $tokens, int $currentIndex, int $count): int + { + $next = $currentIndex + 1; + while ($next < $count) { + if (is_array($tokens[$next]) && in_array($tokens[$next][0], [T_WHITESPACE, T_COMMENT, T_DOC_COMMENT], true)) { + $next++; + continue; + } + break; + } + + return $next; + } + + /** + * Get index of previous non-whitespace, non-comment token. + */ + private function getPrevMeaningfulTokenIndex(array $tokens, int $currentIndex): int + { + $prev = $currentIndex - 1; + while ($prev >= 0) { + if (is_array($tokens[$prev]) && in_array($tokens[$prev][0], [T_WHITESPACE, T_COMMENT, T_DOC_COMMENT], true)) { + $prev--; + continue; + } + break; + } + + return $prev; + } } diff --git a/tests/Feature/Security/ThemeUploadSecurityTest.php b/tests/Feature/Security/ThemeUploadSecurityTest.php index 5d3248ad6..4a5f9ad9b 100644 --- a/tests/Feature/Security/ThemeUploadSecurityTest.php +++ b/tests/Feature/Security/ThemeUploadSecurityTest.php @@ -2,12 +2,11 @@ namespace Tests\Feature\Security; -use App\Models\User; use App\Models\Themes; +use App\Models\User; use Database\Seeders\RoleSpatieSeeder; use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\Storage; -use Tests\TestCase; use ZipArchive; describe('Theme Upload Security (RCE Prevention)', function () { @@ -218,6 +217,90 @@ @unlink($hooksFile); }); + // ── Issue #57 FIX: Backtick Execution Operator Bypass ── + + test('hooks_with_backtick_execution_is_rejected', function () { + $hooksDir = base_path('themes/default'); + if (! is_dir($hooksDir)) { + mkdir($hooksDir, 0755, true); + } + + $hooksFile = $hooksDir . '/hooks.php'; + $proofFile = $hooksDir . '/rce_proof.txt'; + @unlink($proofFile); + + // PoC payload from Issue #57 + file_put_contents($hooksFile, ' ' . $proofFile . ' 2>&1`;'); + + $theme = Themes::firstOrCreate( + ['name' => 'default'], + [ + 'vendor' => 'opendk', + 'version' => '1.0', + 'description' => 'Default', + 'path' => $hooksDir, + 'system' => true, + 'active' => 0, + ] + ); + + $r = $this->actingAs($this->superAdmin) + ->get(route('setting.themes.activate', $theme)); + + $r->assertStatus(302); + $r->assertSessionHas('error'); + + // Verify that command was NOT executed and proof file was NOT created + expect(file_exists($proofFile))->toBeFalse(); + + @unlink($hooksFile); + @unlink($proofFile); + }); + + test('hooks_with_include_construct_is_rejected', function () { + $hooksDir = base_path('themes/default'); + if (! is_dir($hooksDir)) { + mkdir($hooksDir, 0755, true); + } + + $hooksFile = $hooksDir . '/hooks.php'; + file_put_contents($hooksFile, ' 'default'], + [ + 'vendor' => 'opendk', + 'version' => '1.0', + 'description' => 'Default', + 'path' => $hooksDir, + 'system' => true, + 'active' => 0, + ] + ); + + $r = $this->actingAs($this->superAdmin) + ->get(route('setting.themes.activate', $theme)); + + $r->assertStatus(302); + $r->assertSessionHas('error'); + + @unlink($hooksFile); + }); + + test('upload_with_backtick_in_blade_template_is_rejected', function () { + $file = makeZipWithBacktickBlade(); + $r = $this->actingAs($this->superAdmin) + ->post(route('setting.themes.upload'), ['file' => $file]); + $r->assertJson(['status' => 'error']); + }); + + test('upload_with_misplaced_blade_template_is_rejected', function () { + $file = makeZipWithMisplacedBlade(); + $r = $this->actingAs($this->superAdmin) + ->post(route('setting.themes.upload'), ['file' => $file]); + $r->assertJson(['status' => 'error']); + }); + // ── Helpers ── function makeZipWithPhp(): UploadedFile @@ -233,6 +316,38 @@ function makeZipWithPhp(): UploadedFile return new UploadedFile($path, 'theme.zip', 'application/zip', null, true); } + /** + * Helper — creates a ZIP with backtick in a Blade template. + */ + function makeZipWithBacktickBlade(): UploadedFile + { + $path = sys_get_temp_dir() . DIRECTORY_SEPARATOR . uniqid() . '.zip'; + $zip = new ZipArchive(); + $zip->open($path, ZipArchive::CREATE); + $zip->addFromString('evil-theme/composer.json', '{"name":"evil"}'); + $zip->addFromString('evil-theme/theme.json', '{"api_version":"v1"}'); + $zip->addFromString('evil-theme/resources/views/layouts/evil.blade.php', '