WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#15454 closed defect (bug) (fixed)

esc_textarea() for obvious textarea escaping function.

Reported by: markjaquith Owned by:
Milestone: 3.1 Priority: high
Severity: major Version: 3.1
Component: General Keywords: has-patch commit
Focuses: Cc:

Description

Alex King brought this up privately (in case it had security implications).

We escape data in textareas a bunch of different ways. Let's introduce esc_textarea() and standardize around that.

Attachments (4)

garyc40-15454.patch (3.7 KB) - added by garyc40 10 years ago.
fixed various double escaping issues
garyc40-15454-rev2.patch (6.2 KB) - added by garyc40 10 years ago.
two more instances
garyc40-15454-rev3.patch (6.7 KB) - added by garyc40 10 years ago.
revert to esc_html() for link_notes, user_description and term_description
15454.diff (1.1 KB) - added by nacin 10 years ago.
Hack to handle link_notes and term_description, from garyc40's 3rd patch. NOT a regression.

Download all attachments as: .zip

Change History (27)

#1 @scribu
10 years ago

  • Keywords needs-patch added

I'm +1 for the idea.

#2 @scribu
10 years ago

...although I thought we used esc_html() everywhere.

#3 @nacin
10 years ago

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

This missed the ticket:

(In [16431]) esc_textarea() and application for obvious textarea escaping. props alexkingorg. fixes #15454

No, esc_html() isn't proper here, because we need to double-encode. The best function we had for it was wp_htmledit_pre(), or a direct htmlspecialchars() call.

#5 @scribu
10 years ago

Related: #14268

#6 @nacin
10 years ago

  • Resolution fixed deleted
  • Severity changed from normal to major
  • Status changed from closed to reopened

I found another instance of us double-escaping something that somehow hasn't been noticed yet. I think we should audit each of these situations by hand to ensure we're not accidentally double-escaping.

#8 @nacin
10 years ago

(In [16744]) Don't double-escape user description. see #15454.

#9 @markjaquith
10 years ago

(In [16993]) Use ENT_QUOTES in esc_textarea() in case someone uses it in a value= situation by accident. see #15454

#10 @nacin
10 years ago

(In [16995]) The user description field should be esc_textarea when context is edit. see #15454.

@garyc40
10 years ago

fixed various double escaping issues

@garyc40
10 years ago

two more instances

#11 @garyc40
10 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

There's this weird thing with sanitize_bookmark_field() and sanitize_term_field().

Originally, when $context = 'edit', they will run 'link_notes' and 'term_description' through format_to_edit($value). The 2nd parameter of format_to_edit() is left to default (which is false), which means format_to_edit($value) will escape the $value.

However, 'link_notes' and 'term_description' are already escaped before being inserted into the database (when $context = 'db'), because they're processed by "pre_link_notes" and "pre_term_description" filters, to which wp_filter_kses() is attached.

As a result, compounded with the effect of esc_textarea, these values are "triple-escaped".

I removed "format_to_edit()" from sanitize_bookmark_field() and sanitize_term_field(). However, this needs a sanity check.

Last edited 10 years ago by garyc40 (previous) (diff)

#12 @garyc40
10 years ago

In current trunk, try entering this in Link Notes (Links -> Add Link), or Category Description (Post -> Categories -> Edit a category), or Biography Info (profile.php):

Test String < Hello

It will become this inside the textarea:

Test String &lt; Hello

In the source code:

Test String &amp;lt; Hello

This is probably the rationale behind esc_html() in sanitize_user_field() in the first place.

@garyc40
10 years ago

revert to esc_html() for link_notes, user_description and term_description

#13 @ryan
10 years ago

(In [17001]) Remove some unnecessary esc_textarea() calls. Props garyc40. see #15454

#14 @ryan
10 years ago

Partial commit of rev3. I'll look at the remainder a little later. I think the htmlspecialchars() on selection in press-this.php was added for XSS reasons.

#15 @ryan
10 years ago

For 3.1, let's limit this to fixing areas broken by the esc_textarea() commit. Let's leave the calls to format_to_edit() alone until 3.2.

#16 @nacin
10 years ago

(In [17141]) Tag textareas escaped earlier with textarea_escaped. see #15454.

#17 @nacin
10 years ago

(In [17142]) Revert [16995] due to the way the data enters the db. props garyc40, see #15454.

#18 @nacin
10 years ago

Alright. Just went through and audited the three dozen or so esc_textarea calls.

There are three textareas we're still escaping farther up the stack. In wp-admin/includes/nav-menu.php, there is a menu item description. Reverting to esc_html() handles everything except &amp;, so that's exactly what I'm doing.

Indeed there is a problem with both link_notes and term_description, but this is not a regression from 3.0. No one has noticed, so I'm inclined to punt and do a better audit of what's going on there (both into the DB and out again) in 3.2.

#19 @nacin
10 years ago

(In [17143]) Revert to esc_html when escaping the textarea for nav menu item description. see #15454.

@nacin
10 years ago

Hack to handle link_notes and term_description, from garyc40's 3rd patch. NOT a regression.

#20 @nacin
10 years ago

  • Keywords commit added; needs-testing removed

I'm satisfied that there are no more regressions caused by [16431].

Everything in garyc40-15454-rev3.patch is either handled by a recent commit or 15454.diff. Or, it's in press-this.php, which there's no need to touch. (We've broken press-this enough this cycle.)

Leaving open for final review by ryan. Suggesting commit 15454.diff and close as fixed for 3.1, and we can revisit textarea_escaped instances in a new ticket. Alternatively, punt to 3.2-early, but a note on the attachment, esc_html() handles everything that esc_textarea() does except that it does not re-escape, and it does not handle &amp;. So it should be considered safe. (And is far less destructive.)

Last edited 10 years ago by nacin (previous) (diff)

#21 @ryan
10 years ago

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

(In [17146]) link_notes and term_description escaping fixes. Props garyc40. fixes #15454

#22 @Denis-de-Bernardy
7 years ago

@nacin or @ryan or @markjaquith: Any odds you could chime in when you've a moment, to explain when and why esc_textarea() is supposed to be used rather than esc_html()? The codex is rather unclear on when to use it, it doesn't seem to be worrying about the charset, and this ticket and others related to it give the impression that the only thing we introduced with esc_textarea() were bugs and regressions.

Version 1, edited 7 years ago by Denis-de-Bernardy (previous) (next) (diff)

#23 @nacin
7 years ago

esc_textarea() will double-encode html entities, whereas esc_html() will not

That's the key difference.

Note: See TracTickets for help on using tickets.