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+

Alexey Proskuryakov
Reported 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>
Attachments
test case (1.71 KB, application/zip)
2011-01-18 14:18 PST, Alexey Proskuryakov 		no flags
Details
Patch (16.28 KB, patch)
2011-02-03 19:46 PST, Adam Barth 		dbates: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2010-12-01 12:53:56 PST
If we cached the tree better, would that fix the issue?
Alexey Proskuryakov
Comment 2 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.
Adele Peterson
Comment 3 2011-01-17 15:32:10 PST
We're seeing other reports of this slowness with internal website.
Adam Barth
Comment 4 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.
Alexey Proskuryakov
Comment 5 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.
Adam Barth
Comment 6 2011-01-18 14:25:34 PST
Thanks Alexey.
Adam Barth
Comment 7 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.
Adam Barth
Comment 8 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.
Adam Barth
Comment 9 2011-02-03 00:50:07 PST
With the patches up through Bug 53665, this test case is now blazingly fast.
Adam Barth
Comment 10 2011-02-03 19:46:39 PST
Created attachment 81176 [details] Patch
Adam Barth
Comment 11 2011-02-03 19:47:03 PST
I'll add the PerformanceTest in a follow-up patch.
Daniel Bates
Comment 12 2011-02-03 19:52:36 PST
Comment on attachment 81176 [details] Patch r=me
Adam Barth
Comment 13 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
Adam Barth
Comment 14 2011-02-03 19:56:56 PST
Committed r77588: <http://trac.webkit.org/changeset/77588>
Alexey Proskuryakov
Comment 15 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!
Adam Barth
Comment 16 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...
Note You need to log in before you can comment on or make changes to this bug.