diff --git a/core/modules/file/file.module b/core/modules/file/file.module index 0e5c0b31868d37e3dc8ee02e8f2a6bd9b5d5755b..925facef01b9c9bf8fced308bddfea803f39f266 100644 --- a/core/modules/file/file.module +++ b/core/modules/file/file.module @@ -25,6 +25,8 @@ use Drupal\Core\Template\Attribute; use Symfony\Component\Mime\MimeTypeGuesserInterface; +// cspell:ignore btxt + /** * The regex pattern used when checking for insecure file types. */ @@ -1000,10 +1002,11 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va // 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))) { + // If there is no file extension validation at all, or .txt is considered + // a valid extension and the file would otherwise pass validation, rename + // it. If the file will be rejected due to extension validation, it should + // not be renamed; rather, let file_validate_extensions() reject it below. + if (!isset($validators['file_validate_extensions']) || (preg_match('/\btxt\b/', $extensions) && empty(file_validate_extensions($file, $extensions)))) { $file->setMimeType('text/plain'); $filename = $file->getFilename(); if (substr($filename, -4) != '.txt') { @@ -1012,11 +1015,6 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va } $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'; - } } } } diff --git a/core/modules/file/file.post_update.php b/core/modules/file/file.post_update.php new file mode 100644 index 0000000000000000000000000000000000000000..707bb4e6b2e6d94427d5aed948fada434592aa88 --- /dev/null +++ b/core/modules/file/file.post_update.php @@ -0,0 +1,41 @@ +<?php + +/** + * @file + * Post update functions for File. + */ + +use Drupal\Core\Config\Entity\ConfigEntityUpdater; +use Drupal\field\Entity\FieldConfig; +use Drupal\file\Plugin\Field\FieldType\FileItem; + +/** + * Add txt to allowed extensions for all fields that allow uploads of insecure files. + */ +function file_post_update_add_txt_if_allows_insecure_extensions(&$sandbox = NULL) { + if (\Drupal::config('system.file')->get('allow_insecure_uploads')) { + return t('The system is configured to allow insecure file uploads. No file field updates are necessary.'); + } + + $updater = function (FieldConfig $field) { + // Determine if this field uses an item definition that extends FileItem. + if (is_subclass_of($field->getItemDefinition()->getClass(), FileItem::class)) { + $allowed_extensions_string = trim($field->getSetting('file_extensions')); + $allowed_extensions = array_filter(explode(' ', $allowed_extensions_string)); + if (in_array('txt', $allowed_extensions, TRUE)) { + // Since .txt is specifically allowed, there's nothing to do. + return FALSE; + } + foreach ($allowed_extensions as $extension) { + // Allow .txt if an insecure extension is allowed. + if (preg_match(FILE_INSECURE_EXTENSION_REGEX, 'test.' . $extension)) { + $allowed_extensions_string .= ' txt'; + $field->setSetting('file_extensions', $allowed_extensions_string); + return TRUE; + } + } + return FALSE; + } + }; + \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'field_config', $updater); +} diff --git a/core/modules/file/src/Plugin/Field/FieldType/FileItem.php b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php index 41b30e9216cc9571a3564160354d5635be27242c..1f77571c0312cc098acea752e34455a46e56c803 100644 --- a/core/modules/file/src/Plugin/Field/FieldType/FileItem.php +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php @@ -225,14 +225,25 @@ public static function validateDirectory($element, FormStateInterface $form_stat public static function validateExtensions($element, FormStateInterface $form_state) { if (!empty($element['#value'])) { $extensions = preg_replace('/([, ]+\.?)/', ' ', trim(strtolower($element['#value']))); - $extensions = array_filter(explode(' ', $extensions)); - $extensions = implode(' ', array_unique($extensions)); + $extension_array = array_unique(array_filter(explode(' ', $extensions))); + $extensions = implode(' ', $extension_array); if (!preg_match('/^([a-z0-9]+([.][a-z0-9])* ?)+$/', $extensions)) { $form_state->setError($element, t('The list of allowed extensions is not valid, be sure to exclude leading dots and to separate extensions with a comma or space.')); } else { $form_state->setValueForElement($element, $extensions); } + + // If insecure uploads are not allowed and txt is not in the list of + // allowed extensions, ensure that no insecure extensions are allowed. + if (!in_array('txt', $extension_array, TRUE) && !\Drupal::config('system.file')->get('allow_insecure_uploads')) { + foreach ($extension_array as $extension) { + if (preg_match(FILE_INSECURE_EXTENSION_REGEX, 'test.' . $extension)) { + $form_state->setError($element, t('Add %txt_extension to the list of allowed extensions to securely upload files with a %extension extension. The %txt_extension extension will then be added automatically.', ['%extension' => $extension, '%txt_extension' => 'txt'])); + break; + } + } + } } } diff --git a/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php index 3c7180a0b03bb7a48fefa094959d9967b86bd443..399d55f2a78316cea7b652b7a291677a2d0718ad 100644 --- a/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php @@ -32,6 +32,8 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Exception\HttpException; +// cspell:ignore btxt + /** * File upload resource. * @@ -485,24 +487,24 @@ protected function prepareFilename($filename, array &$validators) { // 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])); + // Only allow upload and rename to .txt if .txt files are allowed. + $passes_validation = $passes_validation && preg_match('/\btxt\b/', $validators['file_validate_extensions'][0]); + } + else { + // All file extensions are allowed. + $passes_validation = TRUE; } - if (empty($validators['file_validate_extensions'][0]) || $passes_validation) { + + if ($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'; - } } } } diff --git a/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php index 9d8d41e3c973bc78b6b2a0e2ad09b59d1789b997..97d5db0a91fb0ca37e1e1b5a060d3b866b25b54f 100644 --- a/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php +++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php @@ -470,6 +470,39 @@ public function testTemporaryFileRemovalExploitAnonymous() { $this->doTestTemporaryFileRemovalExploit($victim_user, $attacker_user); } + /** + * Tests configuring file field's allowed file extensions setting. + */ + public function testFileExtensionsSetting() { + // Grant the admin user required permissions. + user_role_grant_permissions($this->adminUser->roles[0]->target_id, ['administer node fields']); + + $type_name = 'article'; + $field_name = strtolower($this->randomMachineName()); + $this->createFileField($field_name, 'node', $type_name); + $field = FieldConfig::loadByName('node', $type_name, $field_name); + $field_id = $field->id(); + + // By default allowing .php files without .txt is not permitted. + $this->drupalGet("admin/structure/types/manage/$type_name/fields/$field_id"); + $edit = ['settings[file_extensions]' => 'jpg php']; + $this->submitForm($edit, 'Save settings'); + $this->assertSession()->pageTextContains('Add txt to the list of allowed extensions to securely upload files with a php extension. The txt extension will then be added automatically.'); + + // Test allowing .php and .txt. + $edit = ['settings[file_extensions]' => 'jpg php txt']; + $this->submitForm($edit, 'Save settings'); + $this->assertSession()->pageTextContains('Saved ' . $field_name . ' configuration.'); + + // If the system is configured to allow insecure uploads, .txt is not + // required when allowing .php. + $this->config('system.file')->set('allow_insecure_uploads', TRUE)->save(); + $this->drupalGet("admin/structure/types/manage/$type_name/fields/$field_id"); + $edit = ['settings[file_extensions]' => 'jpg php']; + $this->submitForm($edit, 'Save settings'); + $this->assertSession()->pageTextContains('Saved ' . $field_name . ' configuration.'); + } + /** * Helper for testing exploiting the temporary file removal using fid. * diff --git a/core/modules/file/tests/src/Functional/SaveUploadFormTest.php b/core/modules/file/tests/src/Functional/SaveUploadFormTest.php index e3abbdf73c98d3826bae18e754eda8a280bcd886..8ff605cf5198e30fe7c9594623443334fa874c35 100644 --- a/core/modules/file/tests/src/Functional/SaveUploadFormTest.php +++ b/core/modules/file/tests/src/Functional/SaveUploadFormTest.php @@ -222,7 +222,7 @@ public function testHandleDangerousFile() { 'file_test_replace' => FileSystemInterface::EXISTS_REPLACE, 'files[file_test_upload][]' => $file_system->realpath($this->phpfile->uri), 'is_image_file' => FALSE, - 'extensions' => 'php', + 'extensions' => 'php txt', ]; $this->drupalPostForm('file-test/save_upload_from_form_test', $edit, 'Submit'); @@ -251,6 +251,24 @@ public function testHandleDangerousFile() { // Turn off insecure uploads. $config->set('allow_insecure_uploads', 0)->save(); + + // Reset the hook counters. + file_test_reset(); + + $edit = [ + 'file_test_replace' => FileSystemInterface::EXISTS_REPLACE, + 'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->phpfile->uri), + 'is_image_file' => FALSE, + 'extensions' => 'php', + ]; + + $this->drupalPostForm('file-test/upload', $edit, 'Submit'); + $this->assertSession()->statusCodeEquals(200); + $this->assertSession()->pageTextContains('For security reasons, your upload has been rejected.'); + $this->assertSession()->pageTextContains('Epic upload FAIL!'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(['validate']); } /** diff --git a/core/modules/file/tests/src/Functional/SaveUploadTest.php b/core/modules/file/tests/src/Functional/SaveUploadTest.php index 6abe0ef9c990b0f220a78141dd638bdef745bc1e..6d15245da4cd0f0e05feee0adc686b49b760ed46 100644 --- a/core/modules/file/tests/src/Functional/SaveUploadTest.php +++ b/core/modules/file/tests/src/Functional/SaveUploadTest.php @@ -261,7 +261,7 @@ public function testHandleDangerousFile() { 'file_test_replace' => FileSystemInterface::EXISTS_REPLACE, 'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->phpfile->uri), 'is_image_file' => FALSE, - 'extensions' => 'php', + 'extensions' => 'php txt', ]; $this->drupalPostForm('file-test/upload', $edit, 'Submit'); @@ -320,6 +320,21 @@ public function testHandleDangerousFile() { // Reset the hook counters. file_test_reset(); + + $edit = [ + 'file_test_replace' => FileSystemInterface::EXISTS_REPLACE, + 'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->phpfile->uri), + 'is_image_file' => FALSE, + 'extensions' => 'php', + ]; + + $this->drupalPostForm('file-test/upload', $edit, 'Submit'); + $this->assertSession()->statusCodeEquals(200); + $this->assertSession()->pageTextContains('For security reasons, your upload has been rejected.'); + $this->assertSession()->pageTextContains('Epic upload FAIL!'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(['validate']); } /** diff --git a/core/modules/file/tests/src/Functional/Update/FileFieldFileExtensionsUpdateTest.php b/core/modules/file/tests/src/Functional/Update/FileFieldFileExtensionsUpdateTest.php new file mode 100644 index 0000000000000000000000000000000000000000..66f0446dbc37766befb3f29a4b5684fbc678aed7 --- /dev/null +++ b/core/modules/file/tests/src/Functional/Update/FileFieldFileExtensionsUpdateTest.php @@ -0,0 +1,112 @@ +<?php + +namespace Drupal\Tests\file\Functional\Update; + +use Drupal\Core\Database\Database; +use Drupal\field\Entity\FieldConfig; +use Drupal\FunctionalTests\Update\UpdatePathTestBase; + +/** + * Tests file_post_update_add_txt_if_allows_insecure_extensions(). + * + * @group Update + * @group legacy + */ +class FileFieldFileExtensionsUpdateTest extends UpdatePathTestBase { + + /** + * {@inheritdoc} + */ + protected static $modules = ['file']; + + /** + * {@inheritdoc} + */ + protected function setDatabaseDumpFiles() { + $this->databaseDumpFiles = [ + __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.8.0.bare.standard.php.gz', + ]; + } + + /** + * Tests adding txt extension to field that allow insecure extensions. + */ + public function testInsecureUpdatesNotAllowed() { + $this->setAllowedExtensions('php jpg'); + $this->runUpdates(); + $this->assertSession()->statusCodeEquals('200'); + $field = FieldConfig::load('node.article.field_image'); + $this->assertSame('php jpg txt', $field->getSetting('file_extensions')); + } + + /** + * Tests file fields that permit all extensions. + */ + public function testAllFileTypesAllowed() { + $this->setAllowedExtensions(''); + $this->runUpdates(); + $this->assertSession()->statusCodeEquals('200'); + $field = FieldConfig::load('node.article.field_image'); + $this->assertSame('', $field->getSetting('file_extensions')); + } + + /** + * Tests update when insecure uploads are allowed. + */ + public function testInsecureUpdatesAllowed() { + $this->setAllowedExtensions('php'); + + // Do direct database updates to avoid dependencies. + $connection = Database::getConnection(); + $config = $connection->select('config') + ->fields('config', ['data']) + ->condition('collection', '') + ->condition('name', 'system.file') + ->execute() + ->fetchField(); + $config = unserialize($config); + $config['allow_insecure_uploads'] = TRUE; + $connection->update('config') + ->fields([ + 'data' => serialize($config), + ]) + ->condition('collection', '') + ->condition('name', 'system.file') + ->execute(); + + $this->runUpdates(); + $this->assertSession()->pageTextContains('The system is configured to allow insecure file uploads. No file field updates are necessary.'); + $this->assertSession()->statusCodeEquals('200'); + $field = FieldConfig::load('node.article.field_image'); + $this->assertSame('php', $field->getSetting('file_extensions')); + } + + /** + * Sets the allowed extensions on the article image field. + * + * @param string $allowed_extensions + * The list of allowed extensions. + */ + protected function setAllowedExtensions(string $allowed_extensions) { + // Do direct database updates to avoid dependencies. + $connection = Database::getConnection(); + + $config = $connection->select('config') + ->fields('config', ['data']) + ->condition('collection', '') + ->condition('name', 'field.field.node.article.field_image') + ->execute() + ->fetchField(); + $config = unserialize($config); + $this->assertArrayHasKey('file_extensions', $config['settings']); + $config['settings']['file_extensions'] = $allowed_extensions; + $connection->update('config') + ->fields([ + 'data' => serialize($config), + ]) + ->condition('collection', '') + ->condition('name', 'field.field.node.article.field_image') + ->execute(); + } + +} diff --git a/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php index a887aa7c96b5d4f58be65bb8ad4f7c3bee50cec7..fd60f11f374c5326a7715780a267eb1e3e7b5a1d 100644 --- a/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php @@ -27,6 +27,8 @@ use Symfony\Component\Mime\MimeTypeGuesserInterface; use Symfony\Component\Validator\ConstraintViolation; +// cspell:ignore btxt + /** * Reads data from an upload stream and creates a corresponding file entity. * @@ -403,24 +405,24 @@ protected function prepareFilename($filename, array &$validators) { // 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])); + // Only allow upload and rename to .txt if .txt files are allowed. + $passes_validation = $passes_validation && preg_match('/\btxt\b/', $validators['file_validate_extensions'][0]); + } + else { + // All file extensions are allowed. + $passes_validation = TRUE; } - if (empty($validators['file_validate_extensions'][0]) || $passes_validation) { + + if ($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'; - } } } } diff --git a/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php index b63c030c6e5284ec64b5e777ebc93c301c74d6ab..39299b4a54a12f690e6e35d75cfd06cba584be7a 100644 --- a/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php +++ b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php @@ -627,11 +627,9 @@ public function testFileUploadMaliciousExtension() { $this->assertResponseData($expected, $response); $this->assertFileExists('public://foobar/example.php_.txt'); - // Add php as an allowed format. Allow insecure uploads still being FALSE - // should still not allow this. So it should still have a .txt extension - // appended even though it is not in the list of allowed extensions. - $this->field->setSetting('file_extensions', 'php') - ->save(); + // Add .php and .txt as allowed extensions. Since 'allow_insecure_uploads' + // is FALSE, .php files should be renamed to have a .txt extension. + $this->field->setSetting('file_extensions', 'php txt')->save(); $this->rebuildAll(); $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_2.php"']); @@ -698,6 +696,18 @@ public function testFileUploadMaliciousExtension() { $this->assertResponseData($expected, $response); $this->assertFileExists('public://foobar/example_6.cgi_.png_.txt'); + // Add .php as an allowed extension without .txt. Since insecure uploads are + // are not allowed, .php files will be rejected. + $this->field->setSetting('file_extensions', 'php')->save(); + $this->rebuildAll(); + + $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_7.php"']); + $this->assertResourceErrorResponse(422, "Unprocessable Entity: file validation failed.\nFor security reasons, your upload has been rejected.", $uri, $response); + + // Make sure that no file was saved. + $this->assertFileNotExists('public://foobar/example_7.php'); + $this->assertFileNotExists('public://foobar/example_7.php.txt'); + // Now allow insecure uploads. \Drupal::configFactory() ->getEditable('system.file') diff --git a/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php index a6355c827500b81494177144af6391bbbd97bbc4..e66259854313f6e2fca89eb7242a620667df4910 100644 --- a/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php @@ -523,11 +523,9 @@ public function testFileUploadMaliciousExtension() { $this->assertResponseData($expected, $response); $this->assertFileExists('public://foobar/example.php_.txt'); - // Add php as an allowed format. Allow insecure uploads still being FALSE - // should still not allow this. So it should still have a .txt extension - // appended even though it is not in the list of allowed extensions. - $this->field->setSetting('file_extensions', 'php') - ->save(); + // Add .php and .txt as allowed extensions. Since 'allow_insecure_uploads' + // is FALSE, .php files should be renamed to have a .txt extension. + $this->field->setSetting('file_extensions', 'php txt')->save(); $this->refreshTestStateAfterRestConfigChange(); $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_2.php"']); @@ -594,6 +592,18 @@ public function testFileUploadMaliciousExtension() { $this->assertResponseData($expected, $response); $this->assertFileExists('public://foobar/example_6.cgi_.png_.txt'); + // Add .php as an allowed extension without .txt. Since insecure uploads are + // not allowed, .php files will be rejected. + $this->field->setSetting('file_extensions', 'php')->save(); + $this->refreshTestStateAfterRestConfigChange(); + + $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_7.php"']); + $this->assertResourceErrorResponse(422, "Unprocessable Entity: file validation failed.\nFor security reasons, your upload has been rejected.", $response); + + // Make sure that no file was saved. + $this->assertFileNotExists('public://foobar/example_7.php'); + $this->assertFileNotExists('public://foobar/example_7.php.txt'); + // Now allow insecure uploads. \Drupal::configFactory() ->getEditable('system.file')