Ticket #14758 (closed enhancement: fixed)

Opened 21 months ago

Last modified 18 months ago

Do not run kses on display filters for front page views

Reported by: ryan Owned by:
Priority: normal Milestone: 3.1
Component: Performance Version:
Severity: normal Keywords:
Cc:

Description

Several display filters run wp_kses_data and other heavyweight functions. These functions are already run when saving. They were added to the display filters as a defense-in-depth for the possibility of an exploit sneaking things into the DB. Running these on the display causes a serious performance hit, however. wp_list_bookmarks() running kses on the link fields can burn up 10% of the total page load time. Let's limit running these functions to admin page displays. Displaying bad fields in the admin is more dangerous since those fields can cover their tracks. We can lose the belt and suspenders approach for front page displays where performance is more critical.

Attachments

14758.diff Download (2.1 KB) - added by ryan 21 months ago.

Change History

ryan21 months ago

It's also worth noting that we're often already escaping these values, and that if the person has access to exploit the database, you're hosed anyway.

(In [15559]) Do not run kses on display filters for front page views. see #14758

Ryan, how do you measure the performance of filters?

(Just curious to know and learn. When I try new things I check with the WP-Profile plugin --  http://devel.kostdoktorn.se/wp-profile-plugin -- by Johan Eenfeldt.)

Isn't this contrary to security best practices? I mean, the database is obviously not supposed to contain insecure data. But it remains an untrusted source: if an SQL injection prone plugin allows anything malicious into it, this ticket ensures we're removing our last line of defense against XSS vulnerabilities and so forth.

comment:5 follow-up: ↓ 6   nacin21 months ago

I believe we are still properly escaping in attributes and such.

comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7   Denis-de-Bernardy21 months ago

Replying to nacin:

I believe we are still properly escaping in attributes and such.

Try manually updating, into your database, a category and set its description to:

XSS<script>alert('Evil script that exploits an IE security hole');</script>

And then visit that category on your site's front page. Assuming it gets displayed on your theme, I would assume [15559] allows the alert to show.

comment:7 in reply to: ↑ 6   demetris21 months ago

Replying to Denis-de-Bernardy:

Replying to nacin:

I believe we are still properly escaping in attributes and such.

[SNIP] I would assume [15559] allows the alert to show.

It does.

A description doesn't fall into an attribute, so it wouldn't be escaped.

Try doing the same thing in post content.

Performance measured with xdebug and cachegrind. When Rasmus does his PHP performance presentation, we get dinged for how slow this is, particularly in wp_list_bookmarks() -> sanitize_bookmark_field() -> apply_filters() -> wp_kses_data(). Imagine if we did kses on post_content and post_title during display.

Thanks for the answer, Ryan.

I did not know about that presentation by Rasmus. I watched it and I found it interesting that the only part where he found significant room for improvement was wp_list_bookmarks() (which many people don’t use anyway).

@ryan: status check on this one?

  • Status changed from new to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.