Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#39552 closed defect (bug) (fixed)

SVG upload support broken in 4.7.1

Reported by: freakpants's profile freakpants Owned by: joemcgill's profile 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)

39552.patch (737 bytes) - added by diddledani 8 years ago.
add finfo_file attempt to wp_get_image_mime().
39552.1.patch (680 bytes) - added by diddledani 8 years ago.
fix formatting of previous patch
39552.2.patch (813 bytes) - added by freakpants 8 years ago.
39552.3.patch (827 bytes) - added by freakpants 8 years ago.
fixed comment indentation
39552.2.diff (2.2 KB) - added by diddledani 8 years ago.
this works I think
39552.4.patch (2.2 KB) - added by diddledani 8 years ago.
fix wonky indents - this version should work everywhere I think
functions.php.patch (1.2 KB) - added by freakpants 8 years ago.
Amended version of the patch that should not impact application/octet-stream images.
functions.php.2.patch (1.3 KB) - added by freakpants 8 years ago.
Fixed comment, amended variable name for more precise information about its content

Download all attachments as: .zip

Change History (45)

#1 @SergeyBiryukov
8 years ago

  • Milestone changed from Awaiting Review to 4.7.2

@diddledani
8 years ago

add finfo_file attempt to wp_get_image_mime().

#2 @diddledani
8 years ago

  • Keywords has-patch needs-unit-tests added

@diddledani
8 years ago

fix formatting of previous patch

#3 @freakpants
8 years ago

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;

Last edited 8 years ago by freakpants (previous) (diff)

#4 @dd32
8 years ago

See #39550 as welll.

#5 @freakpants
8 years ago

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

Last edited 8 years ago by freakpants (previous) (diff)

#6 @diddledani
8 years ago

  • Keywords has-patch removed

patch doesn't work. re-thinking.

@freakpants
8 years ago

@freakpants
8 years ago

fixed comment indentation

@diddledani
8 years ago

this works I think

@diddledani
8 years ago

fix wonky indents - this version should work everywhere I think

#7 @diddledani
8 years ago

  • Keywords has-patch added

#8 @ralva83702
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?

Last edited 8 years ago by ralva83702 (previous) (diff)

#9 follow-up: @freakpants
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 @ralva83702
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: @freakpants
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 (?).

#13 @SergeyBiryukov
8 years ago

#39580 was marked as a duplicate.

#14 follow-up: @room34
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 @chrigu99
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: @LewisCowles
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 @LewisCowles
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 @room34
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' ) {
Last edited 8 years ago by room34 (previous) (diff)

#20 @LewisCowles
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 @freakpants
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.

Last edited 8 years ago by freakpants (previous) (diff)

#22 in reply to: ↑ 17 @SergeyBiryukov
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: @freakpants
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: @LewisCowles
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.

Version 0, edited 8 years ago by LewisCowles (next)

#25 in reply to: ↑ 24 @freakpants
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.

Last edited 8 years ago by freakpants (previous) (diff)

#26 @freakpants
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.

Last edited 8 years ago by freakpants (previous) (diff)

@freakpants
8 years ago

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

@freakpants
8 years ago

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

#27 in reply to: ↑ 23 @SergeyBiryukov
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.

#28 @freakpants
8 years ago

Ah i see. :)

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 @jnylen0
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 @joemcgill
8 years ago

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

#36 @joemcgill
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#37 @joemcgill
8 years ago

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.

Note: See TracTickets for help on using tickets.