[wp-trac] [WordPress Trac] #21767: Remove stripslashes from API functions

Thu Oct 4 16:40:21 UTC 2012

#21767: Remove stripslashes from API functions
-------------------------------------------------+-------------------------
 Reporter:  alexkingorg                          |       Owner:
     Type:  defect (bug)                         |      Status:  new
 Priority:  normal                               |   Milestone:  Awaiting
Component:  General                              |  Review
 Severity:  normal                               |     Version:  trunk
 Keywords:  has-patch needs-testing needs-unit-  |  Resolution:
  tests                                          |
-------------------------------------------------+-------------------------

Comment (by nacin):

 Here's a chat between Ryan and I from early this morning. We usually pull
 these into IRC but just got carried away, so pasting it in full here.
 Sorry for the lame formatting; it's not easy to make dialogue pretty here.

 ''Nacin:'' A few years ago we bit the bullet and '''took slashing out of
 the options and transients APIs, and no one even blinked'''. [[BR]]
 ''Nacin:'' There's a ton there. I'm looking at what we can cherry pick to
 see what kind of reaction we'd get, then push the rest in for 3.6. [[BR]]
 > ''Ryan:'' Looking at 21767. '''Not sure I dig adding stripslashes all
 over the place and forking API. It's switching out one mess for
 another.''' [[BR]]
 ''Nacin:'' Any instance of stripslashes( $var ) where $var is not $_POST
 is a bad omen. [[BR]]
 ''Nacin:'' Forking API I don't like either. [[BR]]
 ''Nacin:'' In order to do this without adding stripslashes or forking the
 API, '''we'll likely need to do this in conjunction with some sort of GET
 and POST non-slashed helpers.''' [[BR]]
 > ''Ryan:'' '''Why bother forking the one set of functions when none of
 the others are forked?''' [[BR]]
 > ''Ryan:'' Simply removing slashes from the default flow does nothing for
 plugins. [[BR]]
 ''Nacin:'' Practically none of them are currently slashing. [[BR]]
 > ''Ryan:'' They are if they too are using POST. [[BR]]
 ''Nacin:'' "Given how inconsistent core WP itself is about slashing, I'm
 inclined to go with this. There will be some back-compat grief, but it's
 looking like more plugins are not escaping than are escaping. We'd be
 servicing the more popular expectation." (A wise man once said —
 http://core.trac.wordpress.org/ticket/12416#comment:10) [[BR]]
 > ''Ryan:'' A wise man who wasn't going to fork functions. [[BR]]
 ''Nacin:'' Right. The heavy use of *meta( ... $_POST ... ) is really the
 only situation where we are stuck with an unclear path forward. [[BR]]
 > ''Ryan:'' I'd say the same about wp_insert_post(). [[BR]]
 ''Nacin:'' I'd be surprised if any WP.com code was doing that right.
 [[BR]]
 ''Nacin:'' I mean, we don't. No one does. [[BR]]
 > ''Ryan:'' I dunno. '''I think the only way we do this is if we stop GPC
 slashing.''' [[BR]]
 ''Nacin:'' That would be a SQL injection floodgate for plugins. [[BR]]
 ''Nacin:'' '''If instead of stripslashes() everywhere, it was
 wp_unslash()?''' [[BR]]
 > ''Ryan:'' That would help. '''At least we could mark these things.'''
 [[BR]]
 > ''Ryan:'' 1298 wp_insert_post() calls on dotcom. [[BR]]
 ''Nacin:'' A few hundred might be properly slashed by receiving postdata.
 If more than 10 are *explicitly* slashed, I'd be surprised. [[BR]]
 > ''Ryan:'' '''A random sampling of five were pulls from the db that were
 sent back without slashes.''' [[BR]]
 ''Nacin:'' I will say that we did this for option values and I never saw a
 single bug report or complaint. [[BR]]
 ''Nacin:'' How many people *really* have \' or \" in everyday content?
 [[BR]]
 > ''Ryan:'' I like $post_data = stripslashes_deep( $_POST );  at the top
 of functions. [[BR]]
 ''Nacin:'' You're right, the big issue is $_POST data. [[BR]]
 > ''Ryan:'' And files. Core admin files that use GPC should just strip at
 the top instead of situationally. [[BR]]
 ''Nacin:'' But that could then mean an API function tries to double-strip.
 [[BR]]
 > ''Ryan:'' Something to make it easier to switch to unslashed GPC? [[BR]]
 > ''Ryan:'' It can't be a copy though. It'd get out of sync. [[BR]]
 ''Nacin:'' '''Nah, it'd be a magic object.''' [[BR]]
 ''Nacin:'' '''`WP::GET['key'], WP::POST['key']`''' [[BR]]
 > ''Ryan:'' strip on the fly every time? Too slow. [[BR]]
 ''Nacin:'' I mean, we're basically already doing that. [[BR]]
 > ''Ryan:'' True. [[BR]]
 > ''Ryan:'' Looking through the patch. '''Any function that strips GPC
 should probably strip at the very top.''' [[BR]]
 > ''Ryan:'' I'm coming around. [[BR]]
 > ''Ryan:'' Something like WP::GET/POST would be nice and clean. [[BR]]
 ''Nacin:'' Hello WP class, nice to see you, time to make you more
 decorative. [[BR]]
 ''Nacin:'' '''The problem with a magic object is that it does not solve
 $_POST going into _meta()''' [[BR]]
 > ''Ryan:'' True. Forking there seems okay. It smelled bad a first, but
 now I can see the reasoning. [[BR]]
 ''Nacin:'' So I guess our next question is, '''can we actually get away
 with just dropping GPC slashing?''' [[BR]]
 > ''Ryan:'' probably 90% get away with it, 10% sql injection. [[BR]]
 > ''Ryan:'' We can always keep that in our pocket. This is a step towards
 that. [[BR]]
 > ''Ryan:'' Meanwhile, encourage WP::GET and leave the old GPC slashed.
 [[BR]]
 ''Nacin:'' We already see a ton of SQL injection reports in plugins.
 Ultimately, they are already doing it wrong, and GPC slashing is saving
 them at the last possible moment. It won't affect any API calls or
 prepares. [[BR]]
 > ''Ryan:'' '''Would we want decorated WP::GET even if we stopped slashing
 GPC?''' [[BR]]
 ''Nacin:'' The only way it'd be needed is if people needed a way to
 determine if GPC was slashed. [[BR]]
 ''Nacin:'' Something as simple as function '''wp_gpc_slashed()''' { return
 false; } and then toggling that to true in a future release can offer an
 upgrade path. [[BR]]
 ''Nacin:'' We'd need to '''commit this day one of a cycle''', announce it
 before that, and probably do it in coordination with some proactive scans
 on the plugin directory. [[BR]]
 ''Nacin:'' '''You know, we could solve this at the dev summit.''' I was
 going to recommend a dev chat, but there, we'd have everyone in the same
 room. This is exactly the kind of architecture decision we should be
 making at a summit. [[BR]]

-- 
Ticket URL: <http://core.trac.wordpress.org/ticket/21767#comment:28>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software