Problem/Motivation
Symfony providers two mime guessers FileinfoMimeTypeGuesser
and FileBinaryMimeTypeGuesser
which do a better and more secure job than ExtensionMimeTypeGuesser.
If either are supported we should add them to Drupal\Core\File\MimeType\MimeTypeGuesser
with a preference for FileinfoMimeTypeGuesser
.
Beta phase evaluation
Issue priority | Major because using fileinfo or the file command represents a security improvement over the extension based guessing. |
---|---|
Prioritized changes | The main goal of this issue is security. |
Disruption | Whilst this removes a hook from core this hook is only used by the File Entity module in Drupal 7 (Looking at http://drupalcontrib.org/api/drupal/drupal%21modules%21system%21system.a...). All the additional maapins it adds are in Drupal 8 so this is not necessary since #1443070: Add support for popular e-book formats, Google web formats, mkv and mka in file_default_mimetype_mapping() landed. |
Proposed resolution
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Fix file upload when new MIME type guessers are enabled | See details in #2388749-72: Register symfony's mime guessers if they are supported | ||
Reroll the patch if it no longer applies. | Instructions |
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff-2388749-52-54.txt | 1.28 KB | BR0kEN |
#54 | 2388749-54.patch | 13.22 KB | BR0kEN |
Comments
Comment #1
alexpottComment #3
dawehnerduplicate key in the
core.services.yml
file.Comment #4
alexpottdoh.
Comment #5
dawehnerLooks perfect from my perspective
Comment #7
alexpottlolz - c&p fail
Comment #9
alexpottSo
file_get_mimetype
used to accept just a filename and not a complete path. Since we have moved to Symfony's MimeTypeGuesserInterface we should completely remove this function as it is not only deprecated it's documentation is misleading and incorrect - you can no longer add mappings.Comment #10
alexpottThe original issue to move to Symfony's mime guessing #1921558: Convert file_get_mimetype() to use Symfony MimeTypeGuessers left the ability to alter mappings and set mappings. This does not make sense. If a module needs to add additional guessers it should just add a new service tagged with
mime_type_guesser
. This patch removeshook_file_mimetype_mapping_alter()
.Comment #11
alexpottComment #12
alexpottMissing a new line
Comment #14
alexpott#2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser is related. The expectation to be able to list all the supported mime types is interesting. D8 has not had this ability for a while.
Comment #15
dawehnerIn an ideal world, we would ensure here that we care about the order. http://docs.mockery.io/en/latest/reference/expectations.html allows this really nicely to do.
Comment #16
alexpottThe order is tested. The test adds one guessers and makes an assertion. Then is adds another guesser and makes an assertion on the same file path but the new guesser (higher priority) changes the expected result.
Comment #19
alexpottSome testbot weirdness... back to rtbc as per #15
Comment #21
YesCT CreditAttribution: YesCT commentedComment #22
adci_contributor CreditAttribution: adci_contributor commentedI'm trying to reroll.
Please review
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedi agree with this, but it needs a change record
other than that its good to go
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedAh, seems #22 misses ExtensionMimeTypeGuesserTest from #12
Comment #25
adci_contributor CreditAttribution: adci_contributor commentedSorry, I missed an untracked file.
ExtensionMimeTypeGuesserTest returned in a patch.
And rerolled core.services.yml after #2379741: Add Renderer::getCacheableRenderArray() to encapsulate which data is needed for caching a render array and have views use it
Comment #26
YesCT CreditAttribution: YesCT commented@adci_contributor
An interdiff between #22 and #25 would make it more likely you will get a review faster.
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff
Comment #27
adci_contributor CreditAttribution: adci_contributor commentedI rerolled #22 and returned ExtensionMimeTypeGuesserTest in the same patch, so i could not make interdiff between #22(which doesn't apply) and #25.
I guess we can just ignore #22, because #25 is actually a correct reroll of #12. Sorry again for this confusion.
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commentedI updated https://www.drupal.org/node/2258015
Comment #30
ParisLiakos CreditAttribution: ParisLiakos commentedjust a reroll
Comment #31
catchCommitted/pushed to 8.0.x, thanks!
Comment #33
Dave ReidThis is missing a change notice for how to replace hook_file_mimetype_mapping_alter() for all conditions of adding, removing, and altering existing extension mappings. I don't agree with the removal of that hook either because it was also used to *fix* invalid mappings in core when it happened (which it did in Drupal 7's lifetime).
Comment #34
Dave ReidOk I see the change record mentions the hook was removed (which I don't think all use cases were fully justified in this issue before it was thrown out), but the examples need to be expanded on how to cover what hook_file_mimetype_mapping_alter() was used for before, rather than incredibly generic examples.
Comment #35
alexpottThe issue fixing invalid mappings will rarely be Drupal's responsibility in the future. The problem with the alter hook is that it makes it impossible to use the better and more secure services for mimetype guessing because how can we alter the response from PHP's fileinfo extension or the system file command. The whole point is that extensions are not a great way of determining a mimetype. In Drupal 7 there was only 1 module that used the hook to add new mappings - http://drupalcontrib.org/api/drupal/drupal%21modules%21system%21system.a...
If you want to an add additional mime type guesser or replace Drupal 8's extension mime guesser service that is entirely possible but I don't think it is the role of the CR to go into more detail than it does - we have enough trouble keeping CRs up-to-date with valid code samples for changes that affect 100's of modules. The existing CR details how to add a new guesser but it does not delve into the implementation because that is not its responsibility.
I think this issue should be reverted to major and fixed but I will wait for @Dave Reid to detail any other concerns.
Comment #36
Dave ReidI don't see at all how supporting the alter hook makes it impossible to use the better and more secure services, especially after reading the patch that was committed.
Comment #37
alexpottBecause the alter hook would only affect the FIeExtensionGuesser - and so would only be used if you fallback to that guesser. Therefore, in order to guarantee that your alter is being used then you need to either remove the other mime guessing services or add a new service with a higher priority. In all ways you have to change services - so why not just add a new service.
Comment #38
Dave ReidMy assumption is that the alter hook only affects data for that specific service, not that it would automatically be used in favor of the other services. I say that as the person who's written the only module that implements this hook in D7.
Comment #39
mondrakeMy 2 cents - can we think of adding something similar in #2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser, but as a method of the
MimeTypeMapper
service rather than a hook? AFAICS the point here is not about determining the mime type of a 'real' file (the extension based guessing is becoming vestigial after this issue, there are two more prioritised guessers that take precedence), but rather about determining the mime type of a 'virtual' file based on its extension? And the mapping extension<->mimetype built-in in Drupal is a valuable asset that can be used viaMimeTypeMapper
methods, not guessers, introduced by the other issue.Comment #40
alexpottAlso it is about the expectations of having the hook.
This is true if you only think about extension guessing but on many sites now that we are using the better and more secure services this just won't perform as the developer expects since the extension guesser is (correctly) the lowest priority. If a developer really wants to change the mimetype for all files ending with txt then they are going to have to register a new mime type guesser.
I would also argue that we should just be better at adding mime types to core - comparing Symfony's
MimeTypeExtensionGuesser
with ours - shows we've got a lot to add. We should have a commitment to keep up-to-date with https://svn.apache.org/repos/asf/httpd/httpd/trunk/docs/conf/mime.types if we want to have this array in core.I'm going to reset the issue status to fixed - any further discussions about the need for a hook can take place on #2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser
Comment #41
mfb(Setting back to needs work because of major security vulnerability)
I'm very unclear why analyzing a file to determine its mimetype is considered "secure". To me, it's always been safer to assign the mime type based on file extension. The site will whitelist file extensions for a file upload field, and because each extension maps to a mime type, it's also whitelisting mime types. So it could easily prevent everything from malware to HTML from being uploaded.
Currently in Drupal 8, if I create a file field and allow .txt files to be uploaded, then create a file that starts with
<?php
and upload it, Drupal saves this as a text/x-php file. That seems sketchy, but maybe not easy to exploit.Let's try another one, if I create a file that starts with
<html>
and upload it, Drupal saves it as a text/html file. When downloaded from the private filesystem, this file is downloaded as text/html and interpreted by the browser as HTML, which could contain javascript - this is a nice fat XSS vulnerability!Aside from these simple examples, it's well-known that malicious files can be constructed that could be interpreted as a valid file of multiple different types. I didn't yet try these w/ Drupal 8.
BTW, see https://www.drupal.org/project/filemime for a drupal 7 contrib module that allows altering the built-in mime/extension mapping. I was planning to upgrade this to d8 at some point.
Comment #43
catchThanks mfb. I have a vague memory of us discussing this before a long time ago, and it may have been one of the original reasons we stuck to extension-based mime type guessing for so long. Would have been better to have remembered this before committing the patch...
Rolled this back for now.
We should:
1. Have a test for the file uploads/file downloads issue to avoid regressing.
2. Decide whether this problem is specific to file uploads (where we could possibly restrict to the extension guesser) or whether generally it's better not to do this.
Comment #44
alexpott@mfb nice catch. Totally agree with rolling back here. I'm going to unpublish the CR.
Comment #45
alexpottSo given that we've obviously not achieved the desired benefits to security I think we should postpone this issue until we can. There are two parts to this. Firstly, determining what mimetype a file has. Secondly, determining what to allow to be uploaded. In HEAD these are linked because we use an extension based guesser. We then serve the file with guessed mimetype which protects us from the "nice fat XSS vulnerability".
I think we should postpone this, the amount of work to change file uploads to limit by actual mimetype and not extension is significant and unnecessarily risky and disruptive.
Any work on this patch need to add a test based on the security hole outlined by @mfb in #41
Comment #46
mfbI see three things to consider:
Comment #52
BR0kENRe-roll for 8.3.x with additional tests.
Comment #54
BR0kENForgot to add new files.
Comment #55
BR0kENAlso, as @mfb, a long time ago I've made a separate module - https://www.drupal.org/project/mimeinfo - which meets the needs. In case of 7.x it allows optionally override stream wrapper for checking the MIME type, 8.x - defines couple guessers from Symfony package and overrides
addGuesser
method offile.mime_type.guesser
service to check that guesser supported (most part of last patch is there, in 8.x).Comment #67
PieterDCWow, this is an old issue.
But it got relevant to me today because I have to guess the mimetype of files that don't have extensions so I can judge whether that file is allowed or not.
Thanks for sharing your knowledge.
The patch from comment #54 still applies. I tried it on Drupal 8.9.x and it does what the issue description says.
But it does also break regular file upload functionality like with a file field on a node edit form.
I could cover my use case without that patch, but directly using the Symfony Guesser class and applying #3156672-6: ExtensionMimeTypeGuesser breaks other mime_type_guesser services to let the guesser based on file extension fall back to the other guessers.
Comment #72
mxr576Extending PieterDC's thoughts in #67
It also breaks file upload via JSONAPI - this is how I found this ancient one... - because also passes file name only here when \Symfony\Component\Mime\FileinfoMimeTypeGuesser::guessMimeType() needs a valid path. So the root cause of the issue is pretty much the same. (See also \Drupal\file\Upload\FileUploadHandler::handleFileUpload()
Comment #73
mxr576