WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#40193 closed defect (bug) (fixed)

wp_ajax_crop_image capability checks too strict

Reported by: Cybr Owned by: johnbillion
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.3
Component: Media Keywords: has-patch needs-testing
Focuses: administration Cc:
PR Number:

Description

This function wp_ajax_crop_image() has two issues, on these two lines:
https://github.com/WordPress/WordPress/blob/62061bab5658386d0bbfbfeb3f89be883fd75ec4/wp-admin/includes/ajax-actions.php#L3166-L3167

Line 3167 (capability):
It checks for user capability customize.
I believe this should be upload_files.

This is because the function has a default handler for $context, which therefore can be used outside of the Customizer feature.

Line 3166 (referer):
It checks for image_editor- . $attachment_id nonce.
This nonce is created only if the user can edit the file.

Because this function creates a new image from the old without actually editing or removing the old image, this is also too strict.

https://github.com/WordPress/WordPress/blob/62061bab5658386d0bbfbfeb3f89be883fd75ec4/wp-includes/media.php#L3136-L3140

Conclusions:

  1. The function works neatly outside of the Customizer, aside from the capability and "referer" restrictions.
  2. The function has too strict capabilities.
  3. Function wp_prepare_attachment_for_js requires an additional nonce (e.g. {crop-image-$id}) based on the upload_files capability.

Attachments (1)

40193.diff (514 bytes) - added by johnbillion 2 years ago.

Download all attachments as: .zip

Change History (8)

#1 @Cybr
3 years ago

  • Focuses javascript added; ui removed

#2 @swissspidy
3 years ago

  • Version changed from trunk to 4.3

@johnbillion
2 years ago

#3 @johnbillion
2 years ago

  • Focuses javascript removed
  • Keywords has-patch reporter-feedback needs-testing added

@Cybr Thanks for the report. What's the use case for a user being able to generated a crop of an image if they cannot edit the attachment?

I think 40193.diff should be sufficient, but there may well be a use case that I'm unaware of. Let me know!

#4 @andreiglingeanu
2 years ago

@johnbillion I'm personally building some simple UI for quickly changing the origin of the crop. And I'd like to quickly be able to regenerate the image after the user made that change. Having that ajax action at our disposal is very handy!

Looks like the patch is just enough, yes.

#5 @Cybr
2 years ago

  • Keywords reporter-feedback removed

@johnbillion Here are two scenarios I could quickly think of:

  1. Setting a social image for Open Graph/Twitter.
  2. Setting a page header image (or any other layout part).

I believe the change is sufficient for most scenarios plugin/theme authors will encounter.
If you'd also like to cover scenarios outside of post editing, then upload_files would be a better fit (as per original comment).

#6 @johnbillion
2 years ago

  • Milestone changed from Awaiting Review to 4.9
  • Owner set to johnbillion
  • Status changed from new to reviewing

wp_ajax_imgedit_preview() and wp_ajax_image_editor() both use current_user_can( 'edit_post', $post_id ) as the capability check.

#7 @johnbillion
2 years ago

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

In 41270:

Media: bring the capability check in wp_ajax_crop_image() inline with those in wp_ajax_imgedit_preview() and wp_ajax_image_editor().

This change means that a user can crop an image if they have the ability to edit its attachment post, without requiring the ability to access the Customizer.

Fixes #40193

Note: See TracTickets for help on using tickets.