Unescaped user input in image preview

[tollmanz]

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.

[tollmanz]

It appears that this is the case with audio (​http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/post-formats.php#L132) and video (​http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/post-formats.php#L94) too.

[kovshenin]

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?

[tollmanz]

@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:

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.

[SergeyBiryukov]

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.

[tollmanz]

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

[kovshenin]

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?

[SergeyBiryukov]

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

[tollmanz]

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.

[SergeyBiryukov]

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.

[kovshenin]

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.

[tollmanz]

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

[SergeyBiryukov]

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?

[kovshenin]

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

[SergeyBiryukov]

Probably <object> too?

[kovshenin]

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

[markjaquith]

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

[georgestephanis]

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.

[ocean90]

[24388] / #24452