From 1cd1830d79f221cc8490f53c2bb487dd07094f17 Mon Sep 17 00:00:00 2001 From: xjm Date: Wed, 20 Jul 2022 10:11:42 -0500 Subject: [PATCH] SA-CORE-2022-014 by elarlang, pwolanin, xjm, mcdruid, effulgentsia, greggles, jenlampton, larowlan, longwave --- lib/Drupal/Core/File/FileSystemInterface.php | 4 ++-- .../SecurityFileUploadEventSubscriber.php | 11 ++++++++++- .../Event/SecurityFileUploadEventSubscriberTest.php | 3 ++- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/lib/Drupal/Core/File/FileSystemInterface.php b/lib/Drupal/Core/File/FileSystemInterface.php index c1781b6944f..59d2ee37f0b 100644 --- a/lib/Drupal/Core/File/FileSystemInterface.php +++ b/lib/Drupal/Core/File/FileSystemInterface.php @@ -37,14 +37,14 @@ interface FileSystemInterface { * * @see \Drupal\Core\File\FileSystemInterface::INSECURE_EXTENSION_REGEX */ - public const INSECURE_EXTENSIONS = ['phar', 'php', 'pl', 'py', 'cgi', 'asp', 'js']; + public const INSECURE_EXTENSIONS = ['phar', 'php', 'pl', 'py', 'cgi', 'asp', 'js', 'htaccess']; /** * The regex pattern used when checking for insecure file types. * * @see \Drupal\Core\File\FileSystemInterface::INSECURE_EXTENSIONS */ - public const INSECURE_EXTENSION_REGEX = '/\.(phar|php|pl|py|cgi|asp|js)(\.|$)/i'; + public const INSECURE_EXTENSION_REGEX = '/\.(phar|php|pl|py|cgi|asp|js|htaccess)(\.|$)/i'; /** * Moves an uploaded file to a new location. diff --git a/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php b/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php index 4aeeb574d35..4bf03463d6d 100644 --- a/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php +++ b/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php @@ -63,6 +63,15 @@ public function sanitizeName(FileUploadSanitizeNameEvent $event): void { $filename = array_shift($filename_parts); // Remove final extension. $final_extension = (string) array_pop($filename_parts); + // Check if we're dealing with a dot file that is also an insecure extension + // e.g. .htaccess. In this scenario there is only one 'part' and the + // extension becomes the filename. We use the original filename from the + // event rather than the trimmed version above. + $insecure_uploads = $this->config->get('allow_insecure_uploads'); + if (!$insecure_uploads && $final_extension === '' && str_contains($event->getFilename(), '.') && in_array(strtolower($filename), FileSystemInterface::INSECURE_EXTENSIONS, TRUE)) { + $final_extension = $filename; + $filename = ''; + } $extensions = $event->getAllowedExtensions(); if (!empty($extensions) && !in_array(strtolower($final_extension), $extensions, TRUE)) { @@ -76,7 +85,7 @@ public function sanitizeName(FileUploadSanitizeNameEvent $event): void { return; } - if (!$this->config->get('allow_insecure_uploads') && in_array(strtolower($final_extension), FileSystemInterface::INSECURE_EXTENSIONS, TRUE)) { + if (!$insecure_uploads && in_array(strtolower($final_extension), FileSystemInterface::INSECURE_EXTENSIONS, TRUE)) { if (empty($extensions) || in_array('txt', $extensions, TRUE)) { // Add .txt to potentially executable files prior to munging to help prevent // exploits. This results in a filenames like filename.php being changed to diff --git a/modules/system/tests/src/Unit/Event/SecurityFileUploadEventSubscriberTest.php b/modules/system/tests/src/Unit/Event/SecurityFileUploadEventSubscriberTest.php index f51f750b42b..ffeaa0de152 100644 --- a/modules/system/tests/src/Unit/Event/SecurityFileUploadEventSubscriberTest.php +++ b/modules/system/tests/src/Unit/Event/SecurityFileUploadEventSubscriberTest.php @@ -84,7 +84,8 @@ public function provideFilenames() { 'filename is munged' => ['foo.phar.png.php.jpg', 'jpg png', 'foo.phar_.png_.php_.jpg'], 'filename is munged regardless of case' => ['FOO.pHAR.PNG.PhP.jpg', 'jpg png', 'FOO.pHAR_.PNG_.PhP_.jpg'], 'null bytes are removed' => ['foo' . chr(0) . '.txt' . chr(0), '', 'foo.txt'], - 'dot files are renamed' => ['.htaccess', '', 'htaccess'], + 'dot files are renamed' => ['.git', '', 'git'], + 'htaccess files are renamed even if allowed' => ['.htaccess', 'htaccess txt', '.htaccess_.txt', '.htaccess'], ]; }