diff --git a/core/includes/file.inc b/core/includes/file.inc index 33fd9f8b81..b938dc2118 100644 --- a/core/includes/file.inc +++ b/core/includes/file.inc @@ -176,8 +176,14 @@ function file_build_uri($path) { * * @return string * The potentially modified $filename. + * + * @deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Dispatch a + * \Drupal\Core\File\Event\FileEvents::SANITIZE_NAME event instead. + * + * @see https://www.drupal.org/node/3032541 */ function file_munge_filename($filename, $extensions, $alerts = TRUE) { + @trigger_error('file_munge_filename() is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Dispatch a \Drupal\Core\File\Event\FileEvents::SANITIZE_NAME event instead. See https://www.drupal.org/node/3032541', E_USER_DEPRECATED); $original = $filename; // Allow potentially insecure uploads for very savvy users and admin @@ -229,8 +235,14 @@ function file_munge_filename($filename, $extensions, $alerts = TRUE) { * * @return * An unmunged filename string. + * + * @deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use + * str_replace() instead. + * + * @see https://www.drupal.org/node/3032541 */ function file_unmunge_filename($filename) { + @trigger_error('file_unmunge_filename() is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use str_replace() instead. See https://www.drupal.org/node/3032541', E_USER_DEPRECATED); return str_replace('_.', '.', $filename); } diff --git a/core/lib/Drupal/Core/File/Event/FileEvents.php b/core/lib/Drupal/Core/File/Event/FileEvents.php new file mode 100644 index 0000000000..1e803f1c4c --- /dev/null +++ b/core/lib/Drupal/Core/File/Event/FileEvents.php @@ -0,0 +1,22 @@ +setFilename($filename); + $this->allowedExtensions = trim(strtolower($allowed_extensions)); + } + + /** + * Gets the filename. + * + * @return string + * The filename. + */ + public function getFilename(): string { + return $this->filename; + } + + /** + * Sets the filename. + * + * @param string $filename + * The filename to use for the uploaded file. + * + * @return $this + */ + public function setFilename(string $filename): self { + if ($filename !== basename($filename)) { + throw new \InvalidArgumentException(sprintf('$filename must be a filename with no path information, "%s" provided', $filename)); + } + $this->filename = $filename; + return $this; + } + + /** + * Gets the list of allowed extensions. + * + * @return string + * The list of allowed extensions. + */ + public function getAllowedExtensions(): string { + return $this->allowedExtensions; + } + + /** + * Adds an extension to the list of allowed extensions. + * + * @param string $extension + * The extension to add to the list of allowed extensions. + * + * @return $this + */ + public function addAllowedExtension(string $extension): self { + if ($this->allowedExtensions) { + $this->allowedExtensions .= ' ' . $extension; + } + else { + $this->allowedExtensions = $extension; + } + return $this; + } + + /** + * Sets the security rename flag. + * + * @return $this + */ + public function setSecurityRename(): self { + $this->isSecurityRename = TRUE; + return $this; + } + + /** + * Get the security rename flag. + * + * @return bool + * TRUE if there is a rename for security reasons. + */ + public function isSecurityRename(): bool { + return $this->isSecurityRename; + } + +} diff --git a/core/modules/file/file.module b/core/modules/file/file.module index 0e5c0b3186..2f699d3197 100644 --- a/core/modules/file/file.module +++ b/core/modules/file/file.module @@ -18,6 +18,9 @@ use Drupal\Core\Link; use Drupal\Core\Url; use Drupal\file\Entity\File; +use Drupal\Core\File\Event\FileEvents; +use Drupal\Core\File\Event\FileUploadSanitizeNameEvent; +use Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber; use Drupal\file\FileInterface; use Drupal\Component\Utility\NestedArray; use Drupal\Component\Utility\Unicode; @@ -27,8 +30,13 @@ /** * The regex pattern used when checking for insecure file types. + * + * @deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use + * \Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber::INSECURE_EXTENSION_REGEX. + * + * @see https://www.drupal.org/node/3032541 */ -define('FILE_INSECURE_EXTENSION_REGEX', '/\.(phar|php|pl|py|cgi|asp|js)(\.|$)/i'); +define('FILE_INSECURE_EXTENSION_REGEX', SecurityFileUploadEventSubscriber::INSECURE_EXTENSION_REGEX); // Load all Field module hooks for File. require_once __DIR__ . '/file.field.inc'; @@ -333,8 +341,7 @@ function file_validate_name_length(FileInterface $file) { function file_validate_extensions(FileInterface $file, $extensions) { $errors = []; - $regex = '/\.(' . preg_replace('/ +/', '|', preg_quote($extensions)) . ')$/i'; - if (!preg_match($regex, $file->getFilename())) { + if (!SecurityFileUploadEventSubscriber::validateExtension($file->getFilename(), $extensions)) { $errors[] = t('Only files with the following extensions are allowed: %files-allowed.', ['%files-allowed' => $extensions]); } return $errors; @@ -921,7 +928,7 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL */ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $validators = [], $destination = FALSE, $replace = FileSystemInterface::EXISTS_REPLACE) { $user = \Drupal::currentUser(); - $original_file_name = trim($file_info->getClientOriginalName(), '.'); + $original_file_name = $file_info->getClientOriginalName(); // Check for file upload errors and return FALSE for this file if a lower // level system error occurred. For a complete list of errors: // See http://php.net/manual/features.file-upload.errors.php. @@ -949,24 +956,8 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va return FALSE; } - // Begin building file entity. - $values = [ - 'uid' => $user->id(), - 'status' => 0, - 'filename' => $original_file_name, - 'uri' => $file_info->getRealPath(), - 'filesize' => $file_info->getSize(), - ]; - $guesser = \Drupal::service('file.mime_type.guesser'); - if ($guesser instanceof MimeTypeGuesserInterface) { - $values['filemime'] = $guesser->guessMimeType($values['filename']); - } - else { - $values['filemime'] = $guesser->guess($values['filename']); - @trigger_error('\Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Implement \Symfony\Component\Mime\MimeTypeGuesserInterface instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED); - } - $file = File::create($values); + // Build a list of allowed extensions. $extensions = ''; if (isset($validators['file_validate_extensions'])) { if (isset($validators['file_validate_extensions'][0])) { @@ -982,45 +973,13 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va } else { // No validator was provided, so add one using the default list. - // Build a default non-munged safe list for file_munge_filename(). + // Build a default non-munged safe list for + // \Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber::sanitizeName(). $extensions = 'jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp'; $validators['file_validate_extensions'] = []; $validators['file_validate_extensions'][0] = $extensions; } - // Don't rename if 'allow_insecure_uploads' evaluates to TRUE. - if (!\Drupal::config('system.file')->get('allow_insecure_uploads')) { - if (!empty($extensions)) { - // Munge the filename to protect against possible malicious extension - // hiding within an unknown file type (ie: filename.html.foo). - $file->setFilename(file_munge_filename($file->getFilename(), $extensions)); - } - - // Rename potentially executable files, to help prevent exploits (i.e. will - // rename filename.php.foo and filename.php to filename.php_.foo_.txt and - // filename.php_.txt, respectively). - if (preg_match(FILE_INSECURE_EXTENSION_REGEX, $file->getFilename())) { - // If the file will be rejected anyway due to a disallowed extension, it - // should not be renamed; rather, we'll let file_validate_extensions() - // reject it below. - if (!isset($validators['file_validate_extensions']) || empty(file_validate_extensions($file, $extensions))) { - $file->setMimeType('text/plain'); - $filename = $file->getFilename(); - if (substr($filename, -4) != '.txt') { - // The destination filename will also later be used to create the URI. - $filename .= '.txt'; - } - $file->setFilename(file_munge_filename($filename, $extensions)); - \Drupal::messenger()->addStatus(t('For security reasons, your upload has been renamed to %filename.', ['%filename' => $file->getFilename()])); - // The .txt extension may not be in the allowed list of extensions. We - // have to add it here or else the file upload will fail. - if (!empty($validators['file_validate_extensions'][0])) { - $validators['file_validate_extensions'][0] .= ' txt'; - } - } - } - } - // If the destination is not provided, use the temporary directory. if (empty($destination)) { $destination = 'temporary://'; @@ -1036,22 +995,55 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va return FALSE; } - $file->source = $form_field_name; // A file URI may already have a trailing slash or look like "public://". if (substr($destination, -1) != '/') { $destination .= '/'; } + + // Remember the original filename so we can print a message if it changes. + $event = new FileUploadSanitizeNameEvent($original_file_name, $extensions); + $event_dispatcher = \Drupal::service('event_dispatcher'); + $event_dispatcher->dispatch($event, FileEvents::SANITIZE_NAME); + + if (isset($validators['file_validate_extensions'][0])) { + // @see \Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber::sanitizeName() + $validators['file_validate_extensions'][0] = $event->getAllowedExtensions(); + } + + // Begin building file entity. + $values = [ + 'uid' => $user->id(), + 'status' => 0, + // This will be replaced later with a filename based on the destination. + 'filename' => $event->getFilename(), + 'uri' => $file_info->getRealPath(), + 'filesize' => $file_info->getSize(), + ]; + $file = File::create($values); + /** @var \Drupal\Core\File\FileSystemInterface $file_system */ $file_system = \Drupal::service('file_system'); try { - $file->destination = $file_system->getDestinationFilename($destination . $file->getFilename(), $replace); + // Use the result of the sanitization event as the destination name. + $file->destination = $file_system->getDestinationFilename($destination . $event->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. + + $guesser = \Drupal::service('file.mime_type.guesser'); + if ($guesser instanceof MimeTypeGuesserInterface) { + $file->setMimeType($guesser->guessMimeType($values['filename'])); + } + else { + $file->setMimeType($guesser->guess($values['filename'])); + @trigger_error('\Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Implement \Symfony\Component\Mime\MimeTypeGuesserInterface instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED); + } + $file->source = $form_field_name; + + // If file_destination() returns FALSE then $replace === FILE_EXISTS_ERROR and + // there's an existing file so we need to bail. if ($file->destination === FALSE) { \Drupal::messenger()->addError(t('The file %source could not be uploaded because a file by that name already exists in the destination %directory.', ['%source' => $form_field_name, '%directory' => $destination])); return FALSE; @@ -1088,6 +1080,21 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va return FALSE; } + // Update the filename with any changes as a result of the renaming due to an + // existing file. + $file->setFilename(\Drupal::service('file_system')->basename($file->destination)); + + // If the filename has been modified, let the user know. + if ($file->getFilename() !== $original_file_name) { + if ($event->isSecurityRename()) { + $message = t('For security reasons, your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]); + } + else { + $message = t('Your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]); + } + \Drupal::messenger()->addStatus($message); + } + // Set the permissions on the new file. $file_system->chmod($file->getFileUri()); diff --git a/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php index 3c7180a0b0..a423b3e2d2 100644 --- a/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php @@ -9,11 +9,13 @@ use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\Core\Field\FieldDefinitionInterface; use Drupal\Core\File\Exception\FileException; +use Drupal\Core\File\Event\FileEvents; use Drupal\Core\File\FileSystemInterface; use Drupal\Core\Lock\LockBackendInterface; use Drupal\Core\Session\AccountInterface; use Drupal\Core\Utility\Token; use Drupal\file\FileInterface; +use Drupal\Core\File\Event\FileUploadSanitizeNameEvent; use Drupal\rest\ModifiedResourceResponse; use Drupal\rest\Plugin\ResourceBase; use Drupal\Component\Render\PlainTextOutput; @@ -23,6 +25,7 @@ use Drupal\rest\RequestHandler; use Psr\Log\LoggerInterface; use Symfony\Component\DependencyInjection\ContainerInterface; +use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; @@ -126,6 +129,13 @@ class FileUploadResource extends ResourceBase { */ protected $systemFileConfig; + /** + * The event dispatcher to dispatch the filename sanitize event. + * + * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface + */ + protected $eventDispatcher; + /** * Constructs a FileUploadResource instance. * @@ -155,8 +165,10 @@ class FileUploadResource extends ResourceBase { * The lock service. * @param \Drupal\Core\Config\Config $system_file_config * The system file configuration. + * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher + * The event dispatcher service. */ - public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileSystemInterface $file_system, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, $mime_type_guesser, Token $token, LockBackendInterface $lock, Config $system_file_config) { + public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileSystemInterface $file_system, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, $mime_type_guesser, Token $token, LockBackendInterface $lock, Config $system_file_config, EventDispatcherInterface $event_dispatcher = NULL) { parent::__construct($configuration, $plugin_id, $plugin_definition, $serializer_formats, $logger); $this->fileSystem = $file_system; $this->entityTypeManager = $entity_type_manager; @@ -166,6 +178,11 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition $this->token = $token; $this->lock = $lock; $this->systemFileConfig = $system_file_config; + if (!$event_dispatcher) { + @trigger_error('The event dispatcher service should be passed to FileUploadResource::__construct() since 9.2.0. This will be required in Drupal 10.0.0. See https://www.drupal.org/node/3032541', E_USER_DEPRECATED); + $event_dispatcher = \Drupal::service('event_dispatcher'); + } + $this->eventDispatcher = $event_dispatcher; } /** @@ -185,7 +202,8 @@ public static function create(ContainerInterface $container, array $configuratio $container->get('file.mime_type.guesser'), $container->get('token'), $container->get('lock'), - $container->get('config.factory')->get('system.file') + $container->get('config.factory')->get('system.file'), + $container->get('event_dispatcher') ); } @@ -468,46 +486,14 @@ protected function validate(FileInterface $file, array $validators) { * The prepared/munged filename. */ protected function prepareFilename($filename, array &$validators) { - // Don't rename if 'allow_insecure_uploads' evaluates to TRUE. - if (!$this->systemFileConfig->get('allow_insecure_uploads')) { - if (!empty($validators['file_validate_extensions'][0])) { - // If there is a file_validate_extensions validator and a list of - // valid extensions, munge the filename to protect against possible - // malicious extension hiding within an unknown file type. For example, - // "filename.html.foo". - $filename = file_munge_filename($filename, $validators['file_validate_extensions'][0]); - } - - // Rename potentially executable files, to help prevent exploits (i.e. - // will rename filename.php.foo and filename.php to filename._php._foo.txt - // and filename._php.txt, respectively). - if (preg_match(FILE_INSECURE_EXTENSION_REGEX, $filename)) { - // If the file will be rejected anyway due to a disallowed extension, it - // should not be renamed; rather, we'll let file_validate_extensions() - // reject it below. - $passes_validation = FALSE; - if (!empty($validators['file_validate_extensions'][0])) { - $file = File::create([]); - $file->setFilename($filename); - $passes_validation = empty(file_validate_extensions($file, $validators['file_validate_extensions'][0])); - } - if (empty($validators['file_validate_extensions'][0]) || $passes_validation) { - if ((substr($filename, -4) != '.txt')) { - // The destination filename will also later be used to create the URI. - $filename .= '.txt'; - } - $filename = file_munge_filename($filename, $validators['file_validate_extensions'][0] ?? ''); - - // The .txt extension may not be in the allowed list of extensions. We - // have to add it here or else the file upload will fail. - if (!empty($validators['file_validate_extensions'][0])) { - $validators['file_validate_extensions'][0] .= ' txt'; - } - } - } + $extensions = $validators['file_validate_extensions'][0] ?? ''; + $event = new FileUploadSanitizeNameEvent($filename, $extensions); + $this->eventDispatcher->dispatch($event, FileEvents::SANITIZE_NAME); + if (isset($validators['file_validate_extensions'][0])) { + // @see \Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber::sanitizeName() + $validators['file_validate_extensions'][0] = $event->getAllowedExtensions(); } - - return $filename; + return $event->getFilename(); } /** diff --git a/core/modules/file/tests/src/Functional/SaveUploadTest.php b/core/modules/file/tests/src/Functional/SaveUploadTest.php index 47e4c955ba..d243b0cbf6 100644 --- a/core/modules/file/tests/src/Functional/SaveUploadTest.php +++ b/core/modules/file/tests/src/Functional/SaveUploadTest.php @@ -327,6 +327,65 @@ public function testHandleDangerousFile() { file_test_reset(); } + /** + * Test dangerous file handling. + */ + public function testHandleDotFile() { + $dot_file = $this->siteDirectory . '/.test'; + file_put_contents($dot_file, 'This is a test'); + $config = $this->config('system.file'); + // Allow the .php extension and make sure it gets munged and given a .txt + // extension for safety. Also check to make sure its MIME type was changed. + $edit = [ + 'file_test_replace' => FileSystemInterface::EXISTS_REPLACE, + 'files[file_test_upload]' => \Drupal::service('file_system')->realpath($dot_file), + 'is_image_file' => FALSE, + ]; + $this->drupalGet('file-test/upload'); + $this->submitForm($edit, 'Submit'); + $this->assertSession()->statusCodeEquals(200); + $this->assertSession()->pageTextContains('The specified file .test could not be uploaded'); + $this->assertSession()->pageTextContains('Epic upload FAIL!'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(['validate']); + + $edit = [ + 'file_test_replace' => FileSystemInterface::EXISTS_RENAME, + 'files[file_test_upload]' => \Drupal::service('file_system')->realpath($dot_file), + 'is_image_file' => FALSE, + 'allow_all_extensions' => 'empty_array', + ]; + $this->drupalGet('file-test/upload'); + $this->submitForm($edit, 'Submit'); + $this->assertSession()->statusCodeEquals(200); + $this->assertSession()->pageTextContains('For security reasons, your upload has been renamed to test.'); + $this->assertSession()->pageTextContains('File name is test.'); + $this->assertSession()->pageTextContains('You WIN!'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(['validate', 'insert']); + + // Reset the hook counters. + file_test_reset(); + + // Turn off insecure uploads, then try the same thing as above to ensure dot + // files are renamed regardless. + $config->set('allow_insecure_uploads', 0)->save(); + $this->drupalGet('file-test/upload'); + $this->submitForm($edit, 'Submit'); + $this->assertSession()->statusCodeEquals(200); + $this->assertSession()->pageTextContains('For security reasons, your upload has been renamed to test_0.'); + $this->assertSession()->pageTextContains('File name is test_0.'); + $this->assertSession()->pageTextContains('You WIN!'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(['validate', 'insert']); + + // Reset the hook counters. + file_test_reset(); + } + /** * Test file munge handling. */ @@ -412,7 +471,7 @@ public function testHandleFileMunge() { $this->drupalPostForm('file-test/upload', $edit, 'Submit'); $this->assertSession()->statusCodeEquals(200); $this->assertRaw(t('For security reasons, your upload has been renamed')); - $this->assertRaw(t('File name is @filename', ['@filename' => 'image-test.png.php_.png'])); + $this->assertRaw(t('File name is @filename', ['@filename' => 'image-test.png_.php_.png'])); $this->assertRaw(t('You WIN!')); // Check that the correct hooks were called. @@ -430,7 +489,7 @@ public function testHandleFileMunge() { $this->drupalPostForm('file-test/upload', $edit, 'Submit'); $this->assertSession()->statusCodeEquals(200); $this->assertRaw(t('For security reasons, your upload has been renamed')); - $this->assertRaw(t('File name is @filename.', ['@filename' => 'image-test.png_.php_.png_.txt'])); + $this->assertRaw(t('File name is @filename.', ['@filename' => 'image-test.png_.php__0.png'])); $this->assertRaw(t('You WIN!')); // Check that the correct hooks were called. diff --git a/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php index a887aa7c96..1b46c0293c 100644 --- a/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php @@ -8,6 +8,8 @@ use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Entity\Plugin\DataType\EntityAdapter; use Drupal\Core\Field\FieldDefinitionInterface; +use Drupal\Core\File\Event\FileEvents; +use Drupal\Core\File\Event\FileUploadSanitizeNameEvent; use Drupal\Core\File\Exception\FileException; use Drupal\Core\Validation\DrupalTranslator; use Drupal\file\FileInterface; @@ -26,6 +28,7 @@ use Symfony\Component\HttpKernel\Exception\HttpException; use Symfony\Component\Mime\MimeTypeGuesserInterface; use Symfony\Component\Validator\ConstraintViolation; +use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; /** * Reads data from an upload stream and creates a corresponding file entity. @@ -98,6 +101,13 @@ class TemporaryJsonapiFileFieldUploader { */ protected $systemFileConfig; + /** + * The event dispatcher. + * + * @var \Symfony\Contracts\EventDispatcher\EventDispatcherInterface + */ + protected $eventDispatcher; + /** * Constructs a FileUploadResource instance. * @@ -114,13 +124,17 @@ class TemporaryJsonapiFileFieldUploader { * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory * The config factory. */ - public function __construct(LoggerInterface $logger, FileSystemInterface $file_system, $mime_type_guesser, Token $token, LockBackendInterface $lock, ConfigFactoryInterface $config_factory) { + public function __construct(LoggerInterface $logger, FileSystemInterface $file_system, $mime_type_guesser, Token $token, LockBackendInterface $lock, ConfigFactoryInterface $config_factory, EventDispatcherInterface $event_dispatcher = NULL) { $this->logger = $logger; $this->fileSystem = $file_system; $this->mimeTypeGuesser = $mime_type_guesser; $this->token = $token; $this->lock = $lock; $this->systemFileConfig = $config_factory->get('system.file'); + if (!$event_dispatcher) { + $event_dispatcher = \Drupal::service('event_dispatcher'); + } + $this->eventDispatcher = $event_dispatcher; } /** @@ -386,46 +400,14 @@ protected function validate(FileInterface $file, array $validators) { * The prepared/munged filename. */ protected function prepareFilename($filename, array &$validators) { - // Don't rename if 'allow_insecure_uploads' evaluates to TRUE. - if (!$this->systemFileConfig->get('allow_insecure_uploads')) { - if (!empty($validators['file_validate_extensions'][0])) { - // If there is a file_validate_extensions validator and a list of - // valid extensions, munge the filename to protect against possible - // malicious extension hiding within an unknown file type. For example, - // "filename.html.foo". - $filename = file_munge_filename($filename, $validators['file_validate_extensions'][0]); - } - - // Rename potentially executable files, to help prevent exploits (i.e. - // will rename filename.php.foo and filename.php to filename._php._foo.txt - // and filename._php.txt, respectively). - if (preg_match(FILE_INSECURE_EXTENSION_REGEX, $filename)) { - // If the file will be rejected anyway due to a disallowed extension, it - // should not be renamed; rather, we'll let file_validate_extensions() - // reject it below. - $passes_validation = FALSE; - if (!empty($validators['file_validate_extensions'][0])) { - $file = File::create([]); - $file->setFilename($filename); - $passes_validation = empty(file_validate_extensions($file, $validators['file_validate_extensions'][0])); - } - if (empty($validators['file_validate_extensions'][0]) || $passes_validation) { - if (substr($filename, -4) != '.txt') { - // The destination filename will also later be used to create the URI. - $filename .= '.txt'; - } - $filename = file_munge_filename($filename, $validators['file_validate_extensions'][0] ?? ''); - - // The .txt extension may not be in the allowed list of extensions. We - // have to add it here or else the file upload will fail. - if (!empty($validators['file_validate_extensions'][0])) { - $validators['file_validate_extensions'][0] .= ' txt'; - } - } - } + $extensions = $validators['file_validate_extensions'][0] ?? ''; + $event = new FileUploadSanitizeNameEvent($filename, $extensions); + $this->eventDispatcher->dispatch($event, FileEvents::SANITIZE_NAME); + if (isset($validators['file_validate_extensions'][0])) { + // @see \Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber::sanitizeName() + $validators['file_validate_extensions'][0] = $event->getAllowedExtensions(); } - - return $filename; + return $event->getFilename(); } /** diff --git a/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php index b63c030c6e..85828ab267 100644 --- a/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php +++ b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php @@ -680,13 +680,13 @@ public function testFileUploadMaliciousExtension() { $this->field->setSetting('file_extensions', '')->save(); $this->rebuildAll(); $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_5.php.png"']); - $expected = $this->getExpectedDocument(5, 'example_5.php_.png_.txt', TRUE); + $expected = $this->getExpectedDocument(5, 'example_5.php_.png', TRUE); // Override the expected filesize. $expected['data']['attributes']['filesize'] = strlen($php_string); // The file mime should also now be text. - $expected['data']['attributes']['filemime'] = 'text/plain'; + $expected['data']['attributes']['filemime'] = 'image/png'; $this->assertResponseData($expected, $response); - $this->assertFileExists('public://foobar/example_5.php_.png_.txt'); + $this->assertFileExists('public://foobar/example_5.php_.png'); // Dangerous extensions are munged if is renamed to end in .txt. $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_6.cgi.png.txt"']); diff --git a/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php index a6355c8275..69083cde9c 100644 --- a/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php @@ -576,13 +576,13 @@ public function testFileUploadMaliciousExtension() { $this->field->setSetting('file_extensions', '')->save(); $this->rebuildAll(); $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_5.php.png"']); - $expected = $this->getExpectedNormalizedEntity(5, 'example_5.php_.png_.txt', TRUE); + $expected = $this->getExpectedNormalizedEntity(5, 'example_5.php_.png', TRUE); // Override the expected filesize. $expected['filesize'][0]['value'] = strlen($php_string); // The file mime should also now be text. - $expected['filemime'][0]['value'] = 'text/plain'; + $expected['filemime'][0]['value'] = 'image/png'; $this->assertResponseData($expected, $response); - $this->assertFileExists('public://foobar/example_5.php_.png_.txt'); + $this->assertFileExists('public://foobar/example_5.php_.png'); // Dangerous extensions are munged if is renamed to end in .txt. $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_6.cgi.png.txt"']); diff --git a/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php b/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php new file mode 100644 index 0000000000..6d308ba8a6 --- /dev/null +++ b/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php @@ -0,0 +1,156 @@ +config = $config_factory->get('system.file'); + } + + /** + * {@inheritdoc} + */ + public static function getSubscribedEvents() { + // This event must be run last to ensure the filename obeys the security + // rules. + $events[FileEvents::SANITIZE_NAME][] = ['sanitizeName', -1024]; + return $events; + } + + /** + * Sanitizes the upload's filename to make it secure. + * + * @param \Drupal\Core\File\Event\FileUploadSanitizeNameEvent $event + * File upload sanitize name event. + */ + public function sanitizeName(FileUploadSanitizeNameEvent $event): void { + $filename = $event->getFilename(); + // Dot files are renamed regardless of security settings. + $filename = trim($filename, '.'); + + $extensions = $event->getAllowedExtensions(); + if ($extensions === '') { + // An empty extension list means that all file extensions are permitted. + $extensions = []; + } + elseif (!static::validateExtension($filename, $extensions)) { + // This upload will be rejected file_validate_extension() anyway so do not + // make any alterations to the filename. + return; + } + else { + // Convert extensions into an array to make it easier to work with. + $extensions = array_unique(explode(' ', $extensions)); + } + + // Remove any null bytes. See + // http://php.net/manual/security.filesystem.nullbytes.php + $filename = str_replace(chr(0), '', $filename); + + // 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. + $final_extension = (string) array_pop($filename_parts); + + // Add .txt to potentially executable files prior to munging to help prevent + // exploits. This results in a filenames like filename.php being changed to + // filename.php.txt prior to munging. + if (!$this->config->get('allow_insecure_uploads') && in_array(strtolower($final_extension), static::INSECURE_EXTENSIONS, TRUE)) { + $filename_parts[] = $final_extension; + $final_extension = 'txt'; + $event->addAllowedExtension('txt'); + } + + // If there are any insecure extensions in the filename munge all the + // internal extensions. + $munge_everything = !empty(array_intersect(array_map('strtolower', $filename_parts), self::INSECURE_EXTENSIONS)); + + // Munge the filename to protect against possible malicious extension hiding + // within an unknown file type (i.e. filename.html.foo). This was introduced + // as part of SA-2006-006 to fix Apache's risky fallback behaviour. + + // Loop through the middle parts of the name and add an underscore to the + // end of each section that could be a file extension but isn't in the + // list of allowed extensions. + foreach ($filename_parts as $filename_part) { + $filename .= '.' . $filename_part; + if ($munge_everything) { + $filename .= '_'; + } + elseif (!empty($extensions) && !in_array(strtolower($filename_part), $extensions) && preg_match("/^[a-zA-Z]{2,5}\d?$/", $filename_part)) { + $filename .= '_'; + } + } + if ($final_extension !== '') { + $filename .= '.' . $final_extension; + } + if ($filename !== $event->getFilename()) { + $event->setFilename($filename)->setSecurityRename(); + } + } + + /** + * Checks that the filename ends with an allowed extension. + * + * @param $filename + * The filename to check. + * @param string $extension_list + * A list of allowed file extensions separated by ' '. + * + * @return bool + * TRUE if the file's extension is in the extension list. FALSE, if not. + * + * @internal + * This method only checks that the filename ends with an extension that is + * in the provided list. It makes no claims as to the security of the + * filename. + * + * @see file_validate_extensions() + */ + public static function validateExtension(string $filename, string $extension_list): bool { + $regex = '/\.(' . preg_replace('/ +/', '|', preg_quote($extension_list)) . ')$/i'; + return (bool) preg_match($regex, $filename); + } + +} diff --git a/core/modules/system/system.services.yml b/core/modules/system/system.services.yml index 9db3db1a5a..c0e4d605ea 100644 --- a/core/modules/system/system.services.yml +++ b/core/modules/system/system.services.yml @@ -48,3 +48,8 @@ services: arguments: ['@current_user', '@config.factory'] tags: - { name: event_subscriber } + system.file_event.subscriber: + class: Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber + arguments: ['@config.factory'] + tags: + - { name: event_subscriber } diff --git a/core/modules/system/tests/src/Unit/Event/SecurityFileUploadEventSubscriberTest.php b/core/modules/system/tests/src/Unit/Event/SecurityFileUploadEventSubscriberTest.php new file mode 100644 index 0000000000..0af8621b16 --- /dev/null +++ b/core/modules/system/tests/src/Unit/Event/SecurityFileUploadEventSubscriberTest.php @@ -0,0 +1,184 @@ +getConfigFactoryStub([ + 'system.file' => [ + 'allow_insecure_uploads' => FALSE, + ], + ]); + + $subscriber = new SecurityFileUploadEventSubscriber($config_factory); + $event = new FileUploadSanitizeNameEvent($filename, $allowed_extensions); + $subscriber->sanitizeName($event); + + // Check the results of the configured sanitization. + $this->assertSame($expected_filename, $event->getFilename()); + $this->assertSame($expected_filename !== $filename, $event->isSecurityRename()); + + // Rerun the event allowing insecure uploads. + $config_factory = $this->getConfigFactoryStub([ + 'system.file' => [ + 'allow_insecure_uploads' => TRUE, + ], + ]); + + $subscriber = new SecurityFileUploadEventSubscriber($config_factory); + $event = new FileUploadSanitizeNameEvent($filename, $allowed_extensions); + $subscriber->sanitizeName($event); + + // Check the results of the configured sanitization. + $expected_filename_with_insecure_uploads = $expected_filename_with_insecure_uploads ?? $expected_filename; + $this->assertSame($expected_filename_with_insecure_uploads, $event->getFilename()); + $this->assertSame($expected_filename_with_insecure_uploads !== $filename, $event->isSecurityRename()); + } + + /** + * Provides data for testSanitizeName(). + * + * @return array + * Arrays with original name, allowed extensions, expected name and + * (optional) expected name 'allow_insecure_uploads' is set to TRUE. + */ + public function provideFilenames() { + return [ + 'All extensions allowed filename not munged' => ['foo.txt', '', 'foo.txt'], + 'All extensions allowed with .php file' => ['foo.php', '', 'foo.php_.txt', 'foo.php'], + 'All extensions allowed with .pHp file' => ['foo.pHp', '', 'foo.pHp_.txt', 'foo.pHp'], + 'All extensions allowed with .PHP file' => ['foo.PHP', '', 'foo.PHP_.txt', 'foo.PHP'], + '.php extension allowed with .php file' => ['foo.php', 'php', 'foo.php_.txt', 'foo.php'], + '.PhP extension allowed with .php file' => ['foo.php', 'PhP', 'foo.php_.txt', 'foo.php'], + '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'], + 'dot files are renamed' => ['.htaccess', '', 'htaccess'], + ]; + } + + /** + * Tests file name sanitization. + * + * @param string $filename + * The original filename. + * @param string $allowed_extensions + * The allowed extensions. + * + * @dataProvider provideFilenamesNoMunge + * + * @covers ::sanitizeName + */ + public function testSanitizeNameNoMunge(string $filename, string $allowed_extensions) { + $config_factory = $this->getConfigFactoryStub([ + 'system.file' => [ + 'allow_insecure_uploads' => FALSE, + ], + ]); + + $subscriber = new SecurityFileUploadEventSubscriber($config_factory); + $event = new FileUploadSanitizeNameEvent($filename, $allowed_extensions); + $subscriber->sanitizeName($event); + + // Check the results of the configured sanitization. + $this->assertSame($filename, $event->getFilename()); + $this->assertSame(FALSE, $event->isSecurityRename()); + + $config_factory = $this->getConfigFactoryStub([ + 'system.file' => [ + 'allow_insecure_uploads' => TRUE, + ], + ]); + + $event = new FileUploadSanitizeNameEvent($filename, $allowed_extensions); + $subscriber = new SecurityFileUploadEventSubscriber($config_factory); + $subscriber->sanitizeName($event); + + // Check the results of the configured sanitization. + $this->assertSame($filename, $event->getFilename()); + $this->assertSame(FALSE, $event->isSecurityRename()); + + // Ensure that they would be rejected by file_validate_extensions(). + $this->assertFalse(SecurityFileUploadEventSubscriber::validateExtension($filename, $allowed_extensions)); + } + + /** + * Provides data for testSanitizeNameNoMunge(). + * + * @return array + * Arrays with original name and allowed extensions. + */ + public function provideFilenamesNoMunge() { + return [ + // The following filename would be rejected by file_validate_extension() + // 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'], + ]; + } + + /** + * @covers ::validateExtension + * @dataProvider provideValidateExtension + * + * @param bool $expected + * The expected return value. + * @param string $filename + * The filename. + * @param string $extension_list + * The list of valid extensions. + */ + public function testValidateExtension(bool $expected, string $filename, string $extension_list) { + $this->assertSame($expected, SecurityFileUploadEventSubscriber::validateExtension($filename, $extension_list)); + } + + /** + * Data provider for testValidateExtension() + * @return array[] + */ + public function provideValidateExtension() { + return [ + [TRUE, 'file.jpg', 'jpg'], + [FALSE, 'file.png', 'jpg'], + [FALSE, 'file.png', 'jpg gif'], + [TRUE, 'file.png', 'jpg gif png'], + [TRUE, 'file.tar.gz', 'gz tar'], + [TRUE, 'file.tar.gz', 'tar.gz'], + [FALSE, 'file.tar.gz', 'gz.tar'], + [FALSE, 'file', 'jpg'], + [FALSE, '.htaccess', 'jpg'], + ]; + } + +} diff --git a/core/tests/Drupal/KernelTests/Core/File/NameMungingTest.php b/core/tests/Drupal/KernelTests/Core/File/NameMungingTest.php index 26f54f97fc..0941737fb6 100644 --- a/core/tests/Drupal/KernelTests/Core/File/NameMungingTest.php +++ b/core/tests/Drupal/KernelTests/Core/File/NameMungingTest.php @@ -25,6 +25,7 @@ * - (name).foo.txt -> (unchecked) -> (name).foo.txt after un-munging * * @group File + * @group legacy */ class NameMungingTest extends FileTestBase { @@ -60,6 +61,7 @@ protected function setUp(): void { * Create a file and munge/unmunge the name. */ public function testMunging() { + $this->expectDeprecation('file_munge_filename() is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Dispatch a \Drupal\Core\File\Event\FileEvents::SANITIZE_NAME event instead. See https://www.drupal.org/node/3032541'); // Disable insecure uploads. $this->config('system.file')->set('allow_insecure_uploads', 0)->save(); $munged_name = file_munge_filename($this->name, '', TRUE); @@ -117,6 +119,7 @@ public function testMungeUnsafe() { * Ensure that unmunge gets your name back. */ public function testUnMunge() { + $this->expectDeprecation('file_unmunge_filename() is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use str_replace() instead. See https://www.drupal.org/node/3032541'); $munged_name = file_munge_filename($this->name, '', FALSE); $unmunged_name = file_unmunge_filename($munged_name); $this->assertSame($unmunged_name, $this->name, new FormattableMarkup('The unmunged (%unmunged) filename matches the original (%original)', ['%unmunged' => $unmunged_name, '%original' => $this->name])); diff --git a/core/tests/Drupal/Tests/Core/File/FileUploadSanitizeNameEventTest.php b/core/tests/Drupal/Tests/Core/File/FileUploadSanitizeNameEventTest.php new file mode 100644 index 0000000000..c22f083fd9 --- /dev/null +++ b/core/tests/Drupal/Tests/Core/File/FileUploadSanitizeNameEventTest.php @@ -0,0 +1,109 @@ +assertSame('foo.txt', $event->getFilename()); + $event->setFilename('foo.html'); + $this->assertSame('foo.html', $event->getFilename()); + } + + /** + * @covers ::setFilename + */ + public function testSetFilenameException() { + $event = new FileUploadSanitizeNameEvent('foo.txt', ''); + $this->assertSame('foo.txt', $event->getFilename()); + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('$filename must be a filename with no path information, "bar/foo.html" provided'); + $event->setFilename('bar/foo.html'); + } + + /** + * @covers ::__construct + * @covers ::setFilename + */ + public function testConstructorException() { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('$filename must be a filename with no path information, "bar/foo.txt" provided'); + new FileUploadSanitizeNameEvent('bar/foo.txt', ''); + } + + /** + * @covers ::getAllowedExtensions + * @covers ::addAllowedExtension + */ + public function testAllowedExtensions() { + $event = new FileUploadSanitizeNameEvent('foo.txt', ''); + $this->assertSame('', $event->getAllowedExtensions()); + // Blank allows all extensions but adding to the allowed extensions limits + // it. + $this->assertSame('txt', $event->addAllowedExtension('txt')->getAllowedExtensions()); + + // Set an allowed extension during construction. + $event = new FileUploadSanitizeNameEvent('foo.txt', 'txt'); + $this->assertSame('txt', $event->getAllowedExtensions()); + $this->assertSame('txt php', $event->addAllowedExtension('php')->getAllowedExtensions()); + } + + /** + * Test event construction. + * + * @dataProvider provideFilenames + * @covers ::__construct + * @covers ::getFilename + * + * @param string $filename + * The filename to test + */ + public function testEventFilenameFunctions(string $filename) { + $event = new FileUploadSanitizeNameEvent($filename, ''); + $this->assertSame($filename, $event->getFilename()); + } + + /** + * Provides data for testEvent(). + * + * @return array + * Arrays with original name, expected name and allowed extensions (as a + * space-delimited string). + */ + public function provideFilenames() { + return [ + 'ASCII filename with extension' => [ + 'example.txt', + ], + 'ASCII filename with complex extension' => [ + 'example.html.twig', + ], + 'ASCII filename with lots of dots' => [ + 'dotty.....txt', + ], + 'Unicode filename with extension' => [ + 'Ä Ö Ü Å Ø äöüåøhello.txt', + ], + 'Unicode filename without extension' => [ + 'Ä Ö Ü Å Ø äöüåøhello', + ], + ]; + } + +}