diff --git includes/file.inc includes/file.inc index 5eed882..c5a6353 100644 --- includes/file.inc +++ includes/file.inc @@ -1098,6 +1098,12 @@ function file_space_used($uid = NULL, $status = FILE_STATUS_PERMANENT) { * @param $validators * An optional, associative array of callback functions used to validate the * file. See file_validate() for a full discussion of the array format. + * If no extension validator is provided it will default to a limited safe + * list of extensions which is as follows: "jpg jpeg gif png txt + * doc xls pdf ppt pps odt ods odp". To allow all extensions you must + * explicitly set the 'file_validate_extensions' validator to an empty array + * (Beware: this is not safe and should only be allowed for trusted users, if + * at all). * @param $destination * A string containing the URI $source should be copied to. * This must be a stream wrapper URI. If this value is omitted, Drupal's @@ -1160,28 +1166,56 @@ function file_save_upload($source, $validators = array(), $destination = FALSE, return FALSE; } - // Build the list of non-munged extensions. - // @todo: this should not be here. we need to figure out the right place. - $extensions = ''; - foreach ($user->roles as $rid => $name) { - $extensions .= ' ' . variable_get("upload_extensions_$rid", - variable_get('upload_extensions_default', 'jpg jpeg gif png txt html doc xls pdf ppt pps odt ods odp')); - } - // Begin building file object. $file = new stdClass(); $file->uid = $user->uid; $file->status = 0; - $file->filename = file_munge_filename(trim(basename($_FILES['files']['name'][$source]), '.'), $extensions); + $file->filename = trim(basename($_FILES['files']['name'][$source]), '.'); $file->uri = $_FILES['files']['tmp_name'][$source]; $file->filemime = file_get_mimetype($file->filename); $file->filesize = $_FILES['files']['size'][$source]; - // Rename potentially executable files, to help prevent exploits. - if (preg_match('/\.(php|pl|py|cgi|asp|js)$/i', $file->filename) && (substr($file->filename, -4) != '.txt')) { + $extensions = ''; + if (isset($validators['file_validate_extensions'])) { + if (isset($validators['file_validate_extensions'][0])) { + // Build the list of non-munged extensions if the caller provided them. + $extensions = $validators['file_validate_extensions'][0]; + } + else { + // If 'file_validate_extensions' is set and the list is empty then the + // caller wants to allow any extension. In this case we have to remove the + // validator or else it will reject all extensions. + unset($validators['file_validate_extensions']); + } + } + else { + // No validator was provided, so add one using the default list. + // Build a default non-munged safe list for file_munge_filename(). + $extensions = 'jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp'; + $validators['file_validate_extensions'] = array(); + $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|pl|py|cgi|asp|js)(\.|$)/i', $file->filename) && (substr($file->filename, -4) != '.txt')) { $file->filemime = 'text/plain'; $file->uri .= '.txt'; $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 (!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))); + } } // If the destination is not provided, use the temporary directory. diff --git modules/aggregator/aggregator.admin.inc modules/aggregator/aggregator.admin.inc index c674169..0b61857 100644 --- modules/aggregator/aggregator.admin.inc +++ modules/aggregator/aggregator.admin.inc @@ -300,7 +300,8 @@ function aggregator_form_opml_validate($form, &$form_state) { */ function aggregator_form_opml_submit($form, &$form_state) { $data = ''; - if ($file = file_save_upload('upload')) { + $validators = array('file_validate_extensions' => array('opml xml')); + if ($file = file_save_upload('upload', $validators)) { $data = file_get_contents($file->uri); } else { diff --git modules/locale/locale.admin.inc modules/locale/locale.admin.inc index 13acf06..19a6e03 100644 --- modules/locale/locale.admin.inc +++ modules/locale/locale.admin.inc @@ -984,8 +984,9 @@ function locale_translate_import_form($form) { * Process the locale import form submission. */ function locale_translate_import_form_submit($form, &$form_state) { + $validators = array('file_validate_extensions' => array('po')); // Ensure we have the file uploaded - if ($file = file_save_upload('file')) { + if ($file = file_save_upload('file', $validators)) { // Add language, if not yet supported drupal_static_reset('language_list'); diff --git modules/locale/locale.test modules/locale/locale.test index 6fa5c23..0c68e6a 100644 --- modules/locale/locale.test +++ modules/locale/locale.test @@ -758,7 +758,7 @@ class LocaleImportFunctionalTest extends DrupalWebTestCase { * Additional options to pass to the translation import form. */ function importPoFile($contents, array $options = array()) { - $name = tempnam(file_directory_path('temporary'), "po_"); + $name = tempnam(file_directory_path('temporary'), "po_") . '.po'; file_put_contents($name, $contents); $options['files[file]'] = $name; $this->drupalPost('admin/config/regional/translate/import', $options, t('Import')); @@ -905,7 +905,7 @@ class LocaleExportFunctionalTest extends DrupalWebTestCase { function testExportTranslation() { // First import some known translations. // This will also automatically enable the 'fr' language. - $name = tempnam(file_directory_path('temporary'), "po_"); + $name = tempnam(file_directory_path('temporary'), "po_") . '.po'; file_put_contents($name, $this->getPoFile()); $this->drupalPost('admin/config/regional/translate/import', array( 'langcode' => 'fr', diff --git modules/simpletest/tests/file.test modules/simpletest/tests/file.test index 9bd08f2..fc2f350 100644 --- modules/simpletest/tests/file.test +++ modules/simpletest/tests/file.test @@ -537,12 +537,17 @@ class FileSaveUploadTest extends FileHookTestCase { /** * An image file path for uploading. */ - var $image; + protected $image; + + /** + * A PHP file path for upload security testing. + */ + protected $phpfile; /** * The largest file id when the test starts. */ - var $maxFidBefore; + protected $maxFidBefore; public static function getInfo() { return array( @@ -558,14 +563,17 @@ class FileSaveUploadTest extends FileHookTestCase { $this->drupalLogin($account); $this->image = current($this->drupalGetTestFiles('image')); - $this->assertTrue(is_file($this->image->uri), t("The file we're going to upload exists.")); + $this->assertTrue(is_file($this->image->uri), t("The image file we're going to upload exists.")); + + $this->phpfile = current($this->drupalGetTestFiles('php')); + $this->assertTrue(is_file($this->phpfile->uri), t("The PHP file we're going to upload exists.")); $this->maxFidBefore = db_query('SELECT MAX(fid) AS fid FROM {file_managed}')->fetchField(); - // Upload with replace to gurantee there's something there. + // Upload with replace to guarantee there's something there. $edit = array( 'file_test_replace' => FILE_EXISTS_REPLACE, - 'files[file_test_upload]' => drupal_realpath($this->image->uri) + 'files[file_test_upload]' => drupal_realpath($this->image->uri), ); $this->drupalPost('file-test/upload', $edit, t('Submit')); $this->assertResponse(200, t('Received a 200 response for posted test file.')); @@ -631,6 +639,158 @@ class FileSaveUploadTest extends FileHookTestCase { } /** + * Test extension handling. + */ + function testHandleExtension() { + // The file being tested is a .gif which is in the default safe list + // of extensions to allow when the extension validator isn't used. This is + // implicitly tested at the testNormal() test. Here we tell + // file_save_upload() to only allow ".foo". + $extensions = 'foo'; + $edit = array( + 'file_test_replace' => FILE_EXISTS_REPLACE, + 'files[file_test_upload]' => drupal_realpath($this->image->uri), + 'extensions' => $extensions, + ); + + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, t('Received a 200 response for posted test file.')); + $message = t('Only files with the following extensions are allowed: ') . '' . $extensions . ''; + $this->assertRaw($message, t('Can\'t upload a disallowed extension')); + $this->assertRaw(t('Epic upload FAIL!'), t('Found the failure message.')); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate')); + + // Reset the hook counters. + file_test_reset(); + + $extensions = 'foo gif'; + // Now tell file_save_upload() to allow ".gif". + $edit = array( + 'file_test_replace' => FILE_EXISTS_REPLACE, + 'files[file_test_upload]' => drupal_realpath($this->image->uri), + 'extensions' => $extensions, + ); + + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, t('Received a 200 response for posted test file.')); + $this->assertNoRaw(t('Only files with the following extensions are allowed:'), t('Can upload an allowed extension.')); + $this->assertRaw(t('You WIN!'), t('Found the success message.')); + + // 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. + $edit = array( + 'file_test_replace' => FILE_EXISTS_REPLACE, + 'files[file_test_upload]' => drupal_realpath($this->image->uri), + 'allow_all_extensions' => TRUE, + ); + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, t('Received a 200 response for posted test file.')); + $this->assertNoRaw(t('Only files with the following extensions are allowed:'), t('Can upload any extension.')); + $this->assertRaw(t('You WIN!'), t('Found the success message.')); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate', 'load', 'update')); + } + + /** + * 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. + $edit = array( + 'file_test_replace' => FILE_EXISTS_REPLACE, + 'files[file_test_upload]' => drupal_realpath($this->phpfile->uri), + 'is_image_file' => FALSE, + 'extensions' => 'php', + ); + + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, t('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, t('Dangerous file was renamed.')); + $this->assertRaw(t('File MIME type is text/plain.'), t('Dangerous file\'s MIME type was changed.')); + $this->assertRaw(t('You WIN!'), t('Found the success message.')); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate', 'insert')); + + // Ensure dangerous files are not renamed when insecure uploads is TRUE. + // Turn on insecure uploads. + variable_set('allow_insecure_uploads', 1); + // Reset the hook counters. + file_test_reset(); + + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, t('Received a 200 response for posted test file.')); + $this->assertNoRaw(t('For security reasons, your upload has been renamed'), t('Found no security message.')); + $this->assertRaw(t('File name is !filename', array('!filename' => $this->phpfile->filename)), t('Dangerous file was not renamed when insecure uploads is TRUE.')); + $this->assertRaw(t('You WIN!'), t('Found the success message.')); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate', 'insert')); + + // Turn off insecure uploads. + variable_set('allow_insecure_uploads', 0); + } + + /** + * Test file munge handling. + */ + function testHandleFileMunge() { + // Ensure insecure uploads are disabled for this test. + variable_set('allow_insecure_uploads', 0); + $this->image = file_move($this->image, $this->image->uri . '.foo.gif'); + + // Reset the hook counters to get rid of the 'move' we just called. + file_test_reset(); + + $extensions = 'gif'; + $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 .= '_.gif'; + + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, t('Received a 200 response for posted test file.')); + $this->assertRaw(t('For security reasons, your upload has been renamed'), t('Found security message.')); + $this->assertRaw(t('File name is !filename', array('!filename' => $munged_filename)), t('File was successfully munged.')); + $this->assertRaw(t('You WIN!'), t('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, + ); + + $this->drupalPost('file-test/upload', $edit, t('Submit')); + $this->assertResponse(200, t('Received a 200 response for posted test file.')); + $this->assertNoRaw(t('For security reasons, your upload has been renamed'), t('Found no security message.')); + $this->assertRaw(t('File name is !filename', array('!filename' => $this->image->filename)), t('File was not munged when allowing any extension.')); + $this->assertRaw(t('You WIN!'), t('Found the success message.')); + + // Check that the correct hooks were called. + $this->assertFileHooksCalled(array('validate', 'insert')); + } + + /** * Test renaming when uploading over a file that already exists. */ function testExistingRename() { diff --git modules/simpletest/tests/file_test.module modules/simpletest/tests/file_test.module index d7624e5..24a5b61 100644 --- modules/simpletest/tests/file_test.module +++ modules/simpletest/tests/file_test.module @@ -47,7 +47,7 @@ function file_test_stream_wrappers() { function _file_test_form($form, &$form_state) { $form['file_test_upload'] = array( '#type' => 'file', - '#title' => t('Upload an image'), + '#title' => t('Upload a file'), ); $form['file_test_replace'] = array( '#type' => 'select', @@ -61,9 +61,28 @@ function _file_test_form($form, &$form_state) { ); $form['file_subdir'] = array( '#type' => 'textfield', - '#title' => 'Subdirectory for test image', + '#title' => t('Subdirectory for test file'), '#default_value' => '', ); + + $form['extensions'] = array( + '#type' => 'textfield', + '#title' => t('Allowed extensions.'), + '#default_value' => '', + ); + + $form['allow_all_extensions'] = array( + '#type' => 'checkbox', + '#title' => t('Allow all extensions?'), + '#default_value' => FALSE, + ); + + $form['is_image_file'] = array( + '#type' => 'checkbox', + '#title' => t('Is this an image file?'), + '#default_value' => TRUE, + ); + $form['submit'] = array( '#type' => 'submit', '#value' => t('Submit'), @@ -75,7 +94,7 @@ function _file_test_form($form, &$form_state) { * Process the upload. */ function _file_test_form_submit(&$form, &$form_state) { - // Process the upload and validate that it is an image. Note: we're using the + // Process the upload and perform validation. Note: we're using the // form value for the $replace parameter. if (!empty($form_state['values']['file_subdir'])) { $destination = 'temporary://' . $form_state['values']['file_subdir']; @@ -84,10 +103,26 @@ function _file_test_form_submit(&$form, &$form_state) { else { $destination = FALSE; } - $file = file_save_upload('file_test_upload', array('file_validate_is_image' => array()), $destination, $form_state['values']['file_test_replace']); + + // Setup validators. + $validators = array(); + if ($form_state['values']['is_image_file']) { + $validators['file_validate_is_image'] = array(); + } + + if ($form_state['values']['allow_all_extensions']) { + $validators['file_validate_extensions'] = array(); + } + else if (!empty($form_state['values']['extensions'])) { + $validators['file_validate_extensions'] = array($form_state['values']['extensions']); + } + + $file = file_save_upload('file_test_upload', $validators, $destination, $form_state['values']['file_test_replace']); if ($file) { $form_state['values']['file_test_upload'] = $file; drupal_set_message(t('File @filepath was uploaded.', array('@filepath' => $file->uri))); + drupal_set_message(t('File name is @filename.', array('@filename' => $file->filename))); + drupal_set_message(t('File MIME type is @mimetype.', array('@mimetype' => $file->filemime))); drupal_set_message(t('You WIN!')); } elseif ($file === FALSE) {