Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#24301 closed defect (bug) (invalid)

Unescaped user input in image preview

Reported by: tollmanz's profile tollmanz Owned by:
Milestone: Priority: high
Severity: major Version: 3.6
Component: Post Formats Keywords: has-patch
Focuses: Cc:

Description

On line 36 of wp-admin/includes/post-formats.php as of r24227, user inputted data is printed to the screen without being escaped. The data is the fourth fallback for the image data.

To recreate the issue:

  1. Go to Posts > Add New.
  2. Click the Image post format icon.
  3. Click "use an image URL or HTML".
  4. Enter <img src="http://placehold.it/200x200 />, being sure to omit the last ".
  5. Enter a title.
  6. Save the post.
  7. Things are messed up.

The problem is that on line 36 of wp-admin/includes/post-formats.php a value is printed directly to the screen without being escaped. I am not sure how this should be fixed as not all mangled HTML can be repaired; however, I do not think that unescaped user input should be printed to the screen like this. My example is annoying, but harmless. This seems like something that is exploitable.

Attachments (7)

24301.patch (468 bytes) - added by SergeyBiryukov 12 years ago.
24301.2.diff (524 bytes) - added by tollmanz 12 years ago.
24301.3.diff (493 bytes) - added by SergeyBiryukov 12 years ago.
24301.4.diff (1.8 KB) - added by SergeyBiryukov 12 years ago.
24301.5.diff (1.8 KB) - added by SergeyBiryukov 12 years ago.
24301.6.diff (1.8 KB) - added by SergeyBiryukov 12 years ago.
24301.7.diff (2.0 KB) - added by georgestephanis 12 years ago.

Download all attachments as: .zip

Change History (27)

#1 @SergeyBiryukov
12 years ago

  • Milestone changed from Awaiting Review to 3.6
  • Version set to trunk

#2 @SergeyBiryukov
12 years ago

  • Priority changed from normal to high

#4 @kovshenin
12 years ago

Note that this breaks for users only with unfiltered_html caps, otherwise it becomes <img /> after running through wp_filter_post_kses. We generally know what to expect in these meta fields, so does it make sense to run kses even if the current user has unfiltered html caps, for whom we may extend to allow iframe, object, script, etc?

#5 @tollmanz
12 years ago

@kovshenin Your point is well taken; however, the problem that I see if that if a user mistakenly messes up the HTML, the display is broken:

http://f.cl.ly/items/2w1Y1G322u1Y0I2i1d2a/broken-display.png

Because the HTML is broken, I cannot fix the issue without manually correcting the HTML in Web Inspector/Firebug. In similar situations when unfiltered HTML is allowed, the admin display does not have the potential to break because the HTML is not rendered back to the screen (e.g., the WordPress editor, text widgets).

As an example, if I enter <img src="http://placehold.it/200x200 /> into the text tab of the WP editor and save the post, I get <img src="http://placehold.it/200x200 /> back, the same content that I entered. Interestingly, if I enter that content into the text tab, switch to the visual tab, then switch back, the content changes to <img alt="" src="&quot;http://placehold.it/200x200" />. It seems it tries to fix the broken content. Perhaps it is reasonable to use this same escaping strategy?

Even though we might be able to trust users with these permissions more, I still think it is quite important to escape this user output in some way. If the escaping breaks the inputted HTML, yet saves the page display, this is probably a good thing as it will fix serious issues for the user with the frontend display of the content. Not to mention, there is likely a way to abuse this maliciously.

#6 @SergeyBiryukov
12 years ago

  • Keywords has-patch added

Apart from a missing quote, it's also fairly easy to break the admin by inserting a closing </div> tag here.

24301.patch adds strip_tags( $value, '<img>' ) to only allow <img> tags. It turned out that this helps with the missing quote too, strip_tags() removes the broken markup.

Last edited 12 years ago by SergeyBiryukov (previous) (diff)

@tollmanz
12 years ago

#7 @tollmanz
12 years ago

For parity with the HTML produced when you upload an image, we should probably allow <a> as well. 24301.2.diff adds this.

#8 @kovshenin
12 years ago

24301.patch looks good. Since we need to really just display an image at that point, I don't see a need for allowing links there too, but it wouldn't hurt. Any ideas on audio and video?

#9 @SergeyBiryukov
12 years ago

24301.3.diff also adds force_balance_tags() to prevent an unclosed <a> tag.

#10 @tollmanz
12 years ago

The only reason I think the link would be helpful to display is for consistency. When I choose an image from the media uploader, it is presented as a linked image. I think UI consistency is important.

That said, I agree that the link is not important and would rather that it always displays just the image, but that is perhaps beyond this ticket.

#11 follow-up: @SergeyBiryukov
12 years ago

24301.4.diff is an attempt at the same approach for audio and video.

It's also possible to break the admin by inserting a closing </div> tag after [audio] or [video] shortcode. The patch should prevent that too.

#12 in reply to: ↑ 11 ; follow-up: @kovshenin
12 years ago

Replying to SergeyBiryukov: Audio (and possibly video) embeds could be more than just video, source and a. We should allow iframe, embed and even script, possibly more.

#13 @tollmanz
12 years ago

Should we also allow <embed> and <iframe> tags as well as they are popular forms of embed code for video (and I am not sure about audio).

#14 in reply to: ↑ 12 ; follow-up: @SergeyBiryukov
12 years ago

Replying to kovshenin:

Audio (and possibly video) embeds could be more than just video, source and a. We should allow iframe, embed and even script, possibly more.

24301.5.diff adds <embed> and <iframe>. Not sure about <script>, could you give an example that would require it?

#15 in reply to: ↑ 14 @kovshenin
12 years ago

Replying to SergeyBiryukov: couldn't find any video services with <script> but I did come across quite a few that do <param>.

#16 follow-up: @SergeyBiryukov
12 years ago

Probably <object> too?

#17 in reply to: ↑ 16 @kovshenin
12 years ago

Replying to SergeyBiryukov: Indeed. I thought you had it in the list already.

#18 follow-up: @markjaquith
12 years ago

I've come across video services in the past that have utilized <script> tags.

#19 in reply to: ↑ 18 @georgestephanis
12 years ago

Replying to markjaquith:

I've come across video services in the past that have utilized <script> tags.

For the sake of security, I'd definitely not allow script embeds.

I think we need to draw the line somewhere, and I'd personally like to say we'll just accept videos from oEmbed providers -- potentially even if they're not whitelisted to allow for future growth?

That being improbable for a good UX experience, patch forthcoming with the allowed tags in a filter to let the users add new ones if they want. Default to higher security, but they can add <script> in if they want.

#20 @ocean90
12 years ago

  • Milestone 3.6 deleted
  • Resolution set to invalid
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.