WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 5 years ago

#3243 closed enhancement (invalid)

Usermeta and postmeta functions assume data to be pre-escaped

Reported by: markjaquith Owned by: markjaquith
Milestone: 2.9 Priority: high
Severity: normal Version: 2.1
Component: Administration Keywords: needs-patch early
Focuses: Cc:

Description (last modified by markjaquith)

User meta functions assume that data passed to them is already escaped ( $wpdb->escape()

Post meta functions assume data is not already escaped.

I think we should move to a standardized way of doing this, and I think the standard should be to expect unescaped data.

  1. It is safer.
    • Worst case scenario with assuming data to be unescaped is that it gets double slashed
    • Worst case scenario with assuming data to be escaped is SQL injection vulnerability
  2. Post meta has been doing it this way, for a longer time, so less code would have to change
  3. It would allow code consolidation, in terms of handling arrays/objects/strings, serialization, and escape.
  4. Currently, things like First Name and Last Name are passed through filters pre-slashed, which means that filters have to work around this.

Setting a milestone of 2.2

We can do this in trunk right after 2.1 ships, so that plugin authors will have 4 months to adapt.

Change History (14)

comment:1 markjaquith8 years ago

  • Description modified (diff)
  • Status changed from new to assigned

comment:2 markjaquith7 years ago

Sink or swim time for this issue. I still think we should do it. Thoughts?

comment:3 foolswisdom7 years ago

  • Milestone changed from 2.2 to 2.3

comment:4 Nazgul7 years ago

  • Milestone changed from 2.3 (trunk) to 2.4 (future)

comment:5 darkdragon6 years ago

  • Keywords hunt-irrelevant added

This might have to do with the use prepare method. A lot of work was done there and would solve this problem.

comment:6 ryan6 years ago

  • Milestone changed from 2.5 to 2.6

comment:7 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; hunt-irrelevant removed
  • Summary changed from Usermeta functions assume data to be pre-escaped to Usermeta and postmeta functions assume data to be pre-escaped

I'd love this to happen. Also for postmeta. But changing this now will make soooo many security issues creep into old plugins.

Imo we should add a new set of functions, and deprecate the existing ones.

comment:8 Denis-de-Bernardy5 years ago

  • Keywords dev-feedback added

comment:9 Denis-de-Bernardy5 years ago

  • Keywords close added; needs-patch dev-feedback removed
  • Priority changed from normal to high

in *_post_meta(), I see:

$wpdb->update( $wpdb->postmeta, $data, $where );

in *_user_meta(), I see:

$wpdb->update($wpdb->usermeta, compact('meta_value'), compact('user_id', 'meta_key') );

so presuming fixed.

comment:10 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; close removed
  • Type changed from task (blessed) to enhancement

nm, missed the stripslashes_deep in the post_meta thingy. the ticket report is actually mixed up:

*_post_meta() expects slashed data, but *_usermeta() doesn't, based on the todo item in the update_postmeta() function.

comment:11 Denis-de-Bernardy5 years ago

  • Keywords early added

comment:13 hakre5 years ago

With all my experiences in the whole codebase for the time being: I think it is pretty much important to get things solved to have data as the data (unescaped) and it must be escaped where applicable (IMHO only for sql-queries).

This will clear a lot of points and remove security threats that are currently inside the code by design. The first big point I see is the overall handling of request variables. The next big point I see is the faulty database escaping.

"Real" data (which means unescaped) will additionally help to handle things straight in the future. Also there is no need to document that much with each function, because it is just normal to have data unsecaped by default.

comment:14 hakre5 years ago

  • Resolution set to invalid
  • Status changed from accepted to closed

anyone is really interested to patch this? if not I really suggest to close this. it's open since ages.

Note: See TracTickets for help on using tickets.