Bug 49845 – XSS Auditor severely affects loading performance after submitting a large form

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Alexey Proskuryakov]

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>

[Adam Barth]

If we cached the tree better, would that fix the issue?

[Alexey Proskuryakov]

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]

We're seeing other reports of this slowness with internal website.

[Adam Barth]

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]

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]

Thanks Alexey.

[Adam Barth]

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]

We talked on IRC.  I'm going to try the long-term fix first.  I'll post incremental patches blocking this bug.

[Adam Barth]

With the patches up through Bug 53665, this test case is now blazingly fast.

[Adam Barth]

Created attachment 81176 [details]
Patch

[Adam Barth]

I'll add the PerformanceTest in a follow-up patch.

[Daniel Bates]

Comment on attachment 81176 [details]
Patch

r=me

[Adam Barth]

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]

Committed r77588: <http://trac.webkit.org/changeset/77588>

[Alexey Proskuryakov]

> 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]

(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...