Re: HTMLPurifier and the "Input" class

Quick correction:

> - 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.

This isn't true.  In controllers, it's:
$request->post()

And in non-controllers, it's:
Request::$current->post()

Take care,
Shad


On 5 April 2013 17:29, 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.
>
> ---
>
> 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
> - 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.
> - (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.
>
> 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 :-)
>
> ---
>
> 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 ]