Re: HTMLPurifier and the "Input" class

Hey everyone,

Update: I tweaked/debugged the pull a bit more, and it seems to work!
https://github.com/gallery/gallery3/pull/274

As a quick test, I hacked some var_dump() calls into the bootstrap and
saw how the raw and clean values are populated for various get queries
(e.g. "index.php?foo|bar=hello_w<o>rld" gives "foo_bar=hello_w<o>rld"
as raw and "foo_bar=hello_wrld" as clean).

Thoughts?

Take care,
Shad

On 6 April 2013 18:00, 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 ]