WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 16 months ago

Last modified 16 months ago

#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)

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

Download all attachments as: .zip

Change History (45)

#1 @SergeyBiryukov
17 months ago

  • Milestone changed from Awaiting Review to 4.7.2

@diddledan
17 months ago

add finfo_file attempt to wp_get_image_mime().

#2 @diddledan
17 months ago

  • Keywords has-patch needs-unit-tests added

@diddledan
17 months ago

fix formatting of previous patch

#3 @freakpants
17 months 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 17 months ago by freakpants (previous) (diff)

#4 @dd32
17 months ago

See #39550 as welll.

#5 @freakpants
17 months ago

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

Last edited 17 months ago by freakpants (previous) (diff)

#6 @diddledan
17 months ago

  • Keywords has-patch removed

patch doesn't work. re-thinking.

@freakpants
17 months ago

fixed comment indentation

@diddledan
17 months ago

this works I think

@diddledan
17 months ago

fix wonky indents - this version should work everywhere I think

#7 @diddledan
17 months ago

  • Keywords has-patch added

#8 @ralva83702
17 months 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 17 months ago by ralva83702 (previous) (diff)

#9 follow-up: @freakpants
17 months 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.


17 months ago

#11 in reply to: ↑ 9 @ralva83702
17 months 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
17 months 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
17 months ago

#39580 was marked as a duplicate.

#14 follow-up: @room34
17 months 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
17 months 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.


17 months ago

#17 follow-ups: @LewisCowles
17 months 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
17 months 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
17 months 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 17 months ago by room34 (previous) (diff)

#20 @LewisCowles
17 months 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
17 months 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 17 months ago by freakpants (previous) (diff)

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

Last edited 17 months ago by LewisCowles (previous) (diff)

#25 in reply to: ↑ 24 @freakpants
17 months 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 17 months ago by freakpants (previous) (diff)

#26 @freakpants
17 months 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 17 months ago by freakpants (previous) (diff)

@freakpants
17 months ago

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

@freakpants
17 months ago

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

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

Ah i see. :)

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


17 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


16 months ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


16 months ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


16 months ago

This ticket was mentioned in Slack in #core by jnylen. View the logs.


16 months ago

#34 @jnylen0
16 months 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
16 months ago

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

#36 @joemcgill
16 months 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
16 months 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.