SA-CORE-2019-004 by alexpott, larowlan, greggles, drumm, mlhess, David_Rothstein, pwolanin

core/lib/Drupal/Core/File/FileSystem.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\Core\File;
 
+use Drupal\Component\Utility\Unicode;
 use Drupal\Core\File\Exception\DirectoryNotReadyException;
 use Drupal\Core\File\Exception\FileException;
 use Drupal\Core\File\Exception\FileExistsException;
@@ -571,6 +572,10 @@ class FileSystem implements FileSystemInterface {
    * {@inheritdoc}
    */
   public function getDestinationFilename($destination, $replace) {
+    $basename = $this->basename($destination);
+    if (!Unicode::validateUtf8($basename)) {
+      throw new FileException(sprintf("Invalid filename '%s'", $basename));
+    }
     if (file_exists($destination)) {
       switch ($replace) {
         case FileSystemInterface::EXISTS_REPLACE:
@@ -578,7 +583,6 @@ class FileSystem implements FileSystemInterface {
           break;
 
         case FileSystemInterface::EXISTS_RENAME:
-          $basename = $this->basename($destination);
           $directory = $this->dirname($destination);
           $destination = $this->createFilename($basename, $directory);
           break;
@@ -595,9 +599,13 @@ class FileSystem implements FileSystemInterface {
    * {@inheritdoc}
    */
   public function createFilename($basename, $directory) {
+    $original = $basename;
     // Strip control characters (ASCII value < 32). Though these are allowed in
     // some filesystems, not many applications handle them well.
     $basename = preg_replace('/[\x00-\x1F]/u', '_', $basename);
+    if (preg_last_error() !== PREG_NO_ERROR) {
+      throw new FileException(sprintf("Invalid filename '%s'", $original));
+    }
     if (substr(PHP_OS, 0, 3) == 'WIN') {
       // These characters are not allowed in Windows filenames.
       $basename = str_replace([':', '*', '?', '"', '<', '>', '|'], '_', $basename);

core/lib/Drupal/Core/File/FileSystemInterface.php

@@ -439,6 +439,9 @@ interface FileSystemInterface {
    * @return string
    *   File path consisting of $directory and a unique filename based off
    *   of $basename.
+   *
+   * @throws \Drupal\Core\File\Exception\FileException
+   *   Implementation may throw FileException or its subtype on failure.
    */
   public function createFilename($basename, $directory);
 
@@ -457,6 +460,9 @@ interface FileSystemInterface {
    * @return string|bool
    *   The destination filepath, or FALSE if the file already exists
    *   and FileSystemInterface::EXISTS_ERROR is specified.
+   *
+   * @throws \Drupal\Core\File\Exception\FileException
+   *   Implementation may throw FileException or its subtype on failure.
    */
   public function getDestinationFilename($destination, $replace);
 

core/modules/file/file.module

@@ -1038,7 +1038,13 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   }
   /** @var \Drupal\Core\File\FileSystemInterface $file_system */
   $file_system = \Drupal::service('file_system');
-  $file->destination = $file_system->getDestinationFilename($destination . $file->getFilename(), $replace);
+  try {
+    $file->destination = $file_system->getDestinationFilename($destination . $file->getFilename(), $replace);
+  }
+  catch (FileException $e) {
+    \Drupal::messenger()->addError(t('The file %filename could not be uploaded because the name is invalid.', ['%filename' => $file->getFilename()]));
+    return FALSE;
+  }
   // If the destination is FALSE then there is an existing file and $replace is
   // set to return an error, so we need to exit.
   if ($file->destination === FALSE) {

core/modules/file/tests/src/Functional/SaveUploadTest.php

@@ -2,7 +2,9 @@
 
 namespace Drupal\Tests\file\Functional;
 
+use Drupal\Component\Render\FormattableMarkup;
 use Drupal\Core\File\FileSystemInterface;
+use Drupal\Core\Url;
 use Drupal\file\Entity\File;
 use Drupal\Tests\TestFileCreationTrait;
 
@@ -371,4 +373,60 @@ class SaveUploadTest extends FileManagedTestBase {
     ]), 'Found upload error log entry.');
   }
 
+  /**
+   * Tests that filenames containing invalid UTF-8 are rejected.
+   */
+  public function testInvalidUtf8FilenameUpload() {
+    $this->drupalGet('file-test/upload');
+
+    // Filename containing invalid UTF-8.
+    $filename = "x\xc0xx.gif";
+
+    $page = $this->getSession()->getPage();
+    $data = [
+      'multipart' => [
+        [
+          'name'     => 'file_test_replace',
+          'contents' => FileSystemInterface::EXISTS_RENAME,
+        ],
+        [
+          'name' => 'form_id',
+          'contents' => '_file_test_form',
+        ],
+        [
+          'name' => 'form_build_id',
+          'contents' => $page->find('hidden_field_selector', ['hidden_field', 'form_build_id'])->getAttribute('value'),
+        ],
+        [
+          'name' => 'form_token',
+          'contents' => $page->find('hidden_field_selector', ['hidden_field', 'form_token'])->getAttribute('value'),
+        ],
+        [
+          'name' => 'op',
+          'contents' => 'Submit',
+        ],
+        [
+          'name'     => 'files[file_test_upload]',
+          'contents' => 'Test content',
+          'filename' => $filename,
+        ],
+      ],
+      'cookies' => $this->getSessionCookies(),
+      'http_errors' => FALSE,
+    ];
+
+    $this->assertFileNotExists('temporary://' . $filename);
+    // Use Guzzle's HTTP client directly so we can POST files without having to
+    // write them to disk. Not all filesystem support writing files with invalid
+    // UTF-8 filenames.
+    $response = $this->getHttpClient()->request('POST', Url::fromUri('base:file-test/upload')->setAbsolute()->toString(), $data);
+
+    $content = (string) $response->getBody();
+    $this->htmlOutput($content);
+    $error_text = new FormattableMarkup('The file %filename could not be uploaded because the name is invalid.', ['%filename' => $filename]);
+    $this->assertContains((string) $error_text, $content);
+    $this->assertContains('Epic upload FAIL!', $content);
+    $this->assertFileNotExists('temporary://' . $filename);
+  }
+
 }

core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php

@@ -3,6 +3,7 @@
 namespace Drupal\KernelTests\Core\File;
 
 use Drupal\Component\PhpStorage\FileStorage;
+use Drupal\Core\File\Exception\FileException;
 use Drupal\Core\File\FileSystemInterface;
 
 /**
@@ -156,6 +157,10 @@ class DirectoryTest extends FileTestBase {
     $this->assertNotEqual($path, $destination, 'A new filepath destination is created when filepath destination already exists with FileSystemInterface::EXISTS_RENAME.', 'File');
     $path = $file_system->getDestinationFilename($destination, FileSystemInterface::EXISTS_ERROR);
     $this->assertEqual($path, FALSE, 'An error is returned when filepath destination already exists with FileSystemInterface::EXISTS_ERROR.', 'File');
+
+    // Invalid UTF-8 causes an exception.
+    $this->setExpectedException(FileException::class, "Invalid filename 'a\xFFtest\x80€.txt'");
+    $file_system->getDestinationFilename("core/misc/a\xFFtest\x80€.txt", FileSystemInterface::EXISTS_REPLACE);
   }
 
   /**

core/tests/Drupal/Tests/Core/File/FileSystemTest.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\Tests\Core\File;
 
+use Drupal\Core\File\Exception\FileException;
 use Drupal\Core\File\FileSystem;
 use Drupal\Core\Site\Settings;
 use Drupal\Tests\UnitTestCase;
@@ -175,4 +176,16 @@ class FileSystemTest extends UnitTestCase {
     $this->assertSame($expected_mode, $actual_mode, $message);
   }
 
+  /**
+   * Tests that invalid UTF-8 results in an exception.
+   *
+   * @covers ::createFilename
+   */
+  public function testInvalidUTF8() {
+    vfsStream::setup('dir');
+    $filename = "a\xFFsdf\x80€" . '.txt';
+    $this->setExpectedException(FileException::class, "Invalid filename '$filename'");
+    $this->fileSystem->createFilename($filename, 'vfs://dir');
+  }
+
 }