Bug 26921 – [XSSAuditor] HTML entities can bypass xssAuditor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Adam Barth]

Splitting off from Bug 26708.  We still need to teach the XSSAuditor about HTML entities.  I have a patch building.  I'll post it in the morning.  (Boo for slow computers.)

HTML entities:

test.php?x=%3Ca%20href%3D%26%23x0000006a%26%23x61%26%23x76%26%23x61%26%23x73%26%23x63%26%23x72%26%23x69%26%23x70%26%23x74%26%23x3a%26%23x61%26%23x6c%26%23x65%26%23x72%26%23x74%26%23x28%26%23x31%26%23x29%3Etest%3Ca%3E

test.php?x=%3Cimg%20src=1%20onerror=%26%2397%26%23108%26%23101%26%23114%26%23116%26%2340%26%2349%26%2341%3E

[Adam Barth]

Created attachment 32183 [details]
Not quite right patch

[Adam Barth]

Dan, do you want to look at making this patch work properly?  I'm having trouble getting XCode to work.

[Daniel Bates]

Sure, I'll take a look.

(In reply to comment #2)
> Dan, do you want to look at making this patch work properly?  I'm having
> trouble getting XCode to work.

[Daniel Bates]

Created attachment 32602 [details]
Working patch with tests

Modified initial patch XSSAuditor::decodeHTMLEntities to more closely match the functionality in HTMLTokenizer for handling illegal entities by not decoding them (for example: HTMLTokenizer does not substitute '\0' for &#00, &#x00, but the PreloadScanner, used by XSSAuditor::decodeHTMLEntities, does). To get similar behavior, I make a copy of SegmentedString |source| called sourceShadow before calling the PreloadScanner. If the PreloadScanner returns and invalid entity e == 0xFFFD, then I swap |source| and |sourceShadow|. Maybe there is a more efficient way to achieve the same result?

The list of parameters to findInRequest, decodeURL are becoming unwieldy. The code should be cleaned up, but this may be better to do in a separate bug.

[Adam Barth]

Comment on attachment 32602 [details]
Working patch with tests

This is fine for now.  Now that we've worked through all the known issues, it's time to do a clean up patch for the auditor.  There are some nits that I'd change with this patch, but we can deal with them in the cleanup patch.

Thanks for the thorough test cases.  That work is about to pay off.

[Adam Barth]

Transmitting file data ....................
Committed revision 45752.