Re: HTMLPurifier and the "Input" class

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 ]