WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 13 days 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 3 years ago.
fixed various double escaping issues
garyc40-15454-rev2.patch (6.2 KB) - added by garyc40 3 years ago.
two more instances
garyc40-15454-rev3.patch (6.7 KB) - added by garyc40 3 years ago.
revert to esc_html() for link_notes, user_description and term_description
15454.diff (1.1 KB) - added by nacin 3 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)

comment:1 scribu3 years ago

  • Keywords needs-patch added

I'm +1 for the idea.

comment:2 scribu3 years ago

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

comment:3 nacin3 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.

comment:5 scribu3 years ago

Related: #14268

comment:6 nacin3 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.

comment:8 nacin3 years ago

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

comment:9 markjaquith3 years ago

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

comment:10 nacin3 years ago

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

garyc403 years ago

fixed various double escaping issues

garyc403 years ago

two more instances

comment:11 garyc403 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 3 years ago by garyc40 (previous) (diff)

comment:12 garyc403 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.

garyc403 years ago

revert to esc_html() for link_notes, user_description and term_description

comment:13 ryan3 years ago

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

comment:14 ryan3 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.

comment:15 ryan3 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.

comment:16 nacin3 years ago

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

comment:17 nacin3 years ago

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

comment:18 nacin3 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.

comment:19 nacin3 years ago

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

nacin3 years ago

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

comment:20 nacin3 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 3 years ago by nacin (previous) (diff)

comment:21 ryan3 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

comment:22 Denis-de-Bernardy13 days 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, and this ticket and others related to it give the impression that the only thing we introduced with esc_textarea() were bugs and regressions.

The main difference I'm seeing between the two are:

  • esc_textarea() will blissfully spit out invalid utf8 chars, whereas esc_html() will strip them
  • esc_textarea() will double-encode html entities, whereas esc_html() will not
Last edited 13 days ago by Denis-de-Bernardy (previous) (diff)

comment:23 nacin13 days 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.