Summary: | XSSAuditor bypass under big5 encoding (also sjis). | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Thomas Sepez <tsepez> | ||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, ap, dbates, webkit.review.bot | ||||||
Priority: | P2 | Keywords: | XSSAuditor | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 66588 | ||||||||
Bug Blocks: | 66579 | ||||||||
Attachments: |
|
Description
Thomas Sepez
2011-08-29 10:13:41 PDT
I've written a test for this, but it manifests only under --chromium. There's a happy co-incidence that generally masks this on the KURL.cpp side: namely what happens when a bad sequence is encountered. As indicated above, the KURLGoogle path is something like: %c9%5f => 00c9 005f => c3 89 5f => [ c389 big5 char ] 5f => [ hi unicode char ] 5f => 5f Where each byte of the invalid sequence is passed through as the equivlaent codepoint (after first arrow above). The KURL path is something like: %c9%5f => fffd 005f => ef bf bd 5f => [ efbf big 5 char] [ bd5f big5 char] => [hi unicode][hi unicode]=> empty string where each byte of the invalid sequence is replaced by the unicode replacement character U+fffd. The co-incidence is that during the KURL case, the replacement codepoint turns into an odd number of utf8 bytes (after the second arrow) but the KURLGoogle replacement code point turns into an even number of utf8 bytes. Hence the byte for the following 5f gets eaten as part of a second big5 character in one case, but gets passed through as-is in the other. There will be variations involving other charsets that don't hit this happy co-incidence, but this explains why the test case I was working on passed under the normal build. Have constructed an example in shift_jis that will flunk against the present code but is expected to pass once the issue is fixed. We believe the fix to https://bugs.webkit.org/show_bug.cgi?id=66588 as proposed by Daniel Bates will cover this case. I'm just uploading the two prospective tests while waiting for that bug to close. Created attachment 106655 [details]
Proposed tests.
Just to get code up for review even though we can't land at present.
These now passed for me on mac under both normal and --chromium cases following the fix to 66588. Comment on attachment 106655 [details] Proposed tests. Can we write these tests using some kind of Perl script that echos back the input data instead of hardcoding the response page? Adding such a script or modifying an existing script will make it easier to extend our test coverage for such encoding issues. If it's not possible to write such a script please elaborate on why. Take a look at the Encode module <http://perldoc.perl.org/Encode.html> for encoding text. I'm just concerned about the correct functioning of the test on any given platform depending on the correctness of the perl implementation on that platform. Aren't there already a bunch of these xssauditor tests that flunk only on certain ports? Perhaps because the cgi implementation does some unexpected transformation or the perhaps the encoding doesn't come back as expected. I guess I want a test that says when these bytes go up, and these bytes come back, this is what happens. Created attachment 106895 [details]
patch using perl for testcase.
Updated echo-intertag.pl to allow specification of charset in content-type header. We don't decode the strings because we want the exact byte sequence returned as passed in escaped in the q parameter. This appears to be the case when CGI is invoked without the utf8 option, treating the parameter as if it were a binary string.
Comment on attachment 106895 [details]
patch using perl for testcase.
Thanks Tom. This looks good to me.
Comment on attachment 106895 [details] patch using perl for testcase. Clearing flags on attachment: 106895 Committed r94884: <http://trac.webkit.org/changeset/94884> All reviewed patches have been landed. Closing bug. |