diff --git a/modules/cms/ServiceProvider.php b/modules/cms/ServiceProvider.php index 89eb07aa96..147a920109 100644 --- a/modules/cms/ServiceProvider.php +++ b/modules/cms/ServiceProvider.php @@ -332,12 +332,14 @@ protected function registerBackendPermissions() 'cms.manage_content' => [ 'label' => 'cms::lang.permissions.manage_content', 'tab' => 'cms::lang.permissions.name', + 'comment' => 'cms::lang.permissions.manage_content_comment', 'roles' => [UserRole::CODE_DEVELOPER], 'order' => 100 ], 'cms.manage_assets' => [ 'label' => 'cms::lang.permissions.manage_assets', 'tab' => 'cms::lang.permissions.name', + 'comment' => 'cms::lang.permissions.manage_assets_comment', 'roles' => [UserRole::CODE_DEVELOPER], 'order' => 100 ], diff --git a/modules/cms/lang/en/lang.php b/modules/cms/lang/en/lang.php index bbbac06602..36503a33dc 100644 --- a/modules/cms/lang/en/lang.php +++ b/modules/cms/lang/en/lang.php @@ -276,7 +276,9 @@ 'permissions' => [ 'name' => 'CMS', 'manage_content' => 'Manage website content files', + 'manage_content_comment' => 'This permission should only be given to trusted users, as it allows direct access to the theme\'s content files.', 'manage_assets' => 'Manage website assets - images, JavaScript files, CSS files', + 'manage_assets_comment' => 'This permission should only be given to trusted users, as it allows direct access to the theme\'s asset files, which are combined and served publicly.', 'manage_pages' => 'Create, modify and delete website pages', 'manage_pages_comment' => 'This permission should only be given to trusted users, as it allows direct access to the theme\'s page content files, including PHP code if enabled.', 'manage_layouts' => 'Create, modify and delete CMS layouts', diff --git a/modules/system/classes/CombineAssets.php b/modules/system/classes/CombineAssets.php index 8fa9f0d3fe..bb185bad4a 100644 --- a/modules/system/classes/CombineAssets.php +++ b/modules/system/classes/CombineAssets.php @@ -18,6 +18,7 @@ use Assetic\Filter\CssRewriteFilter; use Assetic\Filter\JavaScriptMinifierFilter; use Assetic\Filter\StylesheetMinifyFilter; +use Winter\Storm\Filesystem\PathResolver; use Winter\Storm\Parse\Assetic\Cache\FilesystemCache; use Winter\Storm\Parse\Assetic\Filter\LessCompiler; use Winter\Storm\Parse\Assetic\Filter\ScssCompiler; @@ -130,30 +131,44 @@ public function init() $this->useDeepHashing = Config::get('app.debug', false); } + // Constrain asset import directives to known asset trees. Without these + // explicit roots, a writable asset could disclose arbitrary server-readable + // files: `@import (inline) ""` in a .less file (GHSA-58fp-mcx6-7qf9), + // `=include ../../../.env` in a .js file (GHSA-2223-f22x-24cq), or an + // `@import` traversal in a .css file. The asset's own source directory is + // always allowed implicitly; this list adds the cross-tree roots that + // legitimate themes/plugins/modules actually import from (e.g. a plugin + // importing a module asset, or a theme importing its own ../vendor). + $allowedImportRoots = [ + themes_path(), + plugins_path(), + base_path('modules'), + ]; + /* * Register JavaScript filters */ - $this->registerFilter('js', new JavascriptImporter); + $jsImporter = new JavascriptImporter; + $jsImporter->setAllowedImportRoots($allowedImportRoots); + $this->registerFilter('js', $jsImporter); /* * Register CSS filters */ - $this->registerFilter('css', new CssImportFilter); + $cssImportFilter = new CssImportFilter; + // Assetic's CssImportFilter resolves `@import` targets relative to the source + // with `..` traversal allowed; confine the resolved path to the allowed roots. + $cssImportFilter->setImportValidator(function ($path) use ($allowedImportRoots) { + $resolved = PathResolver::resolve($path); + + return $resolved !== false + && PathResolver::withinAny($resolved, $allowedImportRoots); + }); + $this->registerFilter('css', $cssImportFilter); $this->registerFilter(['css', 'less', 'scss'], new CssRewriteFilter); - // Constrain @import resolution to known asset trees. Without these - // explicit roots, `@import (inline) ""` in a writable .less file - // would disclose any server-readable file. The asset's own source root - // is always allowed implicitly; this list adds the cross-tree roots - // that legitimate themes/plugins actually use (e.g. a plugin importing - // a module .less, or a theme importing its own ../vendor). See - // GHSA-58fp-mcx6-7qf9. $lessCompiler = new LessCompiler; - $lessCompiler->setAllowedImportRoots([ - themes_path(), - plugins_path(), - base_path('modules'), - ]); + $lessCompiler->setAllowedImportRoots($allowedImportRoots); $this->registerFilter('less', $lessCompiler); $this->registerFilter('scss', new ScssCompiler); diff --git a/modules/system/tests/classes/CombineAssetsTest.php b/modules/system/tests/classes/CombineAssetsTest.php index a4cbe44747..7491fa1cc4 100644 --- a/modules/system/tests/classes/CombineAssetsTest.php +++ b/modules/system/tests/classes/CombineAssetsTest.php @@ -169,6 +169,85 @@ public function testLessCompilerAllowsLegitimatePartial() } } + /** + * Regression for GHSA-2223-f22x-24cq. A writable theme `.js` asset containing + * `=include ../../../.env` must not disclose server files through the combiner, + * whose output is served unauthenticated via the `combine/{file}` route. + */ + public function testJavascriptImporterBlocksTraversalImport() + { + $themeDir = $this->makeTempThemeDir(); + // A real `.js` secret outside the theme subtree (but under base_path so + // Assetic's FileAsset root check passes). It escapes via `..` traversal but + // lands outside every allowed import root, so it must not be inlined. + $secretPath = dirname($themeDir) . '/js-secret-' . bin2hex(random_bytes(4)) . '.js'; + file_put_contents($secretPath, 'var LEAK = "combine-leak-canary";'); + file_put_contents( + $themeDir . '/assets/poc.js', + "/*\n=include ../../" . basename($secretPath) . "\n*/\n" + ); + + try { + $js = $this->compileJsTo($themeDir, 'assets/poc.js'); + $this->assertStringNotContainsString('combine-leak-canary', $js); + } finally { + @unlink($secretPath); + \File::deleteDirectory($themeDir); + } + } + + /** + * The `.js`-only extension gate must block disclosure of non-JS files (e.g. + * `.env`) before any path resolution, even inside an otherwise reachable tree. + */ + public function testJavascriptImporterBlocksDisallowedExtension() + { + $themeDir = $this->makeTempThemeDir(); + $secretPath = dirname($themeDir) . '/js-secret-' . bin2hex(random_bytes(4)) . '.env'; + file_put_contents($secretPath, "APP_KEY=combine-leak-canary\n"); + file_put_contents( + $themeDir . '/assets/poc.js', + "/*\n=include ../../" . basename($secretPath) . "\n*/\n" + ); + + try { + $js = $this->compileJsTo($themeDir, 'assets/poc.js'); + $this->assertStringNotContainsString('combine-leak-canary', $js); + } finally { + @unlink($secretPath); + \File::deleteDirectory($themeDir); + } + } + + /** + * Legitimate same-tree `=include partial.js` must still resolve, otherwise the + * hardening would break every asset that composes its own bundle. + */ + public function testJavascriptImporterAllowsSameTreeInclude() + { + $themeDir = $this->makeTempThemeDir(); + file_put_contents($themeDir . '/assets/partial.js', 'var PARTIAL = "partial-marker";'); + file_put_contents($themeDir . '/assets/main.js', "/*\n=include partial.js\n*/\nvar MAIN = 1;"); + + try { + $js = $this->compileJsTo($themeDir, 'assets/main.js'); + $this->assertStringContainsString('partial-marker', $js); + } finally { + \File::deleteDirectory($themeDir); + } + } + + protected function compileJsTo(string $themeDir, string $relativeAsset): string + { + $dest = sys_get_temp_dir() . '/winter-combine-out-' . bin2hex(random_bytes(4)) . '.js'; + try { + CombineAssets::instance()->combineToFile([$relativeAsset], $dest, $themeDir); + return file_get_contents($dest) ?: ''; + } finally { + @unlink($dest); + } + } + /** @var string */ protected $lastSecretPath = '';