Usermeta and postmeta functions assume data to be pre-escaped

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

[markjaquith]

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

[darkdragon]

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

[Denis-de-Bernardy]

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.

[Denis-de-Bernardy]

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.

[Denis-de-Bernardy]

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.

[Denis-de-Bernardy]

see also: #9640 and #7540

[hakre]

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.

[hakre]

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