#39552 closed defect (bug) (fixed)
SVG upload support broken in 4.7.1
Reported by: | freakpants | Owned by: | joemcgill |
---|---|---|---|
Milestone: | 4.7.3 | Priority: | normal |
Severity: | normal | Version: | 4.8 |
Component: | Upload | Keywords: | needs-unit-tests has-patch |
Focuses: | Cc: |
Description
The added function wp_get_image_mime
<?php function wp_get_image_mime( $file ) { /* * Use exif_imagetype() to check the mimetype if available or fall back to * getimagesize() if exif isn't avaialbe. If either function throws an Exception * we assume the file could not be validated. */ try { if ( is_callable( 'exif_imagetype' ) ) { $mime = image_type_to_mime_type( exif_imagetype( $file ) ); } elseif ( function_exists( 'getimagesize' ) ) { $imagesize = getimagesize( $file ); $mime = ( isset( $imagesize['mime'] ) ) ? $imagesize['mime'] : false; } else { $mime = false; } } catch ( Exception $e ) { $mime = false; } return $mime; }
returns false for svg images.
This results in a security error when uploading svg images.
This is because neither exif-imagetype (http://php.net/manual/en/function.exif-imagetype.php) nor getimagesize() can correctly determine the svg mime type. (it is not one of the constants returned, and therefore just answers with false instead of a constant that would reference image/svg+xml.
Solution: Use finfo to also validate the svg mime type:
<?php $finfo = finfo_open( FILEINFO_MIME_TYPE ); $mime = finfo_file( $finfo, $file ); finfo_close( $finfo );
The breaking functionality was added in this commit:
https://github.com/WordPress/WordPress/commit/8eff9278234f473b66310f3013b96ac6441a20da
Attachments (8)
Change History (45)
#3
@
8 years ago
Note: wp_get_image_mime doesnt actually return false, but application/octet-stream.
#5
@
8 years ago
the elseIf case with getimagesize actually returns false. So the fix needs to check for false OR application/octet-stream.
#8
@
8 years ago
Thanks so much for working on this. I've been testing this, found that in my installation, PHP 5.6.24-1+deb.sury.org~trusty+1, the first part of wp_get_image_mime, the image_type_to_mime_type function sets the mime to "application/octet-stream" so in the part you added I changed the if to:
"if ( ( 0 === strpos($mime , "application/octet-stream") OR false === $mime) && function_exists( 'finfo_file' ) ) {"
Of course why that is happening I have no idea, doesn't match php docs, what version of php are you testing on?
#9
follow-up:
↓ 11
@
8 years ago
While not documented, application/octet-stream is the expected result when image_type_to_mime_type is called with something that isn't one of the constants:
default:
case IMAGE_FILETYPE_UNKNOWN:
return "application/octet-stream"; /* suppose binary format */
(as seen in https://github.com/php/php-src/blob/c8aa6f3a9a3d2c114d0c5e0c9fdd0a465dbb54a5/ext/standard/image.c)
In our specific case it is called with "false", since exif-imagetype cant detect svg.
This ticket was mentioned in Slack in #forums by zoonini. View the logs.
8 years ago
#11
in reply to:
↑ 9
@
8 years ago
Are you saying this patch is going to require php 7? In my case $mime is getting set to "application/octet-stream" right before the change on line 2393 so just checking that it's false isn't doing anything but returning false for svg files and still showing the security error.
Replying to freakpants:
While not documented, application/octet-stream is the expected result when image_type_to_mime_type is called with something that isn't one of the constants:
default: case IMAGE_FILETYPE_UNKNOWN: return "application/octet-stream"; /* suppose binary format */
(as seen in https://github.com/php/php-src/blob/c8aa6f3a9a3d2c114d0c5e0c9fdd0a465dbb54a5/ext/standard/image.c)
In our specific case it is called with "false", since exif-imagetype cant detect svg.
#12
follow-up:
↓ 18
@
8 years ago
No, sorry, it should work fine on PHP > 5.3. The patch @diddledan created doesn't work. He forgot to consider the applicaion/octet-stream return.
My patch works, but might not work on systems that dont have a magic file (?).
#14
follow-up:
↓ 15
@
8 years ago
Since (as I understand it) the core team has decided not to allow SVG uploads by default, be sure not to add SVG to the returned arrays of wp_get_mime_types() or wp_get_ext_types().
I think the best solution would take @freakpants' approach: it should be self-contained within the new wp_get_image_mime() function and just make that function capable of returning "image/svg+xml" when appropriate, instead of false.
#15
in reply to:
↑ 14
@
8 years ago
Jup, agree.
Thanks for your work @freakpants
Replying to room34:
Since (as I understand it) the core team has decided not to allow SVG uploads by default, be sure not to add SVG to the returned arrays of wp_get_mime_types() or wp_get_ext_types().
I think the best solution would take @freakpants' approach: it should be self-contained within the new wp_get_image_mime() function and just make that function capable of returning "image/svg+xml" when appropriate, instead of false.
This ticket was mentioned in Slack in #core by rachelbaker. View the logs.
8 years ago
#17
follow-ups:
↓ 19
↓ 21
↓ 22
@
8 years ago
- Resolution set to invalid
- Status changed from new to closed
Although mildly troubling the simple check; there is actually no break from WP core as it doesn't support SVG.
I patched my plugin to work around the code by faking application/svg+xml
mime type and then overriding later on.
https://github.com/Lewiscowles1986/WordPressSVGPlugin/
Much as it was a little PITA to change code (it always is), SVG upload is separate from core. AFAIK the objection that stands is that it's a security risk, so I'm not sure adding things into core to keep a small number of plugins functional rather than update those plugins should be such a concern.
It would be lovely to have all upload types implemented as plugins to core, so that passing width and height information, metadata, parsing etc becomes easier to support for core and plugin authors augmenting file-types more stringent checks etc.
What would also be lovely although I didn't see it here is if the media templates were extensible as currently to work around display issues we are doing some... interesting things with output buffering.
#18
in reply to:
↑ 12
@
8 years ago
Replying to freakpants:
No, sorry, it should work fine on PHP > 5.3. The patch @diddledan created doesn't work. He forgot to consider the applicaion/octet-stream return.
My patch works, but might not work on systems that dont have a magic file (?).
pretty sure one great reason not to use application/octet-stream
is that it's generic so then handling SVG upload types becomes problematic down the line for other plugins.
#19
in reply to:
↑ 17
@
8 years ago
Replying to LewisCowles:
Much as it was a little PITA to change code (it always is), SVG upload is separate from core. AFAIK the objection that stands is that it's a security risk, so I'm not sure adding things into core to keep a small number of plugins functional rather than update those plugins should be such a concern.
I would suggest that even if core is not going to be modified to make this function properly handle SVGs, it should at least be modified to not run the function if the MIME type is image/svg+xml. What it's returning "gets the job done" in terms of blocking SVG uploads, but it's not technically correct. Since wp_get_image_mime() relies on PHP functions that don't know how to handle SVGs, it shouldn't even run on SVG files.
Specifically, I would recommend that wp_check_filetype_and_ext() be modified (line 2282 in wp-includes/functions.php) from:
if ( $type && 0 === strpos( $type, 'image/' ) ) {
to:
if ( $type && 0 === strpos( $type, 'image/' ) && $type != 'image/svg+xml' ) {
#20
@
8 years ago
It's something they could do, but then again they would be maintaining a bit of code (no matter how small) to enable functionality that is (from my understanding) in their ideas currently a security risk...
I do think it's worrying enough that most fixes I've read have been to disable upload checks in general. I do think that code would work, but we have code I've linked that bypasses the need for Core team to do anything.
#21
in reply to:
↑ 17
@
8 years ago
Replying to LewisCowles:
Although mildly troubling the simple check; there is actually no break from WP core
Sure it is. The function that was introduced is supposed to accurately return the mime type of the file uploaded. What it currently does is return a wrong mimetype for SVG (and possible other unknown image types). Fixing that would NOT reenable SVG support in Core (that still needs the add_filter( 'upload_mimes') workaround). This update actively broke that functionality by returning the wrong mimetype.
Also, the PHP Documentation actively discourages to validate images this way. Fileinfo (which is already being used in the same core file right now anyway) is the superior way to determine mimetype either way.
http://php.net/manual/en/function.getimagesize.php
"Do not use getimagesize() to check that a given file is a valid image. Use a purpose-built solution such as the Fileinfo extension instead."
Disregard diddledans approach, my patch does not introduce any Code specific to SVG.
I also agree that disabling upload checks is troubling. And there is no need for it.
#22
in reply to:
↑ 17
@
8 years ago
- Resolution invalid deleted
- Status changed from closed to reopened
Replying to LewisCowles:
Although mildly troubling the simple check; there is actually no break from WP core as it doesn't support SVG.
We should still make sure there's no regression from 4.7, even if this ticket ends up closed as a duplicate of #39550.
#23
follow-up:
↓ 27
@
8 years ago
Im very dubious about how this other issue applies. The block mentioned there specifically checks NON-Image files. .svg Files should never even run into that specific offending code.
#24
follow-up:
↓ 25
@
8 years ago
So the fix proposed is to force anything using WordPress to follow the mime-type suggested by Fileinfo extension...
That's potentially undesirable for a few reasons I can think of:
- It could potentially override the mime type for other file types (wide-ranging effects hitting more plugins)
- It ties WP to a specific implementation and update path (other PHP exts have died in the past; we maybe shouldn't assume Fileinfo will always be there or rely on it too much)
- Just by avoiding the 'image/' prefix we should be able to avoid this altogether
IMO if anything removing superficial checks, giving advice to use a dedicated program would make it easier to maintain and limit how much WP has to do and maintain.
https://help.dreamhost.com/hc/en-us/articles/214205858-How-do-I-enable-fileinfo-
#25
in reply to:
↑ 24
@
8 years ago
No Lewis, fileinfo is the FALLBACK in my code when the other two approaches fail to detect the mimetype. It's true however that application/octet-stream is also returned for
IMAGETYPE_JPC
IMAGETYPE_JPX
IMAGETYPE_JB2
That case should probably be considered as well. Besides that, my solution should not oversteer the results by exif_imagetype or getimagesize however.
#26
@
8 years ago
The line:
$mime = image_type_to_mime_type( exif_imagetype( $file ) );
Should be improved anyway to check the result of exif_imagetype before the image_type_to_mime_type call.
exif_imagetype returns false and then gets turned into application/octet-stream (as thats the default case instead of false). That can NOT be the desired result.
#27
in reply to:
↑ 23
@
8 years ago
Replying to freakpants:
Im very dubious about how this other issue applies. The block mentioned there specifically checks NON-Image files. .svg Files should never even run into that specific offending code.
Yes, but the solution is still being worked on, and it might affect both image and non-image files.
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jnylen. View the logs.
8 years ago
#34
@
8 years ago
- Owner set to joemcgill
- Status changed from reopened to assigned
Per above Slack discussion, handing off to @joemcgill to resolve at the same time as the closely related #39550.
#35
@
8 years ago
Hi all, please test 39550.4.diff in #39550, as it should address this issue as well.
add finfo_file attempt to wp_get_image_mime().