Bug 49845

Summary: XSS Auditor severely affects loading performance after submitting a large form
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: abarth, adele, dbates, sam, tonikitoo
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 53200, 53205, 53266, 53279, 53336, 53338, 53339, 53345, 53354, 53362, 53364, 53365, 53368, 53370, 53635, 53640, 53643, 53652, 53662, 53664, 53665, 53741, 53750    
Bug Blocks: 53405    
Attachments:
Description Flags
test case
none
Patch dbates: review+

Description Alexey Proskuryakov 2010-11-19 17:11:17 PST
A certain enterprise application has severely degraded performance in Safari, and it turned out that this is due to XSSAuditor checks.

The problem occurs after the application submits a form - the result loads really slow, beachballing Safari. This happens because the form is huge (about 170Kbytes). Creating a SuffixTree from it takes a long time. In addition, the tree isn't effectively cached (it's re-created each time you go from external script to inline script to an event listener attribute).

Perhaps one doesn't need to create a SuffixTree if form data is longer than the script being executed? Or maybe there is an even better solution?

Of course, one can disable XSS Auditor with HTTP headers as a temporary workaround.

<rdar://problem/8546193>
Comment 1 Adam Barth 2010-12-01 12:53:56 PST
If we cached the tree better, would that fix the issue?
Comment 2 Alexey Proskuryakov 2010-12-01 12:59:53 PST
The memory impact of having more than one copy of the tree doesn't appear negligible. Even one copy of a tree for a 170K form is probably not negligible.
Comment 3 Adele Peterson 2011-01-17 15:32:10 PST
We're seeing other reports of this slowness with internal website.
Comment 4 Adam Barth 2011-01-17 16:10:07 PST
Having a test case would be helpful here.  I can try to create one based on the description in Comment #0.  It seems like a reasonable first step is to improve the caching of the suffix tree.
Comment 5 Alexey Proskuryakov 2011-01-18 14:18:08 PST
Created attachment 79330 [details]
test case

I can't easily share or even access actual data from the internal web applications, but here is a synthetic test case. Works fast in Firefox, of course.
Comment 6 Adam Barth 2011-01-18 14:25:34 PST
Thanks Alexey.
Comment 7 Adam Barth 2011-01-24 16:49:48 PST
The proximate cause is that the CachingURLCanonicalizer isn't sufficiently associative.  Alternating inline event handlers and inline script flips the decodeEntities bit back and forth, causing the cache to thrash.

A short-term fix is to make the cache more associative.  A long-term fix is to move to a new XSS detection architecture, which I've wanted to do for a while...

I'm going to try for the short term fix first.
Comment 8 Adam Barth 2011-01-26 10:43:32 PST
We talked on IRC.  I'm going to try the long-term fix first.  I'll post incremental patches blocking this bug.
Comment 9 Adam Barth 2011-02-03 00:50:07 PST
With the patches up through Bug 53665, this test case is now blazingly fast.
Comment 10 Adam Barth 2011-02-03 19:46:39 PST
Created attachment 81176 [details]
Patch
Comment 11 Adam Barth 2011-02-03 19:47:03 PST
I'll add the PerformanceTest in a follow-up patch.
Comment 12 Daniel Bates 2011-02-03 19:52:36 PST
Comment on attachment 81176 [details]
Patch

r=me
Comment 13 Adam Barth 2011-02-03 19:53:56 PST
Some discussion in IRC:

[7:50pm] dydz: abarth: Some of the expected results have more console messages than before...
[7:50pm] abarth: yeah
[7:50pm] abarth: that's because we block injected "type" attributes
[7:51pm] abarth: for objects now too
[7:51pm] abarth: in XSSAuditor, you could inject an object tag for Gears
[7:51pm] abarth: for example
[7:51pm] abarth: because that doesn't result in a load
[7:51pm] abarth: whereas XSSFilter will block that
[7:52pm] abarth: dydz: i think there's some kind of attack scenario there involving a plugin with a non-standard way of loading stuff
[7:52pm] abarth: dydz: but i don't have a concrete example
Comment 14 Adam Barth 2011-02-03 19:56:56 PST
Committed r77588: <http://trac.webkit.org/changeset/77588>
Comment 15 Alexey Proskuryakov 2011-02-14 11:47:51 PST
> Of course, one can disable XSS Auditor with HTTP headers as a temporary workaround.

Ugh, it turns that one cannot do that with WebKit as released with shipping Safari. XSSAuditor::findInRequest() only checks for X-XSS-Protection header after performing all time consuming operations!
Comment 16 Adam Barth 2011-02-14 13:35:00 PST
(In reply to comment #15)
> > Of course, one can disable XSS Auditor with HTTP headers as a temporary workaround.
> 
> Ugh, it turns that one cannot do that with WebKit as released with shipping Safari. XSSAuditor::findInRequest() only checks for X-XSS-Protection header after performing all time consuming operations!

That's unfortunate.  :(

The new design shouldn't have that problem...