497109 - Update tikiwiki XSS sanitization on SUMO to current

Status: RESOLVED WONTFIX

(support.mozilla.org :: Knowledge Base Software, task)

Description

[E. Cooper [:ecooper]]

Tiki sanitizes all input from POST/GET on every request. The current method of XSS sanitization is run on every page load during the inclusion of webroot/tiki-setup_base.php. While it doesn't affect many pages in terms of processing time, webroot/tiki-editpage.php is taking a hit during article submission. On a local dev instance, processing time is at ~900 milliseconds on article submission (hitting save/preview in editor). 

TikiWiki is using an upgraded method in the current stable release, which is 3.0. We should see if backporting the current method will help with the performance issues SUMO is currently facing webroot/tiki-editpage.php.

If the sanitization method in current tiki is considerably faster, we'll see a bump in response times across SUMO.

Comment 1

[E. Cooper [:ecooper]]

The bleeding edge of webroot/tiki-setup_base.php can be found here: http://tikiwiki.svn.sourceforge.net/viewvc/tikiwiki/branches/3.0/tiki-setup_base.php?view=log

Comment 2

[E. Cooper [:ecooper]]

Attachment: List of files that need to be added to SVN

This is the output from svn status | grep '^A' to show what files need to be added to the svn repo to take advantage of the new sanitization method found in tiki 3.0. A majority of the Zend stuff isn't needed. If we want to only include what we need, we can.

Essentially, we need to include all the files from http://tikiwiki.svn.sourceforge.net/viewvc/tikiwiki/branches/3.0/lib/core/lib/ excluding Multilingual and WikiParser.

This introduces a lot of new code, so maybe it will be better to wait for the upgrade to optimize this?

Benchmarks and patch for webroot/tiki-setup_base.php coming shortly.

Comment 3

[E. Cooper [:ecooper]]

Attachment: v1 (webroot/tiki-setup_base.php patch)

So, the very basic benchmarks locally look like this:
- Cost to submit articles with RemoveXSS (SUMO uses): ~900 ± 100 milliseconds
- Cost to submit articles with TikiFilter (tiki 3.0 uses): ~500 ± 100 milliseconds

Benefits during basic article display and other pages range from negligible to 100 milliseconds. We are seeing about half a second increase on article submissions, though.

If a more complex set of benchmarks are needed, I can slap together a mini-report or something in the same vein.

Comment 5

[Laura Thomson :laura]

Comment on attachment 382311 [details] [diff] [review]
v1 (webroot/tiki-setup_base.php patch)

We need one change to avoid regressions.  
$_REQUEST = array_merge($_GET, $_POST);

We need to respect the ordering set in the PHP configuration variable variables_order.  This currently happens in lines 371 - 393.

Comment 6

[E. Cooper [:ecooper]]

(In reply to comment #5)
> (From update of attachment 382311 [details] [diff] [review])
> We need one change to avoid regressions.  
> $_REQUEST = array_merge($_GET, $_POST);
> 
> We need to respect the ordering set in the PHP configuration variable
> variables_order.  This currently happens in lines 371 - 393.

This still happens at 371-393 (well, plus the offset of the new code). If we just get rid of "$_REQUEST = array_merge($_GET, $_POST);" from the patch, we should be good, right?

I just want to make sure I'm not being dense, which I'll conveniently blame on the medicine.

Comment 7

[Laura Thomson :laura]

Yes, that's what we need to do (get rid of the array_merge() line)

Comment 8

[E. Cooper [:ecooper]]

Attachment: v2 (webroot/tiki-setup_base.php patch)

Just for reference, this is the patch that to webroot/tiki-setup_base.php that's going to be committed shortly (as discussed in the meeting today).

Comment 9

[E. Cooper [:ecooper]]

I thought I had posted this, but I guess I didn't in the chaos of yesterday. This is in r28322, trunk only.

Comment 10

[E. Cooper [:ecooper]]

These changes have been backed out in r28375 due to troubles with staging. See bug 500060.

webroot/tiki-setup_base.php has been reverted back to its last revision: http://viewvc.svn.mozilla.org/vc/projects/sumo/trunk/webroot/tiki-setup_base.php?r1=28375&r2=21587&pathrev=28375&limit_changes=0

Comment 11

[E. Cooper [:ecooper]]

Moving this to 1.3 while I try to diagnose the problem we're seeing on staging.

Comment 12

[Laura Thomson :laura]

We'll hold this for the next release, and perf test it.  (Sorry Reed :( )

Comment 13

[Paul Craciunoiu [:paulc]]

This still breaks staging, but not sumotools?

Comment 14

[Marc Laporte]

Related info:

"Beginning with 3.0, more control over the filtering is possible. The very broad filter is now used as a last resort in cases where no specific filters are specified. Specific filters are even more secure and can significantly improve performance."

http://dev.tikiwiki.org/FilteringBestPractices

Comment 15

[David Tenser [:djst]]

This is backported from TW 3.0, so it's already in upstream TW.