Re: HTMLPurifier and the "Input" class

I just reviewed the Purifier pull request and it looks pretty good.  I'm
not sure which approach to go with either - having Purifier as a module
definitely has some drawbacks especially because we're really considering
it a mandatory and non-overridable part of the codebase.  But let's try it
as a module for now and see where it leads.  Looking good so far!


On Sat, Apr 6, 2013 at 9:00 AM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote:

> Hey Bharat,
>
> Congrats on getting a controller going!  I feel like we're getting
> close to having this thing run...
>
> Big picture updates:
> - I went ahead and added formo and pagination.  Since they were
> entering into the conversation, now seemed like a good time to do this
> so we can view them more easily.  Of course, I also removed forge and
> kohana23_compat.
> - I closed the previous pull request for the Input class draft.
> - I opened a new pull request here:
> https://github.com/gallery/gallery3/pull/274
>
> Looking through more code, it does indeed seem like the preferred way
> to interact with $_GET, $_POST, and $_COOKIE is via the request.  This
> is true almost everywhere in the K3 core (except URL::query()) and
> also holds true for...
> - Formo. See Formo::save() and Formo::load(), the later of which
> defaults to request but will accept anything.  This makes
> Formo::load() a good place to reinject raw post values for modules
> that require it.
> - Pagination: See Pagination::setup(), Pagination::url(), and
> Pagination::query(), the last of which is described as a "URL::query()
> replacement for Pagination use only"
>
> That said, *relying* on everyone to do so (and requiring
> Gallery::ready() to jump through hoops to get there) doesn't seem like
> a great policy.  Rather, it seems like an accident waiting to happen.
>
> So, I agree with you: we should overwrite the superglobals after
> they're cleaned, then provide the raw values separately.  Next
> question: how?
>
> --
>
> I spent a lot of time thinking about this yesterday and today, and
> think I've come up with a pretty good solution (see pull referenced
> above).  Here's the gist of it:
> - move purifier code to its own module
> - load this module in bootstrap right after Kohana::init() (and
> therefore after Kohana::sanitize()) but before anything else
> - make a RAW class that defines three static variables
> - clean the superglobals and fill the RAW variables with the
> pre-HTMLPurifier values
>
> Then downstream, we can access the values using:
> - Request::$current->query(), Request::$current->post(), and
> Request::$current->cookie()  (clean; preferred method)
> - $_GET, $_POST, and $_COOKIE (clean, except cookie signing isn't checked)
> - RAW::$_GET, RAW::$_POST, and RAW::$_COOKIE (not HTMLPurified)
>
> Additionally, I've beefed up the Purifier class so module devs can
> load new instances of HTMLPurifier with a different config set.  So,
> to re-process form data, you should be able to do something like this:
>
> Purifier::add_config_group("foo", $foo_settings);  // $foo_settings is
> an array of HTMLPurifier config values
> $bar = Purifier::purify(RAW::$_POST["bar"], "foo");   // purifies bar
> using the foo settings
> Request::$current->post("bar", $bar);  // updates the bar value
> Formo::load();  // uses the new bar value
>
> Summary: we have a nice default path that just works, with enough
> extras to make it pretty extensible.  Kinda cool, eh?
>
> --
>
> Next question: why did I decide to use a separate purifier module and
> load it before everyone else?
>
> I struggled with this a bit, and it's a big cart-before-the-horse
> problem.  It's hard to be first while also being relatively flexible
> to everyone else's whim, it's hard to be special while still mostly
> fitting in, and it's kind of a pain that the superglobals don't work
> with variable variable names (i.e. $foo = "_GET"; $$foo = $something).
>
> I quickly came to the conclusion that I needed to load *something*
> before everyone else (the special part), and it was best if that
> something was a Kohana-compatible module (the fitting in part).  I had
> three options:
> - use the gallery module.  This reduces our module count by one, but
> also doesn't neatly divide out our Purifier class (which is special in
> that, since it's first, it can't be transparently extended).  This
> didn't feel right.
> - use the application folder.  This segregates it nicely and removes
> the need to call Kohana::modules() an extra time, but also makes our
> application folder not so spartan.  This also didn't feel right.
> - use a new purifier module.  This segregates the code, calls it out
> as special (while still mostly fitting in), and leaves our spartan
> application folder intact.
>
> So, I went with option 3.
>
> --
>
> Anyway, 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 ]