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']);
}