SVG upload support broken in 4.7.1

[freakpants]

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

[diddledani]

add finfo_file attempt to wp_get_image_mime().

[diddledani]

fix formatting of previous patch

[freakpants]

Note: wp_get_image_mime doesnt actually return false, but application/octet-stream.

image_type_to_mime_type(false) returns that result for some reason;

[dd32]

See #39550 as welll.

[freakpants]

the elseIf case with getimagesize actually returns false. So the fix needs to check for false OR application/octet-stream.

[diddledani]

patch doesn't work. re-thinking.

[freakpants]

fixed comment indentation

[diddledani]

this works I think

[diddledani]

fix wonky indents - this version should work everywhere I think

[ralva83702]

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?

[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.

[ralva83702]

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.

[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 (?).

[SergeyBiryukov]

#39580 was marked as a duplicate.

[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.

[chrigu99]

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.

[LewisCowles]

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.

[LewisCowles]

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.

[room34]

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' ) {

[LewisCowles]

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.

[freakpants]

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.

[SergeyBiryukov]

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.

[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.

[LewisCowles]

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-

[freakpants]

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.

[freakpants]

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.

[freakpants]

Amended version of the patch that should not impact application/octet-stream images.

[freakpants]

Fixed comment, amended variable name for more precise information about its content

[SergeyBiryukov]

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.

[freakpants]

Ah i see. :)

[jnylen0]

Per above Slack discussion, handing off to @joemcgill to resolve at the same time as the closely related #39550.

[joemcgill]

Hi all, please test 39550.4.diff in #39550, as it should address this issue as well.

[joemcgill]

In 40124:

Media: Reduce failing uploads following 4.7.1.

[39831] introduced more strict MIME type checking for uploads, which
resulted in unintetionally blocking several filetypes that were
previously valid. This change uses a more targeted approach to MIME
validation to restore previous behavior for most types.

Props blobfolio, iandunn, ipstenu, markoheijnen, xknown, joemcgill.
Fixes #39550, #39552.

[joemcgill]

In 40134:

Media: Reduce failing uploads following 4.7.1.

[39831] introduced more strict MIME type checking for uploads, which
resulted in unintetionally blocking several filetypes that were
previously valid. This change uses a more targeted approach to MIME
validation to restore previous behavior for most types.

Props blobfolio, iandunn, ipstenu, markoheijnen, xknown, joemcgill.
Merges [40124] and [40125] to the 4.7 branch.
Fixes #39550, #39552.