From 104250f4ea2f130e7e604f259bf37a2ad811e0a6 Mon Sep 17 00:00:00 2001
From: xjm <xjm@65776.no-reply.drupal.org>
Date: Tue, 17 Nov 2020 16:16:53 -0600
Subject: [PATCH] =?UTF-8?q?SA-CORE-2020-012=20by=20ufku,=20mrf,=20fgm,=20s?=
=?UTF-8?q?amuel.mortenson,=20dww,=20Heine,=20mlhess,=20David=5FRothstein,?=
=?UTF-8?q?=20pwolanin,=C2=A0xjm,=20fgm,=20stefan.r,=20dsnopek,=20rickmane?=
=?UTF-8?q?lius,=20David=20Strauss,=20tedbow,=20alexpott,=20dww,=20larowla?=
=?UTF-8?q?n,=20kim.pepper,=20Wim=20Leers,=20quicksketch,=20mcdruid,=20Fab?=
=?UTF-8?q?ianx,=20effulgentsia,=20drumm,=20pandaski,=20Mixologic?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
(cherry picked from commit 1994ca5ccb67007684ee7faf0c3481345d5b4f71)
---
core/includes/file.inc | 13 +-
core/modules/file/file.module | 59 +++++--
.../rest/resource/FileUploadResource.php | 54 ++++--
.../tests/file_test/src/Form/FileTestForm.php | 15 +-
.../src/Form/FileTestSaveUploadFromForm.php | 17 +-
.../src/Functional/SaveUploadFormTest.php | 52 +++++-
.../tests/src/Functional/SaveUploadTest.php | 167 +++++++++++++++++-
.../file/tests/src/Kernel/ValidateTest.php | 19 ++
.../TemporaryJsonapiFileFieldUploader.php | 54 ++++--
.../tests/src/Functional/FileUploadTest.php | 53 +++++-
.../Functional/FileUploadResourceTestBase.php | 53 +++++-
.../KernelTests/Core/File/NameMungingTest.php | 37 +++-
12 files changed, 498 insertions(+), 95 deletions(-)
diff --git a/core/includes/file.inc b/core/includes/file.inc
index c40851dedb90..33fd9f8b8170 100644
--- a/core/includes/file.inc
+++ b/core/includes/file.inc
@@ -159,8 +159,8 @@ function file_build_uri($path) {
* 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 configuration
* 'system.file:allow_insecure_uploads'. If it evaluates to TRUE, no alterations
@@ -168,7 +168,8 @@ function file_build_uri($path) {
* @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::messenger()->addStatus() will be called to display
* a message if the file name was changed.
@@ -187,6 +188,12 @@ function file_munge_filename($filename, $extensions, $alerts = TRUE) {
$allowed_extensions = array_unique(explode(' ', strtolower(trim($extensions))));
+ // Remove unsafe extensions from the allowed list of extensions.
+ // @todo https://www.drupal.org/project/drupal/issues/3032390 Make the list
+ // of unsafe extensions a constant. The list is copied from
+ // FILE_INSECURE_EXTENSION_REGEX.
+ $allowed_extensions = array_diff($allowed_extensions, explode('|', 'phar|php|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);
diff --git a/core/modules/file/file.module b/core/modules/file/file.module
index 596a860ebeda..0e5c0b31868d 100644
--- a/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -281,7 +281,17 @@ function file_validate(FileInterface $file, $validators = []) {
}
// Let other modules perform validation on the new file.
- return array_merge($errors, \Drupal::moduleHandler()->invokeAll('file_validate', [$file]));
+ $errors = array_merge($errors, \Drupal::moduleHandler()->invokeAll('file_validate', [$file]));
+
+ // Ensure the file does not contain a malicious extension. At this point
+ // _file_save_upload_single() 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) && !\Drupal::config('system.file')->get('allow_insecure_uploads') && preg_match(FILE_INSECURE_EXTENSION_REGEX, $file->getFilename())) {
+ $errors[] = t('For security reasons, your upload has been rejected.');
+ }
+ return $errors;
}
/**
@@ -978,25 +988,36 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
$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->setFilename(file_munge_filename($file->getFilename(), $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 (!\Drupal::config('system.file')->get('allow_insecure_uploads') && preg_match(FILE_INSECURE_EXTENSION_REGEX, $file->getFilename()) && (substr($file->getFilename(), -4) != '.txt')) {
- $file->setMimeType('text/plain');
- // The destination filename will also later be used to create the URI.
- $file->setFilename($file->getFilename() . '.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.
+ // Don't rename if 'allow_insecure_uploads' evaluates to TRUE.
+ if (!\Drupal::config('system.file')->get('allow_insecure_uploads')) {
if (!empty($extensions)) {
- $validators['file_validate_extensions'][0] .= ' txt';
- \Drupal::messenger()->addStatus(t('For security reasons, your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]));
+ // Munge the filename to protect against possible malicious extension
+ // hiding within an unknown file type (ie: filename.html.foo).
+ $file->setFilename(file_munge_filename($file->getFilename(), $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).
+ 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))) {
+ $file->setMimeType('text/plain');
+ $filename = $file->getFilename();
+ if (substr($filename, -4) != '.txt') {
+ // The destination filename will also later be used to create the URI.
+ $filename .= '.txt';
+ }
+ $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/src/Plugin/rest/resource/FileUploadResource.php b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
index 1aaa5a753bad..3c7180a0b03b 100644
--- a/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
+++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
@@ -468,26 +468,42 @@ protected function validate(FileInterface $file, array $validators) {
* The prepared/munged filename.
*/
protected function prepareFilename($filename, array &$validators) {
- if (!empty($validators['file_validate_extensions'][0])) {
- // If there is a file_validate_extensions validator and a list of
- // valid extensions, munge the filename to protect against possible
- // malicious extension hiding within an unknown file type. For example,
- // "filename.html.foo".
- $filename = file_munge_filename($filename, $validators['file_validate_extensions'][0]);
- }
-
- // 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 (!$this->systemFileConfig->get('allow_insecure_uploads') && preg_match(FILE_INSECURE_EXTENSION_REGEX, $filename) && (substr($filename, -4) != '.txt')) {
- // The destination filename will also later be used to create the URI.
- $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.
+ // Don't rename if 'allow_insecure_uploads' evaluates to TRUE.
+ if (!$this->systemFileConfig->get('allow_insecure_uploads')) {
if (!empty($validators['file_validate_extensions'][0])) {
- $validators['file_validate_extensions'][0] .= ' txt';
+ // If there is a file_validate_extensions validator and a list of
+ // valid extensions, munge the filename to protect against possible
+ // malicious extension hiding within an unknown file type. For example,
+ // "filename.html.foo".
+ $filename = file_munge_filename($filename, $validators['file_validate_extensions'][0]);
+ }
+
+ // 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).
+ if (preg_match(FILE_INSECURE_EXTENSION_REGEX, $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.
+ $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]));
+ }
+ if (empty($validators['file_validate_extensions'][0]) || $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/file_test/src/Form/FileTestForm.php b/core/modules/file/tests/file_test/src/Form/FileTestForm.php
index 08c4a57ffd7a..c9fc1f0e9311 100644
--- a/core/modules/file/tests/file_test/src/Form/FileTestForm.php
+++ b/core/modules/file/tests/file_test/src/Form/FileTestForm.php
@@ -49,9 +49,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
];
$form['allow_all_extensions'] = [
- '#type' => 'checkbox',
'#title' => t('Allow all extensions?'),
- '#default_value' => FALSE,
+ '#type' => 'radios',
+ '#options' => [
+ 'false' => 'No',
+ 'empty_array' => 'Empty array',
+ 'empty_string' => 'Empty string',
+ ],
+ '#default_value' => 'false',
];
$form['is_image_file'] = [
@@ -92,9 +97,13 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
$validators['file_validate_is_image'] = [];
}
- if ($form_state->getValue('allow_all_extensions')) {
+ $allow = $form_state->getValue('allow_all_extensions');
+ if ($allow === 'empty_array') {
$validators['file_validate_extensions'] = [];
}
+ elseif ($allow === 'empty_string') {
+ $validators['file_validate_extensions'] = [''];
+ }
elseif (!$form_state->isValueEmpty('extensions')) {
$validators['file_validate_extensions'] = [$form_state->getValue('extensions')];
}
diff --git a/core/modules/file/tests/file_test/src/Form/FileTestSaveUploadFromForm.php b/core/modules/file/tests/file_test/src/Form/FileTestSaveUploadFromForm.php
index 10127dc94cfe..79bff3ad7b5c 100644
--- a/core/modules/file/tests/file_test/src/Form/FileTestSaveUploadFromForm.php
+++ b/core/modules/file/tests/file_test/src/Form/FileTestSaveUploadFromForm.php
@@ -90,9 +90,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
];
$form['allow_all_extensions'] = [
- '#type' => 'checkbox',
- '#title' => $this->t('Allow all extensions?'),
- '#default_value' => FALSE,
+ '#title' => t('Allow all extensions?'),
+ '#type' => 'radios',
+ '#options' => [
+ 'false' => 'No',
+ 'empty_array' => 'Empty array',
+ 'empty_string' => 'Empty string',
+ ],
+ '#default_value' => 'false',
];
$form['is_image_file'] = [
@@ -139,9 +144,13 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
$validators['file_validate_is_image'] = [];
}
- if ($form_state->getValue('allow_all_extensions')) {
+ $allow = $form_state->getValue('allow_all_extensions');
+ if ($allow === 'empty_array') {
$validators['file_validate_extensions'] = [];
}
+ elseif ($allow === 'empty_string') {
+ $validators['file_validate_extensions'] = [''];
+ }
elseif (!$form_state->isValueEmpty('extensions')) {
$validators['file_validate_extensions'] = [$form_state->getValue('extensions')];
}
diff --git a/core/modules/file/tests/src/Functional/SaveUploadFormTest.php b/core/modules/file/tests/src/Functional/SaveUploadFormTest.php
index 643dbcf37c62..c80ddf242089 100644
--- a/core/modules/file/tests/src/Functional/SaveUploadFormTest.php
+++ b/core/modules/file/tests/src/Functional/SaveUploadFormTest.php
@@ -198,7 +198,7 @@ public function testHandleExtension() {
$edit = [
'file_test_replace' => FileSystemInterface::EXISTS_REPLACE,
'files[file_test_upload][]' => $file_system->realpath($this->image->getFileUri()),
- 'allow_all_extensions' => TRUE,
+ 'allow_all_extensions' => 'empty_array',
];
$this->drupalPostForm('file-test/save_upload_from_form_test', $edit, 'Submit');
$this->assertSession()->statusCodeEquals(200);
@@ -227,7 +227,7 @@ public function testHandleDangerousFile() {
$this->drupalPostForm('file-test/save_upload_from_form_test', $edit, 'Submit');
$this->assertSession()->statusCodeEquals(200);
- $message = t('For security reasons, your upload has been renamed to') . ' <em class="placeholder">' . $this->phpfile->filename . '.txt' . '</em>';
+ $message = t('For security reasons, your upload has been renamed to') . ' <em class="placeholder">' . $this->phpfile->filename . '_.txt' . '</em>';
$this->assertRaw($message);
$this->assertRaw(t('File MIME type is text/plain.'));
$this->assertRaw(t('You WIN!'));
@@ -262,7 +262,8 @@ public function testHandleFileMunge() {
$file_system = \Drupal::service('file_system');
// Ensure insecure uploads are disabled for this test.
$this->config('system.file')->set('allow_insecure_uploads', 0)->save();
- $this->image = file_move($this->image, $this->image->getFileUri() . '.foo.' . $this->imageExtension);
+ $original_uri = $this->image->getFileUri();
+ $this->image = file_move($this->image, $original_uri . '.foo.' . $this->imageExtension);
// Reset the hook counters to get rid of the 'move' we just called.
file_test_reset();
@@ -286,13 +287,37 @@ public function testHandleFileMunge() {
// Check that the correct hooks were called.
$this->assertFileHooksCalled(['validate', 'insert']);
+ // Test with uppercase extensions.
+ $this->image = file_move($this->image, $original_uri . '.foo2.' . $this->imageExtension);
+ // Reset the hook counters.
+ file_test_reset();
+ $extensions = $this->imageExtension;
+ $edit = [
+ 'files[file_test_upload][]' => $file_system->realpath($this->image->getFileUri()),
+ 'extensions' => mb_strtoupper($extensions),
+ ];
+
+ $munged_filename = $this->image->getFilename();
+ $munged_filename = substr($munged_filename, 0, strrpos($munged_filename, '.'));
+ $munged_filename .= '_.' . $this->imageExtension;
+
+ $this->drupalPostForm('file-test/save_upload_from_form_test', $edit, 'Submit');
+ $this->assertSession()->statusCodeEquals(200);
+ $this->assertRaw(t('For security reasons, your upload has been renamed'));
+ $this->assertRaw(t('File name is @filename', ['@filename' => $munged_filename]));
+ $this->assertRaw(t('You WIN!'));
+
+ // Check that the correct hooks were called.
+ $this->assertFileHooksCalled(['validate', 'insert']);
+
// Ensure we don't munge files if we're allowing any extension.
// Reset the hook counters.
file_test_reset();
+ // Ensure we don't munge files if we're allowing any extension.
$edit = [
'files[file_test_upload][]' => $file_system->realpath($this->image->getFileUri()),
- 'allow_all_extensions' => TRUE,
+ 'allow_all_extensions' => 'empty_array',
];
$this->drupalPostForm('file-test/save_upload_from_form_test', $edit, 'Submit');
@@ -303,6 +328,24 @@ public function testHandleFileMunge() {
// Check that the correct hooks were called.
$this->assertFileHooksCalled(['validate', 'insert']);
+
+ // Ensure that setting $validators['file_validate_extensions'] = ['']
+ // rejects all files.
+ // Reset the hook counters.
+ file_test_reset();
+
+ $edit = [
+ 'files[file_test_upload][]' => $file_system->realpath($this->image->getFileUri()),
+ 'allow_all_extensions' => 'empty_string',
+ ];
+
+ $this->drupalPostForm('file-test/save_upload_from_form_test', $edit, 'Submit');
+ $this->assertSession()->statusCodeEquals(200);
+ $this->assertNoRaw(t('For security reasons, your upload has been renamed'));
+ $this->assertRaw(t('Epic upload FAIL!'));
+
+ // Check that the correct hooks were called.
+ $this->assertFileHooksCalled(['validate']);
}
/**
@@ -467,7 +510,6 @@ public function testCombinedErrorMessages() {
$submit_xpath = $this->assertSession()->buttonExists('Submit')->getXpath();
$form = $client->getCrawler()->filterXPath($submit_xpath)->form();
$edit = [
- 'allow_all_extensions' => FALSE,
'is_image_file' => TRUE,
'extensions' => 'jpeg',
];
diff --git a/core/modules/file/tests/src/Functional/SaveUploadTest.php b/core/modules/file/tests/src/Functional/SaveUploadTest.php
index 2e765cbc72a0..3b185fc10246 100644
--- a/core/modules/file/tests/src/Functional/SaveUploadTest.php
+++ b/core/modules/file/tests/src/Functional/SaveUploadTest.php
@@ -219,7 +219,7 @@ public function testHandleExtension() {
$edit = [
'file_test_replace' => FileSystemInterface::EXISTS_REPLACE,
'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->image->getFileUri()),
- 'allow_all_extensions' => TRUE,
+ 'allow_all_extensions' => 'empty_array',
];
$this->drupalPostForm('file-test/upload', $edit, 'Submit');
$this->assertSession()->statusCodeEquals(200);
@@ -228,6 +228,27 @@ public function testHandleExtension() {
// Check that the correct hooks were called.
$this->assertFileHooksCalled(['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 = [
+ 'file_test_replace' => FileSystemInterface::EXISTS_REPLACE,
+ 'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->phpfile->uri),
+ 'allow_all_extensions' => 'empty_array',
+ 'is_image_file' => FALSE,
+ ];
+ $this->drupalPostForm('file-test/upload', $edit, 'Submit');
+ $this->assertSession()->statusCodeEquals(200);
+ $message = t('For security reasons, your upload has been renamed to') . ' <em class="placeholder">' . $this->phpfile->filename . '_.txt' . '</em>';
+ $this->assertRaw($message);
+ $this->assertSession()->pageTextContains('File name is php-2.php_.txt.');
+ $this->assertRaw(t('File MIME type is text/plain.'));
+ $this->assertRaw(t('You WIN!'));
+ // Check that the correct hooks were called.
+ $this->assertFileHooksCalled(['validate', 'insert']);
}
/**
@@ -235,8 +256,8 @@ public function testHandleExtension() {
*/
public function testHandleDangerousFile() {
$config = $this->config('system.file');
- // 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 = [
'file_test_replace' => FileSystemInterface::EXISTS_REPLACE,
'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->phpfile->uri),
@@ -246,9 +267,9 @@ public function testHandleDangerousFile() {
$this->drupalPostForm('file-test/upload', $edit, 'Submit');
$this->assertSession()->statusCodeEquals(200);
- $message = t('For security reasons, your upload has been renamed to') . ' <em class="placeholder">' . $this->phpfile->filename . '.txt' . '</em>';
+ $message = t('For security reasons, your upload has been renamed to') . ' <em class="placeholder">' . $this->phpfile->filename . '_.txt' . '</em>';
$this->assertRaw($message);
- $this->assertSession()->pageTextContains('File name is php-2.php.txt.');
+ $this->assertSession()->pageTextContains('File name is php-2.php_.txt.');
$this->assertRaw(t('File MIME type is text/plain.'));
$this->assertRaw(t('You WIN!'));
@@ -270,8 +291,39 @@ public function testHandleDangerousFile() {
// Check that the correct hooks were called.
$this->assertFileHooksCalled(['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->drupalPostForm('file-test/upload', $edit, 'Submit');
+ $this->assertSession()->statusCodeEquals(200);
+ $message = t('Only files with the following extensions are allowed:') . ' <em class="placeholder">' . $edit['extensions'] . '</em>';
+ $this->assertRaw($message);
+ $this->assertRaw(t('Epic upload FAIL!'));
+
+ // Check that the correct hooks were called.
+ $this->assertFileHooksCalled(['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).
$config->set('allow_insecure_uploads', 0)->save();
+ $this->drupalPostForm('file-test/upload', $edit, 'Submit');
+ $this->assertSession()->statusCodeEquals(200);
+ $message = t('Only files with the following extensions are allowed:') . ' <em class="placeholder">' . $edit['extensions'] . '</em>';
+ $this->assertRaw($message);
+ $this->assertRaw(t('Epic upload FAIL!'));
+
+ // Check that the correct hooks were called.
+ $this->assertFileHooksCalled(['validate']);
+
+ // Reset the hook counters.
+ file_test_reset();
}
/**
@@ -280,7 +332,8 @@ public function testHandleDangerousFile() {
public function testHandleFileMunge() {
// Ensure insecure uploads are disabled for this test.
$this->config('system.file')->set('allow_insecure_uploads', 0)->save();
- $this->image = file_move($this->image, $this->image->getFileUri() . '.foo.' . $this->imageExtension);
+ $original_image_uri = $this->image->getFileUri();
+ $this->image = file_move($this->image, $original_image_uri . '.foo.' . $this->imageExtension);
// Reset the hook counters to get rid of the 'move' we just called.
file_test_reset();
@@ -304,13 +357,34 @@ public function testHandleFileMunge() {
// Check that the correct hooks were called.
$this->assertFileHooksCalled(['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->imageExtension;
+ $edit = [
+ 'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->image->getFileUri()),
+ 'extensions' => $extensions,
+ ];
+
+ $this->drupalPostForm('file-test/upload', $edit, 'Submit');
+ $this->assertSession()->statusCodeEquals(200);
+ $this->assertNoRaw(t('For security reasons, your upload has been renamed'));
+ $this->assertRaw(t('File name is @filename', ['@filename' => $this->image->getFilename()]));
+ $this->assertRaw(t('You WIN!'));
+
+ // Check that the correct hooks were called.
+ $this->assertFileHooksCalled(['validate', 'insert']);
+
// Ensure we don't munge files if we're allowing any extension.
+ $this->image = file_move($this->image, $original_image_uri . '.foo.txt.' . $this->imageExtension);
// Reset the hook counters.
file_test_reset();
$edit = [
'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->image->getFileUri()),
- 'allow_all_extensions' => TRUE,
+ 'allow_all_extensions' => 'empty_array',
];
$this->drupalPostForm('file-test/upload', $edit, 'Submit');
@@ -321,6 +395,83 @@ public function testHandleFileMunge() {
// Check that the correct hooks were called.
$this->assertFileHooksCalled(['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->imageExtension);
+ // Reset the hook counters.
+ file_test_reset();
+
+ $extensions = 'php ' . $this->imageExtension;
+ $edit = [
+ 'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->image->getFileUri()),
+ 'extensions' => $extensions,
+ ];
+
+ $this->drupalPostForm('file-test/upload', $edit, 'Submit');
+ $this->assertSession()->statusCodeEquals(200);
+ $this->assertRaw(t('For security reasons, your upload has been renamed'));
+ $this->assertRaw(t('File name is @filename', ['@filename' => 'image-test.png.php_.png']));
+ $this->assertRaw(t('You WIN!'));
+
+ // Check that the correct hooks were called.
+ $this->assertFileHooksCalled(['validate', 'insert']);
+
+ // Reset the hook counters.
+ file_test_reset();
+
+ // Dangerous extensions are munged even when all extensions are allowed.
+ $edit = [
+ 'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->image->getFileUri()),
+ 'allow_all_extensions' => 'empty_array',
+ ];
+
+ $this->drupalPostForm('file-test/upload', $edit, 'Submit');
+ $this->assertSession()->statusCodeEquals(200);
+ $this->assertRaw(t('For security reasons, your upload has been renamed'));
+ $this->assertRaw(t('File name is @filename.', ['@filename' => 'image-test.png_.php_.png_.txt']));
+ $this->assertRaw(t('You WIN!'));
+
+ // Check that the correct hooks were called.
+ $this->assertFileHooksCalled(['validate', 'insert']);
+
+ // Dangerous extensions are munged if is renamed to end in .txt.
+ $this->image = file_move($this->image, $original_image_uri . '.cgi.' . $this->imageExtension . '.txt');
+ // Reset the hook counters.
+ file_test_reset();
+
+ // Dangerous extensions are munged even when all extensions are allowed.
+ $edit = [
+ 'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->image->getFileUri()),
+ 'allow_all_extensions' => 'empty_array',
+ ];
+
+ $this->drupalPostForm('file-test/upload', $edit, 'Submit');
+ $this->assertSession()->statusCodeEquals(200);
+ $this->assertRaw(t('For security reasons, your upload has been renamed'));
+ $this->assertRaw(t('File name is @filename.', ['@filename' => 'image-test.png_.cgi_.png_.txt']));
+ $this->assertRaw(t('You WIN!'));
+
+ // Check that the correct hooks were called.
+ $this->assertFileHooksCalled(['validate', 'insert']);
+
+ // Reset the hook counters.
+ file_test_reset();
+
+ // Ensure that setting $validators['file_validate_extensions'] = ['']
+ // rejects all files without munging or renaming.
+ $edit = [
+ 'files[file_test_upload][]' => \Drupal::service('file_system')->realpath($this->image->getFileUri()),
+ 'allow_all_extensions' => 'empty_string',
+ ];
+
+ $this->drupalPostForm('file-test/save_upload_from_form_test', $edit, 'Submit');
+ $this->assertSession()->statusCodeEquals(200);
+ $this->assertNoRaw(t('For security reasons, your upload has been renamed'));
+ $this->assertRaw(t('Epic upload FAIL!'));
+
+ // Check that the correct hooks were called.
+ $this->assertFileHooksCalled(['validate']);
}
/**
diff --git a/core/modules/file/tests/src/Kernel/ValidateTest.php b/core/modules/file/tests/src/Kernel/ValidateTest.php
index 3e9eb07a8ae5..f1547d62efaa 100644
--- a/core/modules/file/tests/src/Kernel/ValidateTest.php
+++ b/core/modules/file/tests/src/Kernel/ValidateTest.php
@@ -35,4 +35,23 @@ public function testCallerValidation() {
$this->assertFileHooksCalled(['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, []);
+ $this->assertEquals('For security reasons, your upload has been rejected.', $errors[0]);
+ $this->assertFileHooksCalled(['validate']);
+ file_test_reset();
+
+ // Test that the 'allow_insecure_uploads' is respected.
+ $this->config('system.file')->set('allow_insecure_uploads', TRUE)->save();
+ $errors = file_validate($file, []);
+ $this->assertEmpty($errors);
+ $this->assertFileHooksCalled(['validate']);
+ }
+
}
diff --git a/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
index 8ebeaf870df8..a887aa7c96b5 100644
--- a/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
+++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
@@ -386,26 +386,42 @@ protected function validate(FileInterface $file, array $validators) {
* The prepared/munged filename.
*/
protected function prepareFilename($filename, array &$validators) {
- if (!empty($validators['file_validate_extensions'][0])) {
- // If there is a file_validate_extensions validator and a list of
- // valid extensions, munge the filename to protect against possible
- // malicious extension hiding within an unknown file type. For example,
- // "filename.html.foo".
- $filename = file_munge_filename($filename, $validators['file_validate_extensions'][0]);
- }
-
- // 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 (!$this->systemFileConfig->get('allow_insecure_uploads') && preg_match(FILE_INSECURE_EXTENSION_REGEX, $filename) && (substr($filename, -4) != '.txt')) {
- // The destination filename will also later be used to create the URI.
- $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.
+ // Don't rename if 'allow_insecure_uploads' evaluates to TRUE.
+ if (!$this->systemFileConfig->get('allow_insecure_uploads')) {
if (!empty($validators['file_validate_extensions'][0])) {
- $validators['file_validate_extensions'][0] .= ' txt';
+ // If there is a file_validate_extensions validator and a list of
+ // valid extensions, munge the filename to protect against possible
+ // malicious extension hiding within an unknown file type. For example,
+ // "filename.html.foo".
+ $filename = file_munge_filename($filename, $validators['file_validate_extensions'][0]);
+ }
+
+ // 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).
+ if (preg_match(FILE_INSECURE_EXTENSION_REGEX, $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.
+ $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]));
+ }
+ if (empty($validators['file_validate_extensions'][0]) || $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 211257efff5e..b63c030c6e52 100644
--- a/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php
+++ b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php
@@ -621,11 +621,11 @@ public function testFileUploadMaliciousExtension() {
$response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example.php"']);
// The filename is not munged because .txt is added and it is a known
// extension to apache.
- $expected = $this->getExpectedDocument(1, 'example.php.txt', TRUE);
+ $expected = $this->getExpectedDocument(1, 'example.php_.txt', TRUE);
// Override the expected filesize.
$expected['data']['attributes']['filesize'] = strlen($php_string);
$this->assertResponseData($expected, $response);
- $this->assertFileExists('public://foobar/example.php.txt');
+ $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
@@ -635,11 +635,11 @@ public function testFileUploadMaliciousExtension() {
$this->rebuildAll();
$response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_2.php"']);
- $expected = $this->getExpectedDocument(2, 'example_2.php.txt', TRUE);
+ $expected = $this->getExpectedDocument(2, 'example_2.php_.txt', TRUE);
// Override the expected filesize.
$expected['data']['attributes']['filesize'] = strlen($php_string);
$this->assertResponseData($expected, $response);
- $this->assertFileExists('public://foobar/example_2.php.txt');
+ $this->assertFileExists('public://foobar/example_2.php_.txt');
$this->assertFileNotExists('public://foobar/example_2.php');
// Allow .doc file uploads and ensure even a mis-configured apache will not
@@ -659,6 +659,45 @@ public function testFileUploadMaliciousExtension() {
$this->assertFileExists('public://foobar/example_3.php_.doc');
$this->assertFileNotExists('public://foobar/example_3.php.doc');
+ // Test that a dangerous extension such as .php is munged even if it is in
+ // the list of allowed extensions.
+ $this->field->setSetting('file_extensions', 'doc php')->save();
+ $this->rebuildAll();
+
+ // Test using a masked exploit file.
+ $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_4.php.doc"']);
+ // The filename is munged.
+ $expected = $this->getExpectedDocument(4, 'example_4.php_.doc', TRUE);
+ // Override the expected filesize.
+ $expected['data']['attributes']['filesize'] = strlen($php_string);
+ // The file mime should be 'application/msword'.
+ $expected['data']['attributes']['filemime'] = 'application/msword';
+ $this->assertResponseData($expected, $response);
+ $this->assertFileExists('public://foobar/example_4.php_.doc');
+ $this->assertFileNotExists('public://foobar/example_4.php.doc');
+
+ // Dangerous extensions are munged even when all extensions are allowed.
+ $this->field->setSetting('file_extensions', '')->save();
+ $this->rebuildAll();
+ $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_5.php.png"']);
+ $expected = $this->getExpectedDocument(5, 'example_5.php_.png_.txt', TRUE);
+ // Override the expected filesize.
+ $expected['data']['attributes']['filesize'] = strlen($php_string);
+ // The file mime should also now be text.
+ $expected['data']['attributes']['filemime'] = 'text/plain';
+ $this->assertResponseData($expected, $response);
+ $this->assertFileExists('public://foobar/example_5.php_.png_.txt');
+
+ // Dangerous extensions are munged if is renamed to end in .txt.
+ $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_6.cgi.png.txt"']);
+ $expected = $this->getExpectedDocument(6, 'example_6.cgi_.png_.txt', TRUE);
+ // Override the expected filesize.
+ $expected['data']['attributes']['filesize'] = strlen($php_string);
+ // The file mime should also now be text.
+ $expected['data']['attributes']['filemime'] = 'text/plain';
+ $this->assertResponseData($expected, $response);
+ $this->assertFileExists('public://foobar/example_6.cgi_.png_.txt');
+
// Now allow insecure uploads.
\Drupal::configFactory()
->getEditable('system.file')
@@ -668,14 +707,14 @@ public function testFileUploadMaliciousExtension() {
$this->field->setSetting('file_extensions', '')->save();
$this->rebuildAll();
- $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_4.php"']);
- $expected = $this->getExpectedDocument(4, 'example_4.php', TRUE);
+ $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_7.php"']);
+ $expected = $this->getExpectedDocument(7, 'example_7.php', TRUE);
// Override the expected filesize.
$expected['data']['attributes']['filesize'] = strlen($php_string);
// The file mime should also now be PHP.
$expected['data']['attributes']['filemime'] = 'application/x-httpd-php';
$this->assertResponseData($expected, $response);
- $this->assertFileExists('public://foobar/example_4.php');
+ $this->assertFileExists('public://foobar/example_7.php');
}
/**
diff --git a/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php
index 619af887f5ba..a6355c827500 100644
--- a/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php
@@ -517,11 +517,11 @@ public function testFileUploadMaliciousExtension() {
$response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example.php"']);
// The filename is not munged because .txt is added and it is a known
// extension to apache.
- $expected = $this->getExpectedNormalizedEntity(1, 'example.php.txt', TRUE);
+ $expected = $this->getExpectedNormalizedEntity(1, 'example.php_.txt', TRUE);
// Override the expected filesize.
$expected['filesize'][0]['value'] = strlen($php_string);
$this->assertResponseData($expected, $response);
- $this->assertFileExists('public://foobar/example.php.txt');
+ $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
@@ -531,11 +531,11 @@ public function testFileUploadMaliciousExtension() {
$this->refreshTestStateAfterRestConfigChange();
$response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_2.php"']);
- $expected = $this->getExpectedNormalizedEntity(2, 'example_2.php.txt', TRUE);
+ $expected = $this->getExpectedNormalizedEntity(2, 'example_2.php_.txt', TRUE);
// Override the expected filesize.
$expected['filesize'][0]['value'] = strlen($php_string);
$this->assertResponseData($expected, $response);
- $this->assertFileExists('public://foobar/example_2.php.txt');
+ $this->assertFileExists('public://foobar/example_2.php_.txt');
$this->assertFileNotExists('public://foobar/example_2.php');
// Allow .doc file uploads and ensure even a mis-configured apache will not
@@ -555,6 +555,45 @@ public function testFileUploadMaliciousExtension() {
$this->assertFileExists('public://foobar/example_3.php_.doc');
$this->assertFileNotExists('public://foobar/example_3.php.doc');
+ // Test that a dangerous extension such as .php is munged even if it is in
+ // the list of allowed extensions.
+ $this->field->setSetting('file_extensions', 'doc php')->save();
+ $this->refreshTestStateAfterRestConfigChange();
+
+ // Test using a masked exploit file.
+ $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_4.php.doc"']);
+ // The filename is munged.
+ $expected = $this->getExpectedNormalizedEntity(4, 'example_4.php_.doc', TRUE);
+ // Override the expected filesize.
+ $expected['filesize'][0]['value'] = strlen($php_string);
+ // The file mime should be 'application/msword'.
+ $expected['filemime'][0]['value'] = 'application/msword';
+ $this->assertResponseData($expected, $response);
+ $this->assertFileExists('public://foobar/example_4.php_.doc');
+ $this->assertFileNotExists('public://foobar/example_4.php.doc');
+
+ // Dangerous extensions are munged even when all extensions are allowed.
+ $this->field->setSetting('file_extensions', '')->save();
+ $this->rebuildAll();
+ $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_5.php.png"']);
+ $expected = $this->getExpectedNormalizedEntity(5, 'example_5.php_.png_.txt', TRUE);
+ // Override the expected filesize.
+ $expected['filesize'][0]['value'] = strlen($php_string);
+ // The file mime should also now be text.
+ $expected['filemime'][0]['value'] = 'text/plain';
+ $this->assertResponseData($expected, $response);
+ $this->assertFileExists('public://foobar/example_5.php_.png_.txt');
+
+ // Dangerous extensions are munged if is renamed to end in .txt.
+ $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_6.cgi.png.txt"']);
+ $expected = $this->getExpectedNormalizedEntity(6, 'example_6.cgi_.png_.txt', TRUE);
+ // Override the expected filesize.
+ $expected['filesize'][0]['value'] = strlen($php_string);
+ // The file mime should also now be text.
+ $expected['filemime'][0]['value'] = 'text/plain';
+ $this->assertResponseData($expected, $response);
+ $this->assertFileExists('public://foobar/example_6.cgi_.png_.txt');
+
// Now allow insecure uploads.
\Drupal::configFactory()
->getEditable('system.file')
@@ -564,14 +603,14 @@ public function testFileUploadMaliciousExtension() {
$this->field->setSetting('file_extensions', '')->save();
$this->refreshTestStateAfterRestConfigChange();
- $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_4.php"']);
- $expected = $this->getExpectedNormalizedEntity(4, 'example_4.php', TRUE);
+ $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_7.php"']);
+ $expected = $this->getExpectedNormalizedEntity(7, 'example_7.php', TRUE);
// Override the expected filesize.
$expected['filesize'][0]['value'] = strlen($php_string);
// The file mime should also now be PHP.
$expected['filemime'][0]['value'] = 'application/x-httpd-php';
$this->assertResponseData($expected, $response);
- $this->assertFileExists('public://foobar/example_4.php');
+ $this->assertFileExists('public://foobar/example_7.php');
}
/**
diff --git a/core/tests/Drupal/KernelTests/Core/File/NameMungingTest.php b/core/tests/Drupal/KernelTests/Core/File/NameMungingTest.php
index 991f9263c9bd..26f54f97fcc1 100644
--- a/core/tests/Drupal/KernelTests/Core/File/NameMungingTest.php
+++ b/core/tests/Drupal/KernelTests/Core/File/NameMungingTest.php
@@ -7,28 +7,51 @@
/**
* Tests filename munging and unmunging.
*
+ * Checks performed, relying on 2 <= strlen('foo') <= 5.
+ *
+ * - testMunging()
+ * - (name).foo.txt -> (name).foo_.txt when allows_insecure_uploads === 0
+ * - testMungeBullByte()
+ * - (name).foo\0.txt -> (name).foo_.txt regardless of allows_insecure_uploads
+ * - testMungeIgnoreInsecure()
+ * - (name).foo.txt unmodified when allows_insecure_uploads === 1
+ * - testMungeIgnoreAllowedExtensions()
+ * - (name).FOO.txt -> (name).FOO when allowing 'foo'.
+ * - (name).foo.txt -> (name).foo.txt when allowing 'FOO'.
+ * - testMungeUnsafe()
+ * - (name).php.txt -> (name).php_.txt even when allowing 'php txt'
+ * - (name).php.txt -> (name).php_.txt even when allowing 'php txt'
+ * - testUnMunge()
+ * - (name).foo.txt -> (unchecked) -> (name).foo.txt after un-munging
+ *
* @group File
*/
class NameMungingTest extends FileTestBase {
/**
+ * An extension to be used as forbidden during munge operations.
+ *
* @var string
*/
protected $badExtension;
/**
+ * The name of a file with a bad extension, after munging.
+ *
* @var string
*/
protected $name;
/**
+ * The name of a file with an upper-cased bad extension, after munging.
+ *
* @var string
*/
protected $nameWithUcExt;
protected function setUp(): void {
parent::setUp();
- $this->badExtension = 'php';
+ $this->badExtension = 'foo';
$this->name = $this->randomMachineName() . '.' . $this->badExtension . '.txt';
$this->nameWithUcExt = $this->randomMachineName() . '.' . strtoupper($this->badExtension) . '.txt';
}
@@ -78,6 +101,18 @@ public function testMungeIgnoreAllowedExtensions() {
$this->assertSame($munged_name, $this->name);
}
+ /**
+ * Tests unsafe extensions are always munged by file_munge_filename().
+ */
+ public function testMungeUnsafe() {
+ $prefix = $this->randomMachineName();
+ $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->assertSame("$prefix.php_.txt", $munged_name);
+ }
+
/**
* Ensure that unmunge gets your name back.
*/
--
GitLab