Bug 49845 - XSS Auditor severely affects loading performance after submitting a large form
Summary: XSS Auditor severely affects loading performance after submitting a large form
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords: InRadar
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
Blocks: 53405
  Show dependency treegraph
 
Reported: 2010-11-19 17:11 PST by Alexey Proskuryakov
Modified: 2011-02-14 13:35 PST (History)
5 users (show)

See Also:


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+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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...