Problem/Motivation

It's possible to change the file extension of the file you are allowed to upload by renaming a file.

This allows you to do something weird like uploading a '.zip' file that then gets renamed to a '.jpg' file or vice-versa. It only affects the validation of the uploaded file name, when the file is actually being replaced on disk, the original file extension is preserved.

Steps to reproduce

1) Upload a file and save it.
2) Edit the file (e.g. at /file/###/edit), and change the 'Filename' of the file, to something more reader-friendly, specifically without the file extension. Save the file.
3) Return to edit the file, try to replace the file -- the file extension is not validated correctly.

This is most noticeable when dealing with a file that is not in Drupal's default list of allowed extensions (e.g. a zip file), because the validation fails, with the message 'Only files with the following extensions are allowed', followed by that default list. Instead, the extension should match whatever the replaced file had.

This is happening because the only allowed extension is being parsed out of the filename property, rather than the file URI. That would be fine if the filename wasn't editable... but it is.

Proposed resolution

Tweak the code that builds the form element to correct read the file extension from the existing uploaded file path.

Remaining tasks

User interface changes

None

Introduced terminology

None

API changes

None

Data model changes

None.

CommentFileSizeAuthor
#22 file_entity-replace_file_extension_validation-2940978-22.patch682 bytessteven jones
#19 file_entity-replace_file_extension_validation-2940978-18.patch6 KBjames.williams
#18 interdiff-2940978-4-18.txt1014 bytesjames.williams
#18 file_entity-replace_file_extension_validation-2940978-18.patch0 bytesjames.williams
#14 Capture d’écran 2018-04-27 à 12.03.58.png22.61 KBclairedesbois@gmail.com
#4 file_entity-replace_file_extension_validation-2940978-4.patch5.14 KBjames.williams
#2 file_entity-replace_file_extension_validation-2940978-2.patch703 bytesjames.williams

Issue fork file_entity-2940978

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

james.williams created an issue. See original summary.

james.williams’s picture

james.williams
Location Coventry, UK
commented
Status: Active » Needs review
StatusFileSize
new file_entity-replace_file_extension_validation-2940978-2.patch703 bytes
james.williams’s picture

james.williams
Location Coventry, UK
commented

I should point out that I expect a user could actually do the following at the moment (without the patch) to circumvent correct validation:

1) Upload allowed file (e.g. something.txt)
2) Edit file, renaming it (i.e. changing the filename text field) to 'something.bad', save
3) Edit file, replacing the file with 'terriblefile.bad'

I would guess that validation would now pass, when '.bad' really shouldn't be an allowed file extension!

The patch would ensure that only a '.txt' file could be used to replace the file.

james.williams’s picture

james.williams
Location Coventry, UK
commented
StatusFileSize
new file_entity-replace_file_extension_validation-2940978-4.patch5.14 KB

The previous patch didn't go far enough, as core's extension validation would still kick in later, when on a node edit form that contained a file field, that could have its file replaced (e.g. via an entity browser widget). This one does.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

… was this not a security vulnerability?

And why was this committed without any test coverage?

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Neither the 8.x-2.x nor the 7.x-2.x branches of this project are covered by the Drupal security team.

Feel free to make it so.

Also, correct me if I am wrong, I believe that the 8.x branch of this project is superceded by something that was added to 8.4.x core (media entity?)

Currently, my clients pay me handsomely to do 7.x. However, any 8.x work I do is charity work unpaid.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Project page states clearly 'use at your own risk' not covered by security policy.

jonmcl’s picture

jonmcl commented

I am finding that this patch, already committed, is causing problems.

I created a new issue: #2951653: Default file extensions are being used for validation during file upload.

By the way, I'm not so sure any of this code is preventing the filename from being renamed into a string with an invalid extension. At least when I do my tests, changing the filename field in the /file/{fid}/edit page allows a bad extension to be accepted. With or without the the addition of the FileEntityItem class I am unable to replace the file with a new file that has an invalid extension.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Closed (fixed) » Active

This should not have been committed.

As #11 and the referenced issue shows, this seems to be causing problems and it is unclear if the problem is really fixed.

More importantly, this change causes the tests to fail, which was not noticed because the old test configuration is no longer supported and patches have not been tested automatically.

Whatever we do exactly will at least need to pass tests and should have test coverage for the bug it fixes.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Note: The commit doesn't want to show up somehow, but i did revert this.

clairedesbois@gmail.com’s picture

clairedesbois@gmail.com
she/her
Primary language French
Location Montreal
commented
StatusFileSize
new Capture d’écran 2018-04-27 à 12.03.58.png22.61 KB

I don't know if it's related but we're no longer able to upload SVG file since the update to Drupal 8.5.x. We had used the core patch to add SVG which worked well. The patch was improved and commit in the core and now, when we try to upload SVG file, we are block to the validation.

Printscreen bug

When I study the bug with xdebug, I see in file_validate_is_image of file.module than the validation function isn't able to find the mimetype and the temporary file doesn't have extension. So, the code never find the type of the file.

I've some difficulties to understand what's the part of the core and the part of the file_entity module in this bug.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

@Calystod: This is file_entity and not the core file module. Maybe you wanted to post that in core?

As mentioned, the patch that was committed here has been reverted again and is not in the latest version.

clairedesbois@gmail.com’s picture

clairedesbois@gmail.com
she/her
Primary language French
Location Montreal
commented

Yes, when I look with xdebug, the file is a file_entity but is validated with core functions. So I don't know if the file_entity in miscontruct or if something is missing in core functions.

I see the patch has been revert, but my validation problem seems to look like the initial bug, even if I don't know if they are related in the code.

Sorry if my posts seem confuse.

clairedesbois@gmail.com’s picture

clairedesbois@gmail.com
she/her
Primary language French
Location Montreal
commented

I fix the problem with the installation of the module SVG Image

james.williams’s picture

james.williams
Location Coventry, UK
commented
StatusFileSize
new file_entity-replace_file_extension_validation-2940978-18.patch0 bytes
new interdiff-2940978-4-18.txt1014 bytes

The previous patches don't work on the Add file form at /file/add, and needed re-rolling. Here's an update.

I'll investigate the issues that the original patches caused, but please note the steps to reproduce the issue, as described earlier in the issue:

1) Upload allowed file (e.g. something.txt)
2) Edit file, simply retitling it (i.e. changing the filename text field, not the name of the actual file-on-disk itself) to 'something.bad', save that.
3) Edit file, replacing the file with 'terriblefile.bad'

james.williams’s picture

james.williams
Location Coventry, UK
commented
StatusFileSize
new file_entity-replace_file_extension_validation-2940978-18.patch6 KB

Sorry, that last patch was empty!

james.williams’s picture

james.williams
Location Coventry, UK
commented
Status: Active » Needs work
Issue tags: +Needs tests

Umm... this feels rather close to SA-CONTRIB-2024-001 . The D8+ versions might not have official security coverage, but it might be prudent for them to get fixed too, especially with the attention that SA will have brought to this project?

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Patch #19 no longer applies. The validator api has changed and deprecated approach was removed from Drupal 11. The whole scenario needs to be re-tested with the latest tagged release on Drupal 11 to see if this issue is still relevant.

steven jones’s picture

steven jones commented
Issue summary: View changes
StatusFileSize
new file_entity-replace_file_extension_validation-2940978-22.patch682 bytes

This issue was discussed in private with the security team via the responsible reporting process and it was decided that the bugfix for the remaining issue could be discussed in public.

The relevant copy from the security issue is basically these steps for a reproduction:

So there's still a small bug with the module that allows you to do something like this:

  1. Upload a file, with an allowed file extension, like say .jpg I.e. I've now uploaded example.jpg to the public file directory.
  2. Using this module, you can then 'rename' the title of the file to something like: example.zip this changes nothing on disk
  3. Now you can use the replace file functionality provided by this module, which will now allow you to upload a file with the .zip extension
  4. However, this file replaces the existing file on disk, using the existing name, so the zip file will be renamed to 'example.jpg' as per the original file

This is all akin to uploading a file called example.jpg originally, that actually happens to be a Zip file.

I'm attaching a patch that resolves the bug. This issue needs tests (and a MR)

joseph.olstad opened merge request !34

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Needs work » Needs review
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Assigned: Unassigned » joseph.olstad
Issue tags: -Needs tests

  • joseph.olstad committed 968bfd06 on 8.x-2.x 
    fix: #2940978 File extension validation on replacing file uses wrong...
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Assigned: joseph.olstad » Unassigned
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

https://www.drupal.org/project/file_entity/releases/8.x-2.9

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.