Re: HTMLPurifier and the "Input" class

Hey Bharat,

A thought on Gallery::ready() - it seems like it'd be a lot more
straightforward if it had access to the request.  In addition to the
reasons you give below, it also calls Request::user_agent().

Two other possibilities:
- build the request, call Gallery::ready(), then execute the request
- make Gallery_Controller() that overrides Kohana_Controller() and
adds a before() function that calls Gallery::ready()

Thoughts?

Take care,
Shad

On 6 April 2013 04:57, Bharat Mediratta <[email protected]> wrote:
>
> Before this email I had mocked up an Input API and have managed to plumb a
> line all the way through to successfully rendering a minimal controller
> (WOOT!).  But I went back and changed it over to using Request::$current
> instead.  This causes a problem in that we do some mechanics in
> Gallery::ready() before the request is created.  The main thing I hit is
> that Gallery_Hook_GalleryEvent::gallery_ready() calls Theme::load_themes
> which:
>
> 1) Cares about $_GET["kohana_uri"] if there's no $_SERVER["PATH_INFO"].  We
> need this to work when we're using mod_rewrite (see the mappings at the
> bottom of gallery3/.htaccess)
>
> 2) Cares about $_GET["theme"] which is the mechanism we use to let admins
> preview a theme
>
> The request isn't built until we do it at the very end of index.php, right
> after bootstrap.  Any thoughts on how to play this?  I'd rather not make
> exceptions to our security flow and use $_GET directly...
>
> -Bharat
>
>
>
> On Fri, Apr 5, 2013 at 10:30 AM, Bharat Mediratta <[email protected]>
> wrote:
>>
>>
>> Outstanding research, Shad.  TL;DR: I agree with you.  Notes inline below.
>>
>>
>> On Fri, Apr 5, 2013 at 8:29 AM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote:
>>>
>>> Hey everyone,
>>>
>>> Some updates:
>>> - HTMLPurifier - Standalone v4.5.0 is included in gallery.
>>> - Purifier - I wrote a Purifier class and config file (*strongly*
>>> based on the 3.0.x purifier module) to load a single instance of it
>>> and purify things (as Purifier::purify($str)).
>>> - Input::instance()->ip_address - changed to K3's Request::$client_ip,
>>> so we no longer need the Input library for this.
>>> - Input - wrote a draft of it
>>> (http://github.com/gallery/gallery3/pull/269)...
>>>
>>> ... but as I learn more about K3 and the form-handling module Formo,
>>> I'm starting to think it's not the right approach.  I'm pretty happy
>>> with the first three steps, but think that the fourth step (the Input
>>> class draft) won't play well with the Request class or the Formo.
>>>
>>> tl;dr: I don't think we should add an Input class, and think that it'd
>>> be a better fit in the K3 framework to add code to Gallery::ready() or
>>> bootstrap and some overrides to the Request class.
>>>
>>> ----
>>>
>>> K2 approach with global_xss_filtering = true (i.e. how Gallery 3.0.x
>>> works)
>>>
>>> - PHP fills the GPCS superglobals for us
>>> - Input::clean() cleans GPCS at init and overwrites the superglobals
>>> - Input library returns GPS values from these arrays without further
>>> processing
>>> - Input library returns C values after running them through
>>> cookie::get() to check for signed cookies and remove invalid ones
>>> - nobody downstream has access to the raw values
>>>
>>> Cleaning consists of:
>>> - GPCS: remove control chars, convert to UTF8 and remove incompatible
>>> chars
>>> - GPC: clean keys (except cookie $Version, $Path, and $Domain),
>>> standardize newlines, remove magic_quotes if needed, XSS clean
>>> - S: none
>>>
>>> Details on the filters:
>>> - clean keys: fast, simple, secure regex in MY_Input
>>> (preg_replace("/[^0-9a-zA-Z:_.-]/", "_", $str)).  This is okay.
>>> - XSS clean: less fast, less simple, less secure regex in Input.  This
>>> is problematic.
>>>
>>> ---
>>>
>>> K3 approach as-shipped
>>>
>>> - PHP fills the GPCS superglobals for us
>>> - Kohana::sanitize() cleans GPC globals at init and overwrites the
>>> superglobals
>>> - Request::factory() stores G as $request->query($_GET)
>>> - Request::factory() stores P as $request->post($_POST)
>>> - Request::factory() stores C as $request->cookie($cookies), where
>>> $cookies is $_COOKIE processed to check signed cookies
>>> - most things downstream are expected to get their GPC values from
>>> Request, which is easily accessible from any controller (*)
>>> - $_SERVER is not touched and always accessed directly
>>>
>>> Cleaning consists of:
>>> - GPC: standardize newlines, remove magic_quotes if needed
>>> - S: none
>>>
>>> Removed from the list:
>>> - remove control chars, convert to UTF8 and remove incompatible chars.
>>>  This is handled by UTF8::clean(), which is not called.
>>> - clean keys
>>> - XSS clean (obviously)
>>>
>>> (*) exception: URL::query() accesses $_GET directly, but nobody else
>>> seems to use this function.
>>
>>
>> URL::query is a somewhat dangerous function because it allows parameter
>> pollution attacks.  Ie, if you go to /index.php?action=logout and we use
>> URL::query to generate any new urls on the page (which is sometimes what
>> things like the paginator do) then you trick the user into clicking a URL
>> which has an extra bad parameter in it AND that url might have a CSRF token
>> in it so it might get accepted by the server.  Just a thought.
>>
>>>
>>>
>>> ---
>>>
>>> So, given that, here's my current thought on what we should add at init:
>>> - GPCS: call UTF8::clean() to process GPCS, overwrite the original values
>>> - GPC: clean keys (simple, secure regex like before), overwrite the
>>> original values
>>> - GPC: clean values using Purifier class, put into Request object but
>>> not into superglobals
>>
>>
>> So this would mean that the superglobals would contain the raw value?
>> Would it be safer for us to duplicate the superglobals into $_RAW_GET for
>> example then clean it into $_GET and just use the superglobals?
>>
>>>
>>> - S: clear HTTP_HOST to force folks to use SERVER_NAME instead
>>>
>>> This can be done by adding:
>>> - class Gallery_Request extends Kohana_Request, with post(), query(),
>>> and cookie() that, if given a full array, purify it before calling its
>>> parent function.  We can make an exception for csrf to run a faster,
>>> simpler [0-9a-f] regex.
>>> - extra stanza in Gallery::ready() (or bootstrap) that calls
>>> UTF8::clean(), does unset($_SERVER("HTTP_HOST")), and cleans keys.
>>
>>
>> I'd prefer to do this processing earlier in the bootstrap so that it's
>> very clear when this is happening and it happens before we load modules
>> (because in K3 when you load modules they can execute code in their init.php
>> file.
>>
>>> - (optional) override URL::query() to make it clean $_GET before using
>>> it.  This isn't strictly needed by Gallery's core, but would be
>>> security against other devs using it and accidentally introducing XSS
>>> insecurities.
>>
>>
>> That is probably a good idea.  Or if we just clean the values in $_GET
>> then it's not necessary to override URL (but it still doesn't protect us
>> from pollution attacks).
>>
>>>
>>>
>>> Then, to access them:
>>> - GPC cleaned: use Request object from controllers ($request->query(),
>>> $request->post(), $request->cookie())
>>> - GPC raw: use $_GET, $_POST, and $_COOKIE directly
>>> - S: use $_SERVER directly
>>>
>>> Which means that:
>>> - Input::instance()->get($key) becomes $request->query($key)
>>> - Input::instance()->post($key) becomes $request->post($key)
>>> - Input::instance()->cookie($key) becomes $request->cookie($key)
>>> - Input::instance()->server($key) becomes $_SERVER($key)
>>> - Input::instance()->post($key, $default) becomes
>>> Arr::get($request->post(), $key, $default)    (and likewise for get,
>>> cookie, and server using defaults)
>>> - references to these in non-controllers needs to be moved to their
>>> respective controllers.  I've perused the code, and don't think this
>>> would be a big deal.
>>> - K3 and Formo should use our cleaned values by default :-)
>>
>>
>> So do K3 and Formo use the Request class or the superglobals?  I guess
>> that dictates what we do as well.  We should stick to their convention as
>> best as possible.
>>
>> Either way - this looks good to me.  Get the code in and let's start using
>> it!
>>
>>>
>>>
>>> ---
>>>
>>> Whew - that writeup took awhile!  Thoughts?
>>>
>>> Take care,
>>> Shad
>>>
>>>
>>>
>>> On 2 April 2013 18:23, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote:
>>> <snipped>
>>> >>>
>>> >>> Input
>>> >>>
>>> >>> This library is gone with K3.  Instead, they recommend working with
>>> >>> $_GET,
>>> >>> $_POST, and $_SERVER directly.  Handling unset and default values can
>>> >>> be
>>> >>> handled using Arr as:
>>> >>> Arr::get($_POST, "foo", "default foo");
>>> >>>
>>> >>> This part is pretty straightforward.  The only catch is that they
>>> >>> need
>>> >>> cleaning first.
>>> >>>
>>> >>> It was recommended to use Security::xss_clean() with K3.0, but that's
>>> >>> since disappeared.  I'm looking for the best alternative, which
>>> >>> shouldn't be
>>> >>> too hard to find, but think the bigger question is this: *where* do
>>> >>> we do
>>> >>> this so nobody downstream has to think about it?
>>> >>> - bootstrap?
>>> >>> - an extension of Route?
>>> >>> - an extension of Controller?
>>> >>> - somewhere else?
>>> >>
>>> >>
>>> >> The XSS cleaner in K2 was questionable at best.  We augmented it with
>>> >> our
>>> >> own tweaks:
>>> >>
>>> >> https://github.com/gallery/gallery3/blob/master/system/libraries/Input.php#L306
>>> >>
>>> >> Even that is pretty crappy and I don't trust it.  We really need to be
>>> >> very
>>> >> suspicious of any content in the database and treat it very carefully
>>> >> when
>>> >> we use it.  For K2 we overloaded html::clean to make it safe:
>>> >>
>>> >> https://github.com/gallery/gallery3/blob/master/modules/gallery/helpers/MY_html.php#L32
>>> >>
>>> >> Then we used that (and all the other cleaning methods) everywhere.  My
>>> >> assumption has always been that with html::clean AND the XSS cleaning
>>> >> code
>>> >> we don't have to catch every single vector of attack, we've got enough
>>> >> defense-in-depth to make hacking hard.
>>> >>
>>> >> The downside, though is that we are always possibly cleaning away
>>> >> something
>>> >> that we really care about.  And cleaning is expensive.  :-/
>>> >>
>>> >> So options:
>>> >> 1) Don't bother cleaning at all
>>> >> 2) Create our own simple API for getting GET/POST/SERVER variables
>>> >> that does
>>> >> cleaning and handles defaults
>>> >> 3) Clean everything up front
>>> >>
>>> >> I'm in favor of #2 with an API like this:
>>> >>
>>> >> Input::GET("foo", "default foo");
>>> >> Input::GET("foo", "default foo", Input::RAW);  // don't sanitize
>>> >> Input::POST("foo", "default foo");
>>> >> Input::SERVER("foo", "default foo");
>>> >>
>>> >
>>> > Me too.  I've been taking a look at how we'd integrate the standalone
>>> > HTMLPurifier, and it looks like we can be a bit faster if we use a
>>> > singleton instance() function.  With this, we could *exactly* copy the
>>> > old API (or at least the parts that Gallery used).  The instance would
>>> > initialize the class, fire up and configure HTMLPurifier once, and set
>>> > up an empty cache of purified entries.  Then, we purify them on demand
>>> > and stick them in the cache so we shouldn't need to do it twice.  I
>>> > haven't yet figured out if it makes sense to do them one superglobal
>>> > array at a time or one entry at a time, but certainly it doesn't make
>>> > sense to do all at once (for example, one might load Input to look for
>>> > $_POST but never care about $_SERVER).
>>> >
>>> > I also like the Input::RAW idea.  It should come with a big
>>> > disclaimer, and really shouldn't be needed too often since we can
>>> > accept HTML markup, but it's still nice to have.
>>> >
>>> >> This has some advantages:
>>> >> 1) It's close to what we have now so conversion is easier
>>> >> 2) We can lazy-clean the input for efficiency
>>> >> 3) We can use Input::RAW as a findable symbol for all places where we
>>> >> work
>>> >> with raw data (set the RAW constant to some wacky value so that API
>>> >> users
>>> >> have to use the constant)
>>> >> 4) profit?
>>> >
>>> > Yes, yes, yes, and hell yes.  :-)
>>> >
>>> >
>>> >> Regarding HTMLPurifier - it added so much weigh to the installed code
>>> >> size
>>> >> that I was loathe to make it a default module.  Unless it's gotten a
>>> >> lot
>>> >> smaller, I suspect I'll still feel the same way...
>>> >
>>> > The standalone version you found seems to be not *too* bad.  It's <1MB
>>> > (<300k compressed) and seems like it was designed to play nice with
>>> > caches.
>>> >
>>> > My feeling is this: we should build the Input class and make it use
>>> > HTMLPurifier.  Then, once we're back in fully-running state, we can
>>> > run some benchmarks to see how heavy it really is.
>>> >
>>> > Thoughts?
>>> >
>>> > Take care,
>>> > Shad
>>> >
>>> >
>>> >>
>>> >> thoughts?
>>> >> -Bharat
>>> >>
>>>
>>>
>>> ------------------------------------------------------------------------------
>>> Minimize network downtime and maximize team effectiveness.
>>> Reduce network management and security costs.Learn how to hire
>>> the most talented Cisco Certified professionals. Visit the
>>> Employer Resources Portal
>>> http://www.cisco.com/web/learning/employer_resources/index.html
>>> __[ g a l l e r y - d e v e l ]_________________________
>>>
>>> [ list info/archive --> http://gallery.sf.net/lists.php ]
>>> [ gallery info/FAQ/download --> http://gallery.sf.net ]
>>>
>>
>

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]