Re: HTMLPurifier and the "Input" class

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 ]