#3243 closed enhancement (invalid)
Usermeta and postmeta functions assume data to be pre-escaped
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Priority: | high | |
Severity: | normal | Version: | 2.1 |
Component: | Administration | Keywords: | needs-patch early |
Focuses: | Cc: |
Description (last modified by )
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.
- 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
- Post meta has been doing it this way, for a longer time, so less code would have to change
- It would allow code consolidation, in terms of handling arrays/objects/strings, serialization, and escape.
- 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 (15)
#5
@
17 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.
#7
@
16 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.
#9
@
16 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.
#10
@
16 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.
#13
@
16 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.
Sink or swim time for this issue. I still think we should do it. Thoughts?