From 2733cbd700d7c1333261faccbb7d1f39f9e2ff24 Mon Sep 17 00:00:00 2001 From: Pantheon Automation Date: Wed, 18 Nov 2020 19:01:37 +0000 Subject: [PATCH] Update to Drupal 7.74. For more information, see https://www.drupal.org/project/drupal/releases/7.74 --- CHANGELOG.txt | 5 + includes/bootstrap.inc | 2 +- includes/file.inc | 70 +++++--- modules/simpletest/tests/file.test | 207 +++++++++++++++++++++- modules/simpletest/tests/file_test.module | 16 +- 5 files changed, 266 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 7f5f570a564..d5b8cb28c23 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -1,3 +1,8 @@ +Drupal 7.74, 2020-11-17 +----------------------- +- Fixed security issues: + - SA-CORE-2020-012 + Drupal 7.73, 2020-09-16 ----------------------- - Fixed security issues: diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc index 3056fbf2e41..2c7ac52d94c 100644 --- a/includes/bootstrap.inc +++ b/includes/bootstrap.inc @@ -8,7 +8,7 @@ /** * The current system version. */ -define('VERSION', '7.73'); +define('VERSION', '7.74'); /** * Core API compatibility. diff --git a/includes/file.inc b/includes/file.inc index 6c10b6e450d..2ea503df4fe 100644 --- a/includes/file.inc +++ b/includes/file.inc @@ -1151,8 +1151,8 @@ function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXIST * exploit.php_.pps. * * Specifically, this function adds an underscore to all extensions that are - * between 2 and 5 characters in length, internal to the file name, and not - * included in $extensions. + * between 2 and 5 characters in length, internal to the file name, and either + * included in the list of unsafe extensions, or not included in $extensions. * * Function behavior is also controlled by the Drupal variable * 'allow_insecure_uploads'. If 'allow_insecure_uploads' evaluates to TRUE, no @@ -1161,7 +1161,8 @@ function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXIST * @param $filename * File name to modify. * @param $extensions - * A space-separated list of extensions that should not be altered. + * A space-separated list of extensions that should not be altered. Note that + * extensions that are unsafe will be altered regardless of this parameter. * @param $alerts * If TRUE, drupal_set_message() will be called to display a message if the * file name was changed. @@ -1179,6 +1180,10 @@ function file_munge_filename($filename, $extensions, $alerts = TRUE) { $whitelist = array_unique(explode(' ', strtolower(trim($extensions)))); + // Remove unsafe extensions from the list of allowed extensions. The list is + // copied from file_save_upload(). + $whitelist = array_diff($whitelist, explode('|', 'php|phar|pl|py|cgi|asp|js')); + // Split the filename up by periods. The first part becomes the basename // the last part the final extension. $filename_parts = explode('.', $filename); @@ -1546,25 +1551,35 @@ function file_save_upload($form_field_name, $validators = array(), $destination $validators['file_validate_extensions'][0] = $extensions; } - if (!empty($extensions)) { - // Munge the filename to protect against possible malicious extension hiding - // within an unknown file type (ie: filename.html.foo). - $file->filename = file_munge_filename($file->filename, $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). Don't rename if 'allow_insecure_uploads' - // evaluates to TRUE. - if (!variable_get('allow_insecure_uploads', 0) && preg_match('/\.(php|phar|pl|py|cgi|asp|js)(\.|$)/i', $file->filename) && (substr($file->filename, -4) != '.txt')) { - $file->filemime = 'text/plain'; - // The destination filename will also later be used to create the URI. - $file->filename .= '.txt'; - // 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 (!variable_get('allow_insecure_uploads', 0)) { if (!empty($extensions)) { - $validators['file_validate_extensions'][0] .= ' txt'; - drupal_set_message(t('For security reasons, your upload has been renamed to %filename.', array('%filename' => $file->filename))); + // Munge the filename to protect against possible malicious extension hiding + // within an unknown file type (ie: filename.html.foo). + $file->filename = file_munge_filename($file->filename, $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). Don't rename if 'allow_insecure_uploads' + // evaluates to TRUE. + if (preg_match('/\.(php|phar|pl|py|cgi|asp|js)(\.|$)/i', $file->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. + if (!isset($validators['file_validate_extensions']) || !file_validate_extensions($file, $extensions)) { + $file->filemime = 'text/plain'; + if (substr($file->filename, -4) != '.txt') { + // The destination filename will also later be used to create the URI. + $file->filename .= '.txt'; + } + $file->filename = file_munge_filename($file->filename, $extensions, FALSE); + drupal_set_message(t('For security reasons, your upload has been renamed to %filename.', array('%filename' => $file->filename))); + // 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'; + } + } } } @@ -1732,7 +1747,18 @@ function file_validate(stdClass &$file, $validators = array()) { } // Let other modules perform validation on the new file. - return array_merge($errors, module_invoke_all('file_validate', $file)); + $errors = array_merge($errors, module_invoke_all('file_validate', $file)); + + // Ensure the file does not contain a malicious extension. At this point + // file_save_upload() will have munged the file so it does not contain a + // malicious extension. Contributed and custom code that calls this method + // needs to take similar steps if they need to permit files with malicious + // extensions to be uploaded. + if (empty($errors) && !variable_get('allow_insecure_uploads', 0) && preg_match('/\.(php|phar|pl|py|cgi|asp|js)(\.|$)/i', $file->filename)) { + $errors[] = t('For security reasons, your upload has been rejected.'); + } + + return $errors; } /** diff --git a/modules/simpletest/tests/file.test b/modules/simpletest/tests/file.test index 429a55180f6..57b315fb0ee 100644 --- a/modules/simpletest/tests/file.test +++ b/modules/simpletest/tests/file.test @@ -706,7 +706,7 @@ class FileSaveUploadTest extends FileHookTestCase { $edit = array( 'file_test_replace' => FILE_EXISTS_REPLACE, 'files[file_test_upload]' => drupal_realpath($this->image->uri), - 'allow_all_extensions' => TRUE, + 'allow_all_extensions' => 'empty_array', ); $this->drupalPost('file-test/upload', $edit, t('Submit')); $this->assertResponse(200, 'Received a 200 response for posted test file.'); @@ -715,14 +715,35 @@ class FileSaveUploadTest extends FileHookTestCase { // Check that the correct hooks were called. $this->assertFileHooksCalled(array('validate', 'load', 'update')); + + // Reset the hook counters. + file_test_reset(); + + // Now tell file_save_upload() to allow any extension and try and upload a + // malicious file. + $edit = array( + 'file_test_replace' => FILE_EXISTS_REPLACE, + 'files[file_test_upload]' => drupal_realpath($this->phpfile->uri), + 'is_image_file' => FALSE, + 'allow_all_extensions' => 'empty_array', + ); + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, 'Received a 200 response for posted test file.'); + $message = t('For security reasons, your upload has been renamed to') . ' ' . $this->phpfile->filename . '_.txt' . ''; + $this->assertRaw($message, 'Dangerous file was renamed.'); + $this->assertText('File name is php-2.php_.txt.'); + $this->assertRaw(t('File MIME type is text/plain.'), "Dangerous file's MIME type was changed."); + $this->assertRaw(t('You WIN!'), 'Found the success message.'); + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate', 'insert')); } /** * Test dangerous file handling. */ function testHandleDangerousFile() { - // Allow the .php extension and make sure it gets renamed to .txt for - // safety. Also check to make sure its MIME type was changed. + // 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 = array( 'file_test_replace' => FILE_EXISTS_REPLACE, 'files[file_test_upload]' => drupal_realpath($this->phpfile->uri), @@ -732,8 +753,9 @@ class FileSaveUploadTest extends FileHookTestCase { $this->drupalPost('file-test/upload', $edit, t('Submit')); $this->assertResponse(200, 'Received a 200 response for posted test file.'); - $message = t('For security reasons, your upload has been renamed to') . ' ' . $this->phpfile->filename . '.txt' . ''; + $message = t('For security reasons, your upload has been renamed to') . ' ' . $this->phpfile->filename . '_.txt' . ''; $this->assertRaw($message, 'Dangerous file was renamed.'); + $this->assertRaw('File name is php-2.php_.txt.'); $this->assertRaw(t('File MIME type is text/plain.'), "Dangerous file's MIME type was changed."); $this->assertRaw(t('You WIN!'), 'Found the success message.'); @@ -755,8 +777,39 @@ class FileSaveUploadTest extends FileHookTestCase { // Check that the correct hooks were called. $this->assertFileHooksCalled(array('validate', 'insert')); - // Turn off insecure uploads. + // Reset the hook counters. + file_test_reset(); + + // Even with insecure uploads allowed, the .php file should not be uploaded + // if it is not explicitly included in the list of allowed extensions. + $edit['extensions'] = 'foo'; + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, 'Received a 200 response for posted test file.'); + $message = t('Only files with the following extensions are allowed:') . ' ' . $edit['extensions'] . ''; + $this->assertRaw($message, 'Cannot upload a disallowed extension'); + $this->assertRaw(t('Epic upload FAIL!'), 'Found the failure message.'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate')); + + // Reset the hook counters. + file_test_reset(); + + // Turn off insecure uploads, then try the same thing as above (ensure that + // the .php file is still rejected since it's not in the list of allowed + // extensions). variable_set('allow_insecure_uploads', 0); + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, 'Received a 200 response for posted test file.'); + $message = t('Only files with the following extensions are allowed:') . ' ' . $edit['extensions'] . ''; + $this->assertRaw($message, 'Cannot upload a disallowed extension'); + $this->assertRaw(t('Epic upload FAIL!'), 'Found the failure message.'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate')); + + // Reset the hook counters. + file_test_reset(); } /** @@ -765,6 +818,7 @@ class FileSaveUploadTest extends FileHookTestCase { function testHandleFileMunge() { // Ensure insecure uploads are disabled for this test. variable_set('allow_insecure_uploads', 0); + $original_image_uri = $this->image->uri; $this->image = file_move($this->image, $this->image->uri . '.foo.' . $this->image_extension); // Reset the hook counters to get rid of the 'move' we just called. @@ -789,13 +843,33 @@ class FileSaveUploadTest extends FileHookTestCase { // Check that the correct hooks were called. $this->assertFileHooksCalled(array('validate', 'insert')); + // Reset the hook counters. + file_test_reset(); + + // Ensure we don't munge the .foo extension if it is in the list of allowed + // extensions. + $extensions = 'foo ' . $this->image_extension; + $edit = array( + 'files[file_test_upload]' => drupal_realpath($this->image->uri), + 'extensions' => $extensions, + ); + + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, 'Received a 200 response for posted test file.'); + $this->assertNoRaw(t('For security reasons, your upload has been renamed'), 'Found no security message.'); + $this->assertRaw(t('File name is @filename', array('@filename' => 'image-test.png.foo.png')), 'File was not munged when all extensions within it are allowed.'); + $this->assertRaw(t('You WIN!'), 'Found the success message.'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate', 'insert')); + // Ensure we don't munge files if we're allowing any extension. // Reset the hook counters. file_test_reset(); $edit = array( 'files[file_test_upload]' => drupal_realpath($this->image->uri), - 'allow_all_extensions' => TRUE, + 'allow_all_extensions' => 'empty_array', ); $this->drupalPost('file-test/upload', $edit, t('Submit')); @@ -806,6 +880,94 @@ class FileSaveUploadTest extends FileHookTestCase { // Check that the correct hooks were called. $this->assertFileHooksCalled(array('validate', 'insert')); + + // Test that a dangerous extension such as .php is munged even if it is in + // the list of allowed extensions. + $this->image = file_move($this->image, $original_image_uri . '.php.' . $this->image_extension); + // Reset the hook counters. + file_test_reset(); + + $extensions = 'php ' . $this->image_extension; + $edit = array( + 'files[file_test_upload]' => drupal_realpath($this->image->uri), + 'extensions' => $extensions, + ); + + $munged_filename = $this->image->filename; + $munged_filename = substr($munged_filename, 0, strrpos($munged_filename, '.')); + $munged_filename .= '_.' . $this->image_extension; + + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, 'Received a 200 response for posted test file.'); + $this->assertRaw(t('For security reasons, your upload has been renamed'), 'Found security message.'); + $this->assertRaw(t('File name is @filename', array('@filename' => $munged_filename)), 'File was successfully munged.'); + $this->assertRaw(t('You WIN!'), 'Found the success message.'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate', 'insert')); + + // Reset the hook counters. + file_test_reset(); + + // Dangerous extensions are munged even when all extensions are allowed. + $edit = array( + 'files[file_test_upload]' => drupal_realpath($this->image->uri), + 'allow_all_extensions' => 'empty_array', + ); + + $munged_filename = $this->image->filename; + $munged_filename = substr($munged_filename, 0, strrpos($munged_filename, '.')); + $munged_filename .= '_.' . $this->image_extension; + + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, 'Received a 200 response for posted test file.'); + $this->assertRaw(t('For security reasons, your upload has been renamed'), 'Found security message.'); + $this->assertRaw(t('File name is @filename.', array('@filename' => 'image-test.png_.php_.png_.txt')), 'File was successfully munged.'); + $this->assertRaw(t('You WIN!'), 'Found the success message.'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate', 'insert')); + + // Dangerous extensions are munged if is renamed to end in .txt. + $this->image = file_move($this->image, $original_image_uri . '.cgi.' . $this->image_extension . '.txt'); + // Reset the hook counters. + file_test_reset(); + + $edit = array( + 'files[file_test_upload]' => drupal_realpath($this->image->uri), + 'allow_all_extensions' => 'empty_array', + ); + + $munged_filename = $this->image->filename; + $munged_filename = substr($munged_filename, 0, strrpos($munged_filename, '.')); + $munged_filename .= '_.' . $this->image_extension; + + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, 'Received a 200 response for posted test file.'); + $this->assertRaw(t('For security reasons, your upload has been renamed'), 'Found security message.'); + $this->assertRaw(t('File name is @filename.', array('@filename' => 'image-test.png_.cgi_.png_.txt')), 'File was successfully munged.'); + $this->assertRaw(t('You WIN!'), 'Found the success message.'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate', 'insert')); + + // Reset the hook counters. + file_test_reset(); + + // Ensure that setting $validators['file_validate_extensions'] = array('') + // rejects all files without munging or renaming. + $edit = array( + 'files[file_test_upload]' => drupal_realpath($this->image->uri), + 'allow_all_extensions' => 'empty_string', + ); + + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, 'Received a 200 response for posted test file.'); + $this->assertNoRaw(t('For security reasons, your upload has been renamed'), 'Found security message.'); + $this->assertRaw(t('Epic upload FAIL!'), 'Found the failure message.'); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate')); } /** @@ -2192,6 +2354,25 @@ class FileValidateTest extends FileHookTestCase { $this->assertEqual(file_validate($file, $failing), array('Failed', 'Badly', 'Epic fail'), 'Validating returns errors.'); $this->assertFileHooksCalled(array('validate')); } + + /** + * Tests hard-coded security check in file_validate(). + */ + public function testInsecureExtensions() { + $file = $this->createFile('test.php', 'Invalid PHP'); + + // Test that file_validate() will check for insecure extensions by default. + $errors = file_validate($file, array()); + $this->assertEqual('For security reasons, your upload has been rejected.', $errors[0]); + $this->assertFileHooksCalled(array('validate')); + file_test_reset(); + + // Test that the 'allow_insecure_uploads' is respected. + variable_set('allow_insecure_uploads', 1); + $errors = file_validate($file, array()); + $this->assertEqual(array(), $errors); + $this->assertFileHooksCalled(array('validate')); + } } /** @@ -2561,7 +2742,7 @@ class FileNameMungingTest extends FileTestCase { function setUp() { parent::setUp(); - $this->bad_extension = 'php'; + $this->bad_extension = 'foo'; $this->name = $this->randomName() . '.' . $this->bad_extension . '.txt'; $this->name_with_uc_ext = $this->randomName() . '.' . strtoupper($this->bad_extension) . '.txt'; } @@ -2610,6 +2791,18 @@ class FileNameMungingTest extends FileTestCase { $this->assertIdentical($munged_name, $this->name, format_string('The new filename (%munged) matches the original (%original) also when the whitelisted extension is in uppercase.', array('%munged' => $munged_name, '%original' => $this->name))); } + /** + * Tests unsafe extensions are munged by file_munge_filename(). + */ + public function testMungeUnsafe() { + $prefix = $this->randomName(); + $name = "$prefix.php.txt"; + // Put the php extension in the allowed list, but since it is in the unsafe + // extension list, it should still be munged. + $munged_name = file_munge_filename($name, 'php txt'); + $this->assertIdentical($munged_name, "$prefix.php_.txt", format_string('The filename (%munged) has been modified from the original (%original) if the allowed extension is also on the unsafe list.', array('%munged' => $munged_name, '%original' => $name))); + } + /** * Ensure that unmunge gets your name back. */ diff --git a/modules/simpletest/tests/file_test.module b/modules/simpletest/tests/file_test.module index 1b11316f9f0..b7d6fd0c2ff 100644 --- a/modules/simpletest/tests/file_test.module +++ b/modules/simpletest/tests/file_test.module @@ -76,9 +76,13 @@ function _file_test_form($form, &$form_state) { ); $form['allow_all_extensions'] = array( - '#type' => 'checkbox', - '#title' => t('Allow all extensions?'), - '#default_value' => FALSE, + '#type' => 'radios', + '#options' => array( + 'false' => 'No', + 'empty_array' => 'Empty array', + 'empty_string' => 'Empty string', + ), + '#default_value' => 'false', ); $form['is_image_file'] = array( @@ -114,9 +118,13 @@ function _file_test_form_submit(&$form, &$form_state) { $validators['file_validate_is_image'] = array(); } - if ($form_state['values']['allow_all_extensions']) { + $allow = $form_state['values']['allow_all_extensions']; + if ($allow === 'empty_array') { $validators['file_validate_extensions'] = array(); } + elseif ($allow === 'empty_string') { + $validators['file_validate_extensions'] = array(''); + } elseif (!empty($form_state['values']['extensions'])) { $validators['file_validate_extensions'] = array($form_state['values']['extensions']); }