WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#14758 closed enhancement (fixed)

Do not run kses on display filters for front page views

Reported by: ryan Owned by:
Milestone: 3.1 Priority: normal
Severity: normal Version:
Component: Performance Keywords:
Focuses: 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 (1)

14758.diff (2.1 KB) - added by ryan 4 years ago.

Download all attachments as: .zip

Change History (14)

ryan4 years ago

comment:1 nacin4 years 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.

comment:2 ryan4 years ago

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

comment:3 demetris4 years ago

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

comment:4 Denis-de-Bernardy4 years ago

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: nacin4 years ago

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

comment:6 in reply to: ↑ 5 ; follow-up: Denis-de-Bernardy4 years 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 demetris4 years 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.

comment:8 nacin4 years ago

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

Try doing the same thing in post content.

comment:9 ryan4 years ago

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.

comment:11 demetris4 years ago

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

comment:12 jane3 years ago

@ryan: status check on this one?

comment:13 ryan3 years ago

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