diff --git a/app/Config/Publisher.php b/app/Config/Publisher.php new file mode 100644 index 000000000000..2588eea2abac --- /dev/null +++ b/app/Config/Publisher.php @@ -0,0 +1,28 @@ + + */ + public $restrictions = [ + ROOTPATH => '*', + FCPATH => '#\.(?css|js|map|htm?|xml|json|webmanifest|tff|eot|woff?|gif|jpe?g|tiff?|png|webp|bmp|ico|svg)$#i', + ]; +} diff --git a/system/Config/Publisher.php b/system/Config/Publisher.php new file mode 100644 index 000000000000..651600ef9c06 --- /dev/null +++ b/system/Config/Publisher.php @@ -0,0 +1,42 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace CodeIgniter\Config; + +/** + * Publisher Configuration + * + * Defines basic security restrictions for the Publisher class + * to prevent abuse by injecting malicious files into a project. + */ +class Publisher extends BaseConfig +{ + /** + * A list of allowed destinations with a (pseudo-)regex + * of allowed files for each destination. + * Attempts to publish to directories not in this list will + * result in a PublisherException. Files that do no fit the + * pattern will cause copy/merge to fail. + * + * @var array + */ + public $restrictions = [ + ROOTPATH => '*', + FCPATH => '#\.(?css|js|map|htm?|xml|json|webmanifest|tff|eot|woff?|gif|jpe?g|tiff?|png|webp|bmp|ico|svg)$#i', + ]; + + /** + * Disables Registrars to prevent modules from altering the restrictions. + */ + final protected function registerProperties() + { + } +} diff --git a/system/Language/en/Publisher.php b/system/Language/en/Publisher.php index 934342b4f98c..2d7ae8418b25 100644 --- a/system/Language/en/Publisher.php +++ b/system/Language/en/Publisher.php @@ -11,9 +11,11 @@ // Publisher language settings return [ - 'collision' => 'Publisher encountered an unexpected {0} while copying {1} to {2}.', - 'expectedDirectory' => 'Publisher::{0} expects a valid directory.', - 'expectedFile' => 'Publisher::{0} expects a valid file.', + 'collision' => 'Publisher encountered an unexpected {0} while copying {1} to {2}.', + 'expectedDirectory' => 'Publisher::{0} expects a valid directory.', + 'expectedFile' => 'Publisher::{0} expects a valid file.', + 'destinationNotAllowed' => 'Destination is not on the allowed list of Publisher directories: {0}', + 'fileNotAllowed' => '{0} fails the following restriction for {1}: {2}', // Publish Command 'publishMissing' => 'No Publisher classes detected in {0} across all namespaces.', diff --git a/system/Publisher/Exceptions/PublisherException.php b/system/Publisher/Exceptions/PublisherException.php index 8144fcfeac02..d54420881ec7 100644 --- a/system/Publisher/Exceptions/PublisherException.php +++ b/system/Publisher/Exceptions/PublisherException.php @@ -50,4 +50,26 @@ public static function forExpectedFile(string $caller) { return new static(lang('Publisher.expectedFile', [$caller])); } + + /** + * Throws when given a destination that is not in the list of allowed directories. + * + * @param string $destination + */ + public static function forDestinationNotAllowed(string $destination) + { + return new static(lang('Publisher.destinationNotAllowed', [$destination])); + } + + /** + * Throws when a file fails to match the allowed pattern for its destination. + * + * @param string $file + * @param string $directory + * @param string $pattern + */ + public static function forFileNotAllowed(string $file, string $directory, string $pattern) + { + return new static(lang('Publisher.fileNotAllowed', [$file, $directory, $pattern])); + } } diff --git a/system/Publisher/Publisher.php b/system/Publisher/Publisher.php index 7018df15c7d1..1bfd4b91768d 100644 --- a/system/Publisher/Publisher.php +++ b/system/Publisher/Publisher.php @@ -72,6 +72,14 @@ class Publisher */ private $published = []; + /** + * List of allowed directories and their allowed files regex. + * Restrictions are intentionally private to prevent overriding. + * + * @var array + */ + private $restrictions; + /** * Base path to use for the source. * @@ -134,6 +142,8 @@ final public static function discover(string $directory = 'Publishers'): array * @param string $directory * * @return string + * + * @throws PublisherException */ private static function resolveDirectory(string $directory): string { @@ -152,6 +162,8 @@ private static function resolveDirectory(string $directory): string * @param string $file * * @return string + * + * @throws PublisherException */ private static function resolveFile(string $file): string { @@ -191,7 +203,7 @@ private static function filterFiles(array $files, string $directory): array * * @return string[] */ - private static function matchFiles(array $files, string $pattern) + private static function matchFiles(array $files, string $pattern): array { // Convert pseudo-regex into their true form if (@preg_match($pattern, null) === false) // @phpstan-ignore-line @@ -236,49 +248,6 @@ private static function wipeDirectory(string $directory): void } } - /** - * Copies a file with directory creation and identical file awareness. - * Intentionally allows errors. - * - * @param string $from - * @param string $to - * @param boolean $replace - * - * @return void - * - * @throws PublisherException For unresolvable collisions - */ - private static function safeCopyFile(string $from, string $to, bool $replace): void - { - // Check for an existing file - if (file_exists($to)) - { - // If not replacing or if files are identical then consider successful - if (! $replace || same_file($from, $to)) - { - return; - } - - // If it is a directory then do not try to remove it - if (is_dir($to)) - { - throw PublisherException::forCollision($from, $to); - } - - // Try to remove anything else - unlink($to); - } - - // Make sure the directory exists - if (! is_dir($directory = pathinfo($to, PATHINFO_DIRNAME))) - { - mkdir($directory, 0775, true); - } - - // Allow copy() to throw errors - copy($from, $to); - } - //-------------------------------------------------------------------- /** @@ -293,6 +262,20 @@ public function __construct(string $source = null, string $destination = null) $this->source = self::resolveDirectory($source ?? $this->source); $this->destination = self::resolveDirectory($destination ?? $this->destination); + + // Restrictions are intentionally not injected to prevent overriding + $this->restrictions = config('Publisher')->restrictions; + + // Make sure the destination is allowed + foreach (array_keys($this->restrictions) as $directory) + { + if (strpos($this->destination, $directory) === 0) + { + return; + } + } + + throw PublisherException::forDestinationNotAllowed($this->destination); } /** @@ -314,6 +297,8 @@ public function __destruct() * discovery. * * @return boolean + * + * @throws RuntimeException */ public function publish(): bool { @@ -683,7 +668,7 @@ final public function copy(bool $replace = true): bool try { - self::safeCopyFile($file, $to, $replace); + $this->safeCopyFile($file, $to, $replace); $this->published[] = $to; } catch (Throwable $e) @@ -707,7 +692,7 @@ final public function merge(bool $replace = true): bool { $this->errors = $this->published = []; - // Get the file from source for special handling + // Get the files from source for special handling $sourced = self::filterFiles($this->getFiles(), $this->source); // Handle everything else with a flat copy @@ -722,7 +707,7 @@ final public function merge(bool $replace = true): bool try { - self::safeCopyFile($file, $to, $replace); + $this->safeCopyFile($file, $to, $replace); $this->published[] = $to; } catch (Throwable $e) @@ -733,4 +718,56 @@ final public function merge(bool $replace = true): bool return $this->errors === []; } + + /** + * Copies a file with directory creation and identical file awareness. + * Intentionally allows errors. + * + * @param string $from + * @param string $to + * @param boolean $replace + * + * @return void + * + * @throws PublisherException For collisions and restriction violations + */ + private function safeCopyFile(string $from, string $to, bool $replace): void + { + // Verify this is an allowed file for its destination + foreach ($this->restrictions as $directory => $pattern) + { + if (strpos($to, $directory) === 0 && self::matchFiles([$to], $pattern) === []) + { + throw PublisherException::forFileNotAllowed($from, $directory, $pattern); + } + } + + // Check for an existing file + if (file_exists($to)) + { + // If not replacing or if files are identical then consider successful + if (! $replace || same_file($from, $to)) + { + return; + } + + // If it is a directory then do not try to remove it + if (is_dir($to)) + { + throw PublisherException::forCollision($from, $to); + } + + // Try to remove anything else + unlink($to); + } + + // Make sure the directory exists + if (! is_dir($directory = pathinfo($to, PATHINFO_DIRNAME))) + { + mkdir($directory, 0775, true); + } + + // Allow copy() to throw errors + copy($from, $to); + } } diff --git a/tests/_support/Config/Registrar.php b/tests/_support/Config/Registrar.php index 4135079b73e2..d80522ba615f 100644 --- a/tests/_support/Config/Registrar.php +++ b/tests/_support/Config/Registrar.php @@ -113,4 +113,18 @@ public static function Database() return $config; } + + /** + * Demonstrates Publisher security. + * + * @see PublisherRestrictionsTest::testRegistrarsNotAllowed() + * + * @return array + */ + public static function Publisher() + { + return [ + 'restrictions' => [SUPPORTPATH => '*'], + ]; + } } diff --git a/tests/system/Publisher/PublisherInputTest.php b/tests/system/Publisher/PublisherInputTest.php index 553a92b14c53..e7450f5c02a9 100644 --- a/tests/system/Publisher/PublisherInputTest.php +++ b/tests/system/Publisher/PublisherInputTest.php @@ -1,4 +1,4 @@ -root = vfsStream::setup('root', null, $this->structure); + + // Add root to the list of allowed destinations + config('Publisher')->restrictions[$this->root->url()] = '*'; } //-------------------------------------------------------------------- diff --git a/tests/system/Publisher/PublisherRestrictionsTest.php b/tests/system/Publisher/PublisherRestrictionsTest.php new file mode 100644 index 000000000000..27db2e9429a9 --- /dev/null +++ b/tests/system/Publisher/PublisherRestrictionsTest.php @@ -0,0 +1,108 @@ +assertArrayNotHasKey(SUPPORTPATH, config('Publisher')->restrictions); + } + + public function testImmutableRestrictions() + { + $publisher = new Publisher(); + + // Try to "hack" the Publisher by adding our desired destination to the config + config('Publisher')->restrictions[SUPPORTPATH] = '*'; + + $restrictions = $this->getPrivateProperty($publisher, 'restrictions'); + + $this->assertArrayNotHasKey(SUPPORTPATH, $restrictions); + } + + /** + * @dataProvider fileProvider + */ + public function testDefaultPublicRestrictions(string $path) + { + $publisher = new Publisher(ROOTPATH, FCPATH); + $pattern = config('Publisher')->restrictions[FCPATH]; + + // Use the scratch space to create a file + $file = $publisher->getScratch() . $path; + file_put_contents($file, 'To infinity and beyond!'); + + $result = $publisher->addFile($file)->merge(); + $this->assertFalse($result); + + $errors = $publisher->getErrors(); + $this->assertCount(1, $errors); + $this->assertSame([$file], array_keys($errors)); + + $expected = lang('Publisher.fileNotAllowed', [$file, FCPATH, $pattern]); + $this->assertSame($expected, $errors[$file]->getMessage()); + } + + public function fileProvider() + { + yield 'php' => ['index.php']; + yield 'exe' => ['cat.exe']; + yield 'flat' => ['banana']; + } + + /** + * @dataProvider destinationProvider + */ + public function testDestinations(string $destination, bool $allowed) + { + config('Publisher')->restrictions = [ + APPPATH => '', + FCPATH => '', + SUPPORTPATH . 'Files' => '', + SUPPORTPATH . 'Files/../' => '', + ]; + + if (! $allowed) + { + $this->expectException(PublisherException::class); + $this->expectExceptionMessage(lang('Publisher.destinationNotAllowed', [$destination])); + } + + $publisher = new Publisher(null, $destination); + $this->assertInstanceOf(Publisher::class, $publisher); + } + + public function destinationProvider() + { + return [ + 'explicit' => [ + APPPATH, + true, + ], + 'subdirectory' => [ + APPPATH . 'Config', + true, + ], + 'relative' => [ + SUPPORTPATH . 'Files/able/../', + true, + ], + 'parent' => [ + SUPPORTPATH, + false, + ], + ]; + } +} diff --git a/tests/system/Publisher/PublisherSupportTest.php b/tests/system/Publisher/PublisherSupportTest.php index 2aff0e34d1f6..42f22d721525 100644 --- a/tests/system/Publisher/PublisherSupportTest.php +++ b/tests/system/Publisher/PublisherSupportTest.php @@ -1,4 +1,4 @@ -assertDirectoryExists($directory); + config('Publisher')->restrictions[$directory] = ''; // Allow the directory $publisher = new Publisher($this->directory, $directory); $publisher->wipe(); diff --git a/user_guide_src/source/libraries/publisher.rst b/user_guide_src/source/libraries/publisher.rst index 1cb27b54aa2b..84b430ba144b 100644 --- a/user_guide_src/source/libraries/publisher.rst +++ b/user_guide_src/source/libraries/publisher.rst @@ -107,6 +107,20 @@ Most of the time you will not need to handle your own discovery, just use the pr By default on your class extension ``publish()`` will add all files from your ``$source`` and merge them out to your destination, overwriting on collision. +Security +======== + +In order to prevent modules from injecting malicious code into your projects, ``Publisher`` contains a config file +that defines which directories and file patterns are allowed as destinations. By default, files may only be published +to your project (to prevent access to the rest of the filesystem), and the **public/** folder (``FCPATH``) will only +receive files with the following extensions: +* Web assets: css, scss, js, map +* Non-executable web files: htm, html, xml, json, webmanifest +* Fonts: tff, eot, woff +* Images: gif, jpg, jpeg, tiff, png, webp, bmp, ico, svg + +If you need to add or adjust the security for your project then alter the ``$restrictions`` property of ``Config\Publisher``. + ******** Examples ******** @@ -159,7 +173,7 @@ Asset Dependencies Example ========================== You want to integrate the frontend library "Bootstrap" into your project, but the frequent updates makes it a hassle -to keep up with. You can create a publication definition in your project to sync frontend assets by adding extending +to keep up with. You can create a publication definition in your project to sync frontend assets by extending ``Publisher`` in your project. So **app/Publishers/BootstrapPublisher.php** might look like this::