Problem/Motivation

When uploading a file using a file field, Drupal can add underscores to the file name as a security measure. When the file name contains periods followed by between 2 and 5 letters, Drupal considers that to be a possible extra file extension. Extra file extensions in the file name are made 'safe' by adding an underscore to the end of the extra extension.

File extensions which are considered 'safe' can be configured in the field 'Allowed file extensions' on the 'Manage field' page of the file field. When extra file extensions are in the allowed list, no extra underscore will be added to those file extensions.

Unfortunately the allowed extensions are stored in lower case. The check to see if extra file extensions are in the allowed list is case sensitive. Extra file extensions with capitals will always get an extra underscore, because they are compared with the list of allowed extensions which is lower case.

DETAILS
-------
includes\file.inc

Function file_save_upload() saves an upload and calls file_munge_filename() with the list of allowed extensions (which are lower case).
Function in_array() is used to compare the extra file extension in $filename_part (which can contain capitals) with the allowed extensions in $whitelist (which are lowercase). The check fails and an underscore is added.

REPRODUCE
---------
How to reproduce:
Create a content type.
Add a field of type 'file'.
Add 'test' and 'txt' to the 'Allowed file extensions'.
Create a node of the content type.
Upload file drupal.Test.txt. It will be renamed to drupal.Test_.txt.
Upload file drupal.test.txt. It is not renamed.
Upload file drupal.test.Txt. It is not renamed.

Proposed resolution

The real file extension of a file is checked in a case insensitive way. So in above example it does not matter if the real extension if txt or Txt.
Checking the file extension is done by the function file_validate_extensions(). That function can also be used to correctly check the extra file extensions in the file name. So in stead of using in_array(), we can use file_validate_extensions() to do the checks.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Add automated tests Novice Instructions
Manually test the patch Novice Instructions
Embed before and after screenshots in the issue summary Novice Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

API changes

CommentFileSizeAuthor
#77 interdiff.txt2.97 KBquietone
#77 tests_followup_valid-2112247-77.patch1.48 KBquietone
#69 tests_followup_valid-2112247-69.patch1.8 KBkostyashupenko
#65 interdiff-2112247-64-65.txt2.15 KBquietone
#65 file_extension_case_sensitivity-2112247-65.patch2.11 KBquietone
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,655 pass(es). View
#64 file_extension_case_sensitivity-2112247-64.patch1.06 KBKimNyholm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,939 pass(es). View
#41 after.png1.41 KBdcam
#41 before.png18.73 KBdcam
#40 file_extension_case_sensitivity-2112247-40.patch3.02 KBmitsuroseba
PASSED: [[SimpleTest]]: [MySQL] 41,034 pass(es). View
#34 file_extension_case_sensitivity-2112247-complete-3.patch3.07 KBsihv
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,223 pass(es). View
#34 file_extension_case_sensitivity-2112247-tests-3.patch1.96 KBsihv
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,996 pass(es), 2 fail(s), and 0 exception(s). View
#30 file_extension_case_sensitivity-2112247-complete-2.patch2.13 KBsihv
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,524 pass(es). View
#30 file_extension_case_sensitivity-2112247-tests-2.patch1.02 KBsihv
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,245 pass(es), 1 fail(s), and 0 exception(s). View
#27 fixed.jpg9.78 KBjoshjalopy
#26 issue fixed.jpg18.16 KBjoshjalopy
#23 file_extension_case_sensitivity-2112247-complete.patch2.09 KBsihv
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,916 pass(es). View
#23 file_extension_case_sensitivity-2112247-tests.patch993 bytessihv
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,901 pass(es), 1 fail(s), and 0 exception(s). View
#18 after.png27.21 KBmErilainen
#18 before.png42.59 KBmErilainen
#18 file_extension_case_sensitivity-2112247-18.patch1.12 KBmErilainen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,497 pass(es). View
#15 before.PNG5.26 KBdcam
#9 file_extension_case_sensitivity-2112247-9.patch1.12 KBdgroene
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_extension_case_sensitivity-2112247-9.patch. Unable to apply patch. See the log in the details link for more information. View
#3 2112247.patch1.38 KBaalamaki
PASSED: [[SimpleTest]]: [MySQL] 40,736 pass(es). View
file.inc_.patch1.12 KBDennis Walgaard
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in file.inc_.patch. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

johnv’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, file.inc_.patch, failed testing.

aalamaki’s picture

FileSize
1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 40,736 pass(es). View

The same issue persists with drupal 7.26, the following will correct the issue.

aalamaki’s picture

Status: Needs work » Needs review
aalamaki’s picture

Version: 7.23 » 7.26

Status: Needs review » Needs work

The last submitted patch, 3: 2112247.patch, failed testing.

jaredsmith’s picture

Issue tags: +sprint, +Needs reroll, +Novice
dgroene’s picture

Working on this issue locally

dgroene’s picture

FileSize
1.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_extension_case_sensitivity-2112247-9.patch. Unable to apply patch. See the log in the details link for more information. View

Changes both whitelist and filename_part to lowercase before comparing them

JacobSanford’s picture

Status: Needs work » Needs review

Looks like patch applies cleanly, triggering testbot

Status: Needs review » Needs work

The last submitted patch, 9: file_extension_case_sensitivity-2112247-9.patch, failed testing.

JacobSanford’s picture

Looks like @dgroene made the same mistake creating the patch I did checking the patch. It applies cleanly against 8.x :) Needs 7 port.

JacobSanford’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Restesting patch by #3, @aalamaki which applies cleanly to 7.26 as he stated! It appears to have failed due to the version number set at the time of submission. This didn't need reroll AFAICT.

JacobSanford’s picture

3: 2112247.patch queued for re-testing.

dcam’s picture

Version: 7.26 » 8.x-dev
Status: Needs review » Needs work
Issue tags: -sprint +needs backport to D7
FileSize
5.26 KB

This is still an issue in D8. See the attached image. It will have to be fixed for D8 first.

dcam’s picture

Issue summary: View changes
dcam’s picture

Issue tags: +Needs tests
mErilainen’s picture

Status: Needs work » Needs review
FileSize
1.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,497 pass(es). View
42.59 KB
27.21 KB

Rerolled (offset -1 lines) patch from #9 for the test bot now that the correct version is active. Seems to work.
Added before and after images.

dcam’s picture

Status: Needs review » Needs work

Setting as Needs Work because it still needs tests.

er.pushpinderrana’s picture

David, Could you please share what other test cases are required that need to be reviewed. As #18 patch look fine, tested manually.

dcam’s picture

Issue summary: View changes

Whenever we fix an issue like this we also add an automated test, which helps to ensure that future changes do not revert our hard work and cause the bug to reoccur. Here are the instructions for writing tests.

sihv’s picture

Assigned: Unassigned » sihv

Working on the tests.

sihv’s picture

Assigned: sihv » Unassigned
Status: Needs work » Needs review
FileSize
993 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,901 pass(es), 1 fail(s), and 0 exception(s). View
2.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,916 pass(es). View

The last submitted patch, 23: file_extension_case_sensitivity-2112247-tests.patch, failed testing.

dcam’s picture

I can't do a proper review of the patch right now, but I like it so far. The only thing I'm wondering about at this time is whether we should include a comment about why we have to normalize the extensions to lowercase.

joshjalopy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
18.16 KB

Patch #23 fixed the problem for Drupal 8.x-dev using Firefox 30.0 on Ubuntu. Drupal no longer renames approved extensions in upper or lower case.

joshjalopy’s picture

FileSize
9.78 KB

screen shot of acceptable file.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/file/src/Tests/ValidatorTest.php
@@ -45,6 +45,15 @@ function testFileValidateExtensions() {
+  function testFileMungeFilename() {
+    $this->assertNotEqual('file.foo.txt', file_munge_filename('file.foo.txt', 'asdf txt pork', FALSE), 'Altered invalid extra extension.', 'File');
+    $this->assertEqual('file.asdf.txt', file_munge_filename('file.asdf.txt', 'asdf txt pork', FALSE), 'Accepted valid same case extra extension.', 'File');
+    $this->assertEqual('file.Asdf.txt', file_munge_filename('file.Asdf.txt', 'asdf txt pork', FALSE), 'Accepted valid different case extra extension.', 'File');
+  }

This test belongs in NameMungingTest and the first assertNotEqual is not necessary as it is already tested. And actually so is the second test. You could adapt testMungeIgnoreWhitelisted() to test this to.

sihv’s picture

@alexpott: Thanks for the feedback, I'll do the changes.

sihv’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,245 pass(es), 1 fail(s), and 0 exception(s). View
2.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,524 pass(es). View

The last submitted patch, 30: file_extension_case_sensitivity-2112247-tests-2.patch, failed testing.

dcam’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/src/Tests/File/NameMungingTest.php
@@ -60,8 +60,8 @@ function testMungeIgnoreInsecure() {
+    // Declare our extension as whitelisted. The declared extensions should be case insensitive so test using one with a different case.

This comment needs to wrap at 80 characters.

I think we need to test this differently. Right now you're making the allowed extension uppercase, not the extension in the file name. It may not make any difference, but I think we ought to test against potential user input instead. A new $name with the uppercase extension should be created in the method to test.

Of course, another valid argument is that the allowed file extension is also user input and we should test to make certain it's normalized too. In which case your assertion should stay and my suggestion should be added as a second assertion. The only reason I know of why we might not care is that core file fields already normalize allowed extensions to lowercase when their settings are saved to the database, but that might not be the case forever or with custom file fields.

sihv’s picture

Thanks, I'll just add another assertion.

sihv’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,996 pass(es), 2 fail(s), and 0 exception(s). View
3.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,223 pass(es). View

Here are the updated patches.

The last submitted patch, 34: file_extension_case_sensitivity-2112247-tests-3.patch, failed testing.

benjf’s picture

Status: Needs review » Reviewed & tested by the community

I followed the steps in the issue summary both before and after applying the patch in #34, and this patch looks to solve the stated problem. Marking as RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed 1ddfac8 and pushed to 8.x. Thanks!

  • alexpott committed 1ddfac8 on 8.x
    Issue #2112247 by sihv, dgroene, aalamaki, Dennis Walgaard, mErilainen:...
mitsuroseba’s picture

Assigned: Unassigned » mitsuroseba

I'm working on the backport.

mitsuroseba’s picture

Version: 8.0.x-dev » 7.x-dev
Assigned: mitsuroseba » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 41,034 pass(es). View
dcam’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
18.73 KB
1.41 KB

Thank you, @mitsuroseba!

#40 is RTBC. Rather than making @mitsuroseba create a tests-only patch, I just ran the tests myself without the fix. The results are shown in the image below. As you can see, the new assertions failed.

After applying the patch a valid file extension with a different case did not have the underscore added.

m1r1k’s picture

Issue tags: +#ams2014contest

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: file_extension_case_sensitivity-2112247-40.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: file_extension_case_sensitivity-2112247-40.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: file_extension_case_sensitivity-2112247-40.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: file_extension_case_sensitivity-2112247-40.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: file_extension_case_sensitivity-2112247-40.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community
David_Rothstein’s picture

Title: Valid file extensions in file names are not properly enforced when uploading files » (Tests followup) Valid file extensions in file names are not properly enforced when uploading files
Version: 7.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Needs work

Committed to 7.x - thanks!

However, I don't understand why this patch removed one of the existing tests?

Before the patch it tested an example like this:

file_munge_filename('something.php.txt', 'php');

After the patch it tested two examples like this:

file_munge_filename('something.PHP.txt', 'php');
file_munge_filename('something.php.txt', 'PHP');

I think we should have a quick followup to add the first test back in addition to the new ones, especially since it's the most common case.

  • David_Rothstein committed c401ec3 on 7.x
    Issue #2112247 by sihv, mitsuroseba, dgroene, aalamaki, Dennis Walgaard...
nlisgo’s picture

Assigned: Unassigned » nlisgo
nlisgo’s picture

Assigned: nlisgo » Unassigned
disasm’s picture

tagging as needs tests to add the test back in.

KimNyholm’s picture

Assigned: Unassigned » KimNyholm
KimNyholm’s picture

Assigned: KimNyholm » Unassigned
Status: Needs work » Needs review
FileSize
1.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,939 pass(es). View

I have added the testcase requested in #58 and tested in 8.0.0-beta11.

quietone’s picture

Issue tags: -Needs tests
FileSize
2.11 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,655 pass(es). View
2.15 KB

@KimNyholm, thanks for adding the test back in.

Made some minor changes to the displayed text and moved the re-added test after the upper case test, because the test results were easier to follow (at least for me).

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Issue tags: +Needs reroll

A test-only change to 8.1.x is allowable.

The patch doesn't apply though.

Mile23’s picture

Status: Needs review » Needs work
kostyashupenko’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.8 KB

Rerolled #65 with auto-merge

dcam’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/src/Tests/File/NameMungingTest.php
@@ -66,10 +66,14 @@ function testMungeIgnoreInsecure() {
-    // Declare our extension as whitelisted. The declared extensions should
-    // be case insensitive so test using one with a different case.
+    // Declare our extension as whitelisted.
+    // The declared extensions should be case insensitive.
+    // Test using one with a different case.

Thank you for the patches!

Unfortunately, this change to the comment is unrelated to the issue and should be removed. If it really enhanced the clarity of what's going on in this section of code then I might not care, but it doesn't in my opinion. Furthermore, it violates our documentation standards. Sentences within a single comment should be separated by spaces, not put on new lines.

  • alexpott committed 1ddfac8 on 8.3.x
    Issue #2112247 by sihv, dgroene, aalamaki, Dennis Walgaard, mErilainen:...

  • alexpott committed 1ddfac8 on 8.3.x
    Issue #2112247 by sihv, dgroene, aalamaki, Dennis Walgaard, mErilainen:...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • alexpott committed 1ddfac8 on 8.4.x
    Issue #2112247 by sihv, dgroene, aalamaki, Dennis Walgaard, mErilainen:...

  • alexpott committed 1ddfac8 on 8.4.x
    Issue #2112247 by sihv, dgroene, aalamaki, Dennis Walgaard, mErilainen:...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
2.97 KB

Fixed the comment.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.