A couple of security checks and practices to bring up for discussion (for core and plugins):
1) extract($this->handler_vars). While this does make it nicer/easier to code, it can be dangerous. $handler_vars contain ALL POST and GET; if you're not careful of where you do it and ensure that variables are set properly, it could lead to "variable injection"; allowing XSS/CSRF scripties to screw things up. We should maintain a list (array_intersect) of the allowed fields to ensure we don't unexpectedly extract an unwanted var. 2) validate, validate, validate. The fact that handler_vars is POST/GET/RR_Args all merged in to one, dismisses the first step of validation; validate the data came from the proper location. This does not provide protection in it's self, but is the first step to validation. it might be better to separate handler_vars into their respective arrays: post, get, rr_args. validate some more. If the data you want should be an int, validate/cast it to an int. If your method expects an object of type FooBar cast it to a FooBar object (same for int, array) etc... then validate some more. If a string can only be 255 utf-8 chars long, validate that it is no more that 255 uft-8 chars. If the string is a URI/Email addy, validate that it only contains valid characters for that scheme. If an int can only be of certain values, ensure that it is. etc.. 3) **** ALL (non-trusted) user supplied data that is sent to the DB, MUST be filtered with InputFilter. This includes data from 3rdparty services not just human users; twitter, delicious, etc.. Ensure that ALL data you generate is properly filtered/sanitized for DB insertion, not just the primary data, but even secondary data. When adding log entries, for example, ensure you have escaped all the data/error messages you are logging. Ensuring that the data is properly filtered going in, ensures that mistakes in retrieving that data for output, will not result in security holes. And yes, we will make mistakes/miss things. 4) ALL (non-trusted) user supplied data assigned to a theme, MUST be sanitized. If you are passing user supplied data back to a theme for re-submition, like commenter_name etc., it MUST be sanitized/escaped. We cannot rely on templates/themes to properly escape things. Again, ensuring the data going in is filtered, does not cause problems if mistakes are made on output. 5) WSSE in the admin provides another layer of validation to ensure the request to admin is a valid request. We should ensure that this WSSE check is made on all POST action admin pages, ie. any page that changes something, before we proceed. We should also put the WSSE in it's own class to reduce code duplication, and increase ease of use. WSSE validation would be done with one method call, and redirect to error page and die() if not valid. (that might cause usability problems, and ajax problems) 6) the fact that AdminHandler separates GET and POST requests, is ignored on some admin pages. We should ensure that GET requests ONLY display data, and POST requests ONLY change data. This is part of validating the data came from the proper location, mentioned above. I think most of the recent exploits come from the fact that Adminhandler is a mess, and next to impossible to follow the code flow. I would like to call out to the community to come up with ideas for a better way to organize adminhandler (I know we had talks before), keeping in mind everything mentioned above. If you think everything I said was crap, or have something to add, please speak up :) -- Matt Read http://mattread.com --~--~---------~--~----~------------~-------~--~----~ To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/habari-dev -~----------~----~----~----~------~----~------~--~---
