Verified Commit 56fe980b authored by Dave Long's avatar Dave Long
Browse files

Issue #3302448 by prudloff, alexpott: Always rename dot files regardless of... 

Issue #3302448 by prudloff, alexpott: Always rename dot files regardless of the configured file extensions creating defense in depth
parent 851df445

core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php

+6 −11
Original line number Diff line number Diff line
@@ -52,22 +52,17 @@ public function sanitizeName(FileUploadSanitizeNameEvent $event): void { 
    // http://php.net/manual/security.filesystem.nullbytes.php

    $filename = str_replace(chr(0), '', $filename);



    if ($filename !== $event->getFilename()) {

      $event->setFilename($filename)->setSecurityRename();

    }



    // Split up the filename by periods. The first part becomes the basename,

    // the last part the final extension.

    $filename_parts = explode('.', $filename);

    // Remove file basename.

    $filename = array_shift($filename_parts);

    // Remove final extension.

    // Remove final extension. In the case of dot filenames this will be empty.

    $final_extension = (string) array_pop($filename_parts);

    // Check if we're dealing with a dot file that is also an insecure extension

    // e.g. .htaccess. In this scenario there is only one 'part' and the

    // extension becomes the filename. We use the original filename from the

    // event rather than the trimmed version above.

    $insecure_uploads = $this->configFactory->get('system.file')->get('allow_insecure_uploads');

    if (!$insecure_uploads && $final_extension === '' && str_contains($event->getFilename(), '.') && in_array(strtolower($filename), FileSystemInterface::INSECURE_EXTENSIONS, TRUE)) {

      $final_extension = $filename;

      $filename = '';

    }



    $extensions = $event->getAllowedExtensions();

    if (!empty($extensions) && !in_array(strtolower($final_extension), $extensions, TRUE)) {
@@ -81,7 +76,7 @@ public function sanitizeName(FileUploadSanitizeNameEvent $event): void { 
      return;

    }



    if (!$insecure_uploads && in_array(strtolower($final_extension), FileSystemInterface::INSECURE_EXTENSIONS, TRUE)) {

    if (!$this->configFactory->get('system.file')->get('allow_insecure_uploads') && in_array(strtolower($final_extension), FileSystemInterface::INSECURE_EXTENSIONS, TRUE)) {

      if (empty($extensions) || in_array('txt', $extensions, TRUE)) {

        // Add .txt to potentially executable files prior to munging to help

        // prevent exploits. This results in a filenames like filename.php being

core/modules/system/tests/src/Unit/Event/SecurityFileUploadEventSubscriberTest.php

+7 −10
Original line number Diff line number Diff line
@@ -85,12 +85,17 @@ public static function provideFilenames() { 
      'no extension produces no errors' => ['foo', '', 'foo'],

      'filename is munged' => ['foo.phar.png.php.jpg', 'jpg png', 'foo.phar_.png_.php_.jpg'],

      'filename is munged regardless of case' => ['FOO.pHAR.PNG.PhP.jpg', 'jpg png', 'FOO.pHAR_.PNG_.PhP_.jpg'],

      'null bytes are removed' => ['foo' . chr(0) . '.txt' . chr(0), '', 'foo.txt'],

      'null bytes are removed even if some extensions are allowed' => [

        'foo' . chr(0) . '.html' . chr(0),

        'txt',

        'foo.html',

      ],

      'dot files are renamed' => ['.git', '', 'git'],

      'htaccess files are renamed even if allowed' => ['.htaccess', 'htaccess txt', '.htaccess_.txt', '.htaccess'],

      'htaccess files are renamed even if allowed' => ['.htaccess', 'htaccess txt', 'htaccess'],

      '.phtml extension allowed with .phtml file' => ['foo.phtml', 'phtml', 'foo.phtml'],

      '.phtml, .txt extension allowed with .phtml file' => ['foo.phtml', 'phtml txt', 'foo.phtml_.txt', 'foo.phtml'],

      'All extensions allowed with .phtml file' => ['foo.phtml', '', 'foo.phtml_.txt', 'foo.phtml'],

      'dot files are renamed even if allowed and not in security list' => ['.git', 'git', 'git'],

    ];

  }
@@ -147,18 +152,10 @@ public static function provideFilenamesNoMunge() { 
      // The following filename would be rejected by 'FileExtension' constraint

      // and therefore remains unchanged.

      '.php is not munged when it would be rejected' => ['foo.php.php', 'jpg'],

      '.php is not munged when it would be rejected and filename contains null byte character' => [

        'foo.' . chr(0) . 'php.php',

        'jpg',

      ],

      'extension less files are not munged when they would be rejected' => [

        'foo',

        'jpg',

      ],

      'dot files are not munged when they would be rejected' => [

        '.htaccess',

        'jpg png',

      ],

    ];

  }