From fd1d6759f28303360f1797ce8c8fc4e0bdf2468b Mon Sep 17 00:00:00 2001 From: Lee Rowlands <lee.rowlands@previousnext.com.au> Date: Sat, 23 Jan 2021 09:16:07 +1000 Subject: [PATCH] Issue #2863652 by alexpott, anmolgoyal74, javi.pl, dww, zaporylie, moinak_dutta, iampuma, a.dmitriiev, benjifisher, Pancho, larowlan, yoroy: Do not allow files with "php|pl|py|cgi|asp|js" extensions to be renamed to *.txt and be uploaded if *.txt is not allowed by the field widget --- core/modules/file/file.module | 16 ++- core/modules/file/file.post_update.php | 41 +++++++ .../src/Plugin/Field/FieldType/FileItem.php | 15 ++- .../rest/resource/FileUploadResource.php | 18 +-- .../src/Functional/FileFieldWidgetTest.php | 33 ++++++ .../src/Functional/SaveUploadFormTest.php | 20 +++- .../tests/src/Functional/SaveUploadTest.php | 17 ++- .../FileFieldFileExtensionsUpdateTest.php | 112 ++++++++++++++++++ .../TemporaryJsonapiFileFieldUploader.php | 18 +-- .../tests/src/Functional/FileUploadTest.php | 20 +++- .../Functional/FileUploadResourceTestBase.php | 20 +++- 11 files changed, 291 insertions(+), 39 deletions(-) create mode 100644 core/modules/file/file.post_update.php create mode 100644 core/modules/file/tests/src/Functional/Update/FileFieldFileExtensionsUpdateTest.php diff --git a/core/modules/file/file.module b/core/modules/file/file.module index 0e5c0b31868d..925facef01b9 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 000000000000..707bb4e6b2e6 --- /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 41b30e9216cc..1f77571c0312 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 3c7180a0b03b..399d55f2a783 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 9d8d41e3c973..97d5db0a91fb 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 e3abbdf73c98..8ff605cf5198 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 6abe0ef9c990..6d15245da4cd 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 000000000000..66f0446dbc37 --- /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 a887aa7c96b5..fd60f11f374c 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 b63c030c6e52..39299b4a54a1 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 a6355c827500..e66259854313 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') -- GitLab