Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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.
Patch to follow.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | file_entity-replace_file_extension_validation-2940978-18.patch | 6 KB | james.williams |
Comments
Comment #2
james.williams CreditAttribution: james.williams at ComputerMinds commentedComment #3
james.williams CreditAttribution: james.williams at ComputerMinds commentedI 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.
Comment #4
james.williams CreditAttribution: james.williams at ComputerMinds commentedThe 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.
Comment #5
joseph.olstadComment #8
Wim Leers… was this not a security vulnerability?
And why was this committed without any test coverage?
Comment #9
joseph.olstadNeither 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.
Comment #10
joseph.olstadProject page states clearly 'use at your own risk' not covered by security policy.
Comment #11
JonMcL CreditAttribution: JonMcL commentedI 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
FileEntityItemclass I am unable to replace the file with a new file that has an invalid extension.Comment #12
BerdirThis 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.
Comment #13
BerdirNote: The commit doesn't want to show up somehow, but i did revert this.
Comment #14
clairedesbois@gmail.comI 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.
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.
Comment #15
Berdir@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.
Comment #16
clairedesbois@gmail.comYes, 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.
Comment #17
clairedesbois@gmail.comI fix the problem with the installation of the module SVG Image
Comment #18
james.williams CreditAttribution: james.williams at ComputerMinds commentedThe 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'
Comment #19
james.williams CreditAttribution: james.williams at ComputerMinds commentedSorry, that last patch was empty!
Comment #20
james.williams CreditAttribution: james.williams at ComputerMinds commentedUmm... 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?