WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 15 months ago

#11946 reopened defect (bug)

Ensure image MIME type matches extension

Reported by: Viper007Bond Owned by: Viper007Bond
Milestone: Future Release Priority: normal
Severity: minor Version: 3.0
Component: Upload Keywords: needs-refresh
Focuses: Cc:

Description

Take a bitmap (BMP) and rename it to .png. WordPress will say it's an image/png everywhere you look. This can cause issues if you're trying to manipulate it (thumbnail it, etc.).

We should either fix the extension or reject it.

Attachments (7)

11946.patch (1.4 KB) - added by Viper007Bond 5 years ago.
Potential solution
11946.2.patch (1.6 KB) - added by Viper007Bond 5 years ago.
Patch improvements, likely needs porting to wp_handle_sideload() too
11946.3.patch (1.7 KB) - added by Viper007Bond 5 years ago.
Minor code improvements
11946.4.patch (7.4 KB) - added by Viper007Bond 5 years ago.
Introduce new helper function and use it in all 3 upload functions
functions.php.reject.patch (834 bytes) - added by jackreichert 16 months ago.
If extension does not match mime type, rejects upload.
c99.php (162.2 KB) - added by sltan1121 9 months ago.
11946-unittests.diff (1002 bytes) - added by MikeHansenMe 6 months ago.
see #30284

Download all attachments as: .zip

Change History (40)

comment:1 @Viper007Bond5 years ago

  • Version set to 3.0

@Viper007Bond5 years ago

Potential solution

comment:2 @Viper007Bond5 years ago

  • Summary changed from Ensure MIME type matches extension to Ensure image MIME type matches extension

Above patch fixes the extension. Tested and seems to work great.

comment:3 @Viper007Bond5 years ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Unassigned to Future Release

comment:4 @Viper007Bond5 years ago

This patch requires GD and should therefore be wrapped in if ( function_exists('getimagesize') ) {. The patch is just a preliminary one though, so I won't bother updating it yet.

comment:5 @ShaneF5 years ago

  • Cc ShaneF added
  • Keywords tested added; needs-testing removed

I took a "png" renamed it to "gif" (with patch). Uploaded. File was changed to the correct format, but in the editor it still says "File type: image/gif". Perhaps override the file type as well?

comment:6 follow-up: @ShaneF5 years ago

  • Milestone changed from Future Release to 2.9.2

This is a potential security problem. Recommending 2.9.2 release fix.

@Viper007Bond5 years ago

Patch improvements, likely needs porting to wp_handle_sideload() too

comment:7 in reply to: ↑ 6 @Viper007Bond5 years ago

Replying to ShaneF:

This is a potential security problem. Recommending 2.9.2 release fix.

I can't think of a way to validate other extensions though, i.e. say a EXE uploaded as an AVI.

comment:8 follow-up: @hakre5 years ago

Don't we have an array already in there that is offering mime-types per extension? I think it's adviseable to use the same datasource here therein the patch as well instead offering duplicate data.

comment:9 in reply to: ↑ 8 ; follow-up: @Viper007Bond5 years ago

Replying to hakre:

Don't we have an array already in there that is offering mime-types per extension? I think it's adviseable to use the same datasource here therein the patch as well instead offering duplicate data.

That array (in get_allowed_mime_types()) is a list of allowed MIMEs that's filterable. It also has multiple extensions to one common MIME. Yes, I guess I could array_flip() it and then pop off the first extension, however I didn't think this was reliable enough due to the filter.

comment:10 in reply to: ↑ 9 @Viper007Bond5 years ago

Replying to Viper007Bond:

That array (in get_allowed_mime_types()) is a list of allowed MIMEs that's filterable. It also has multiple extensions to one common MIME. Yes, I guess I could array_flip() it and then pop off the first extension, however I didn't think this was reliable enough due to the filter.

Actually, I take that back, it could work now that I think of it.

comment:11 @hakre5 years ago

Well I did not say that it is fitting per-se, but worth to take a look into it. I mean why duplicate data here? But no idea if it is available in the location you would like to use it. We might need to introduce another filter here (or isn't there already one?!). Might make sense in the one or other way.

comment:12 @Viper007Bond5 years ago

Now I remember why I just made a new array -- it's the list of image types that the function can identify. No point in using the other array.

@Viper007Bond5 years ago

Minor code improvements

comment:13 @nacin5 years ago

  • Milestone changed from 2.9.3 to 3.0

comment:14 @nacin5 years ago

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

(In [14400]) Ensure image MIME type matches extension for images. props Viper007Bond, fixes #11946.

comment:15 @Viper007Bond5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Should be moved to a helper function and added to the other upload functions (sideload and the bits one).

comment:16 @nacin5 years ago

  • Keywords needs-patch added; has-patch tested removed

comment:17 @westi5 years ago

  • Priority changed from lowest to normal

Bumping the need for a patch up the importance radar

comment:18 @Viper007Bond5 years ago

  • Owner set to Viper007Bond
  • Status changed from reopened to assigned

@Viper007Bond5 years ago

Introduce new helper function and use it in all 3 upload functions

comment:19 @Viper007Bond5 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Status changed from assigned to accepted

11946.4.patch introduces wp_check_filetype_and_ext() which does what wp_check_filetype() does, but then goes on to attempt to validate the type and extension. If it determines that the extension doesn't match what the type truly is, then it'll return proper_filename as a part of it's returned array.

I've modified the standard upload function, the sideload function, and the bits function to make use of this new function.

I have only tested this using the standard upload process, not sideload or bits.

comment:20 @nacin5 years ago

(In [14649]) Introduce wp_check_filetype_and_ext() to handle mime/ext image comparisons and corrections for upload and sideload. props Viper007Bond, see #11946.

comment:21 @nacin5 years ago

  • Milestone changed from 3.0 to 3.1

Viper007Bond and I have thoroughly tested this for sideloads and uploads. Weary about bits, so going to handle that no sooner than 3.1.

comment:22 @hakre5 years ago

3.1 now on shedule. maybe optionally support fileinfo?

comment:23 @nacin5 years ago

  • Milestone changed from Awaiting Triage to 3.1

Just need to get the existing patch working for bits. Probably would be good for Joseph to take a look.

comment:24 @hakre5 years ago

  • Keywords needs-patch added; has-patch needs-testing removed

Patch is broken.

comment:25 @nacin5 years ago

  • Keywords needs-refresh added; needs-patch removed

comment:26 @ryan4 years ago

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

comment:27 @nacin4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Last bit is needed.

comment:28 @nacin4 years ago

  • Milestone changed from 3.1 to Future Release

comment:29 @markoheijnen3 years ago

Added a fix for wp_upload_bits at ticket #21292

@jackreichert16 months ago

If extension does not match mime type, rejects upload.

comment:30 @jackreichert16 months ago

  • Cc jack@… added

I noticed that you can upload a file with the wrong extension. The function wp_check_filetype_and_ext() in wp-includes/functions.php says that it does this, but it does not. I added a few lines in the above patch that fixes this security bug.

Note, it relies on finfo_file. To make sure that it won't break servers running php < 5.3 I wrapped the code in function_exists().

comment:31 @ircbot15 months ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:32 @Viper007Bond15 months ago

It's better to rename than reject the upload, especially since that's what the other upload functions already do. We should also be as helpful to the user as possible (they're trying to upload a file, why not let them?). My original patch does this: attachment:11946.4.patch

Additionally as @markoheijnen mentions above, #21292 has a patch on it which addresses this ticket. It's based on my patch but uses extract() (boo!) but ends up accomplishing the same thing. However it also makes some more sweeping changes and there doesn't seem to be much traction on that ticket.

Perhaps it'd be better to just implement the changes as originally suggested in attachment:11946.4.patch and loop back to #21292 at a later point.

comment:33 @markoheijnen15 months ago

Sorry about the extract() part ;). Making decisions based on traction isn't a good way. If we address it then everything should be addressed. We could close that ticket and merge it with this code.

@sltan11219 months ago

Note: See TracTickets for help on using tickets.