HTMLPurifier and the "Input" class

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 ]