When the code for checking mime types was removed from Drupal, a few lines seem to have been overlooked. These lines add the .txt extension to any file whose mime type has not been identified. As a result, Drupal is adding .txt to every upload, whether JPEG, HTML, PDF, or PNG. This makes uploaded files unusable.
I understand from issue 43220 that we are waiting for someone to come up with a mime type checking solution that reliably works. Until then, the overlooked lines should be removed as well. The attached patch removes them in the CVS and 4.7 branches.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | file-mime.patch | 704 bytes | darren oh |
| #1 | file-4.6.inc_0.patch | 789 bytes | darren oh |
| file-cvs.inc_0.patch | 841 bytes | darren oh |
Comments
Comment #1
darren ohPatch for 4.6 branch.
Comment #2
darren ohIt might be preferable to comment out the lines instead of removing them so they can be restored easily when a mime type checking solution is provided. A patch to do so is available in issue 74542.
Comment #3
beginner commentedCan you review this patch:
http://drupal.org/node/76154
Comment #4
beginner commentedComment #5
dries commentedI'd like to have Morbus' blessing for this one.
Comment #6
drummComment #7
morbus iff[00:32] <drumm> MorbusIff: around?
[00:32] <MorbusIff> not really.
[00:33] <MorbusIff> something quick?
[00:33] <drumm> MorbusIff: know about http://drupal.org/node/78807?
[00:33] <MorbusIff> no. i have to remember it.
[00:33] <drumm> MorbusIff: apparently your blessing is needed.
[00:33] <MorbusIff> i don't recall me actually having much to do with the mime removal, much past "i agree with chx".
[00:33] <MorbusIff> yes, i know. it's in my inbox.
[00:34] <MorbusIff> i'm quite busy nowadays, so it'll be a bit before i can give it some thought.
[00:34] <MorbusIff> i know that i'm certainly unable to duplicate it.
[00:34] <drumm> MorbusIff: forward it to chx if that is what is needed. and comment in the issue
[00:34] <MorbusIff> and that, thereotically, our newly added file munging would cover anything malicious that that doesn;t.
[00:34] <MorbusIff> but, again, i haven't given it much thought yet.
[00:36] <drumm> MorbusIff: thanks. Please post a follow up to the issue saying what you just did
Comment #8
Steven commentedWell, there are two checks in there... one is to make sure we don't upload files that the server will execute (e.g. .php, .asp, .pl, ...), the other part is to make sure no javascript files are snuck to the browser side without a .txt extension to prevent their execution.
The server-side check needs to stay in core.
The client-side check is now pretty useless because we depend on the honesty of the browser/client to submit the right mime type. Perhaps we could simply put ".js" in the list of files we add a .txt extension to?
Comment #9
darren ohI don't see the server-side check you refer to in the current code. Isn't that what was removed? And isn't it because it was removed that we now find .txt appended to every file name?
Comment #10
darren ohFound: mime type checking removed (partially) in CVS commit 26881.
Comment #11
dries commentedDarren: does that mean the code needs more work or?
Comment #12
darren ohWhat it means is that this patch is ready to be committed "until a better fix comes along." This patch completes the patch committed in CVS commit 26881.
Comment #13
Steven commentedif (((substr($file->filemime, 0, 5) == 'text/' || strpos($file->filemime, 'javascript')) && (substr($file->filename, -4) != '.txt')) || preg_match('/\.(php|pl|py|cgi|asp)$/i', $file->filename)) {
The bold part is the server-side check.
Comment #14
Steven commentedComment #15
darren ohTried to remove just the mime type part of the check and add .js to the list of file extensions. Please review.
Comment #16
darren ohI tested the patch on my site and it works as expected.
Comment #17
dries commentedCommitted to CVS HEAD. Thanks.
Comment #18
(not verified) commented