381412 - Avoid XSS with incomplete sequences in multibyte character sets

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[u217243]

Yosuke Hasegawa at WebAppSec.jp reported a bug in the Mozilla parser through JPCERT/CC.

The parser ignores control character '0x80' in HTML tag attributes if character encoding of the page is Shift_JIS commonly used in Japan, and allows Cross-Site Scripting attacks in some cases. Scripts cannot be run if the charset is not Shift_JIS.

He posted the POC here:
http://openmya.hacker.jp/hasegawa/PoC/firefox-0x80.html

It contains the following codes:
<div [0x80]onmouseover="alert('xss');">aaa</div>
<[0x80][0x80]s[0x80][0x80]c[0x80]r[0x80]i[0x80]p[0x80]t[0x80]>[0x80]document.write('[0x80]xss');</script>

Comment 1

[Jesse Ruderman]

Is the HTML parser ignoring characters or is the Shift_JIS decoder ignoring bytes?

Comment 2

[Daniel Veditz [:dveditz]]

I doubt it's the parser because we recently added checks for stuff like this

Comment 3

[Daniel Veditz [:dveditz]]

If it's in the charset converter, are similar issues lurking in scripts other than shift-JIS?

Comment 4

[Simon Montagu :smontagu]

This looks to me like a regression from bug 108136, and probably applies to all Japanese codepages.

Comment 5

[Simon Montagu :smontagu]

Attachment: Minimal patch

This fixes the testcase, but I want to do more investigation in other codepages and regression testing.

Comment 6

[Simon Montagu :smontagu]

Attachment: Similar trick in ISO-2022-JP

Unlike the original, this one also works in IE, and I think it will be somewhat harder to fix. It uses ISO-2022-JP escape sequences to create zero-length JIS X 0208 runs between ASCII characters.

Comment 7

[Simon Montagu :smontagu]

Attachment: And the same in hz-gb2312

For extra points, this one works in Mozilla, IE and Safari

Comment 8

[Simon Montagu :smontagu]

Attachment: Patch

I audited all the DBCS and MCBS encodings in intl/uconv/ucvja ucvcn ucvko ucvtw and ucvtw2 for similar holes. This patch fixes:
The originally reported flaw with 0x80 in Shift-JIS
Assertions with out-of-range bytes in HZ-GB-2312
Zero-length non-ASCII runs in ISO-2022-JP (attachment 267398 [details])
Zero-length non-ASCII runs in ISO-2022-CN
Zero-length non-ASCII runs in ISO-2022-KR
Zero-length non-ASCII runs in HZ-GB-2312 (attachment 268763 [details]).

Comment 9

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 10

[Jungshik Shin]

Comment on attachment 269066 [details] [diff] [review]
Patch

r+. Sorry for the terrible delay. 

>Index: intl/uconv/ucvja/nsJapaneseToUnicode.cpp
>===================================================================

>+++ intl/uconv/ucvja/nsJapaneseToUnicode.cpp	20 Jun 2007 11:55:10 -0000
>@@ -132,18 +132,22 @@ NS_IMETHODIMP nsShiftJISToUnicode::Conve
>                  if(0xFFFD == mData) {
>                    // IE convert fd-ff as single byte and convert to
>                    // U+f8f1 to U+f8f3
>                    if((0xfd == *src) || (0xfe == *src) || (0xff == *src))
>                    {
>                      *dest++ = (PRUnichar) 0xf8f1 + 
>                                    (*src - (unsigned char)(0xfd));
>                      if(dest >= destEnd)
>                         goto error1;
>+                   } else {
>+                     *dest++ = 0x30FB;
>+                     if(dest >= destEnd)
>+                       goto error1;

Does IE also map 0x80 to U+30FB?  If so, please mention that. BTW,  can we do this and 0xFD - 0xFF mapping regardless of the value of intl.jis0208.map?

Comment 11

[Simon Montagu :smontagu]

Attachment: addressed review comments

> Does IE also map 0x80 to U+30FB?  If so, please mention that. 

IE maps 0x80 to itself, and 0xA0 to U+F8F0. I've changed the patch to follow this.

> BTW,  can we do
> this and 0xFD - 0xFF mapping regardless of the value of intl.jis0208.map?  

I've tested that that's what we do already.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 278301 [details] [diff] [review]
addressed review comments

sr=dveditz

Comment 13

[Simon Montagu :smontagu]

Checked in, with testcases.

Comment 14

[Daniel Veditz [:dveditz]]

Simon says IE, Opera and Safari are also affected

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 278301 [details] [diff] [review]
addressed review comments

approved for 1.8.1.7 and 1.8.0.14, a=dveditz for release-drivers

Comment 16

[Daniel Veditz [:dveditz]]

Microsoft and Opera both asked for us to delay disclosure of this flaw while they address it, both have releases early-to-mid December coming up so tentatively that's when we can announce this (along with FF2009 perhaps).

Comment 17

[Daniel Veditz [:dveditz]]

I re-pinged Apple for a status update (so far I've only gotten the initial "The issue is being investigated" reply). Kohei suggested I post their tracking number here.

> Please include the line below in follow-up emails for this request.
> Follow-up:  34991641

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: Thunderbird 2.0.0.9 counldn't decode the subject at all

Why ISO-2022-JP decoder fails if it encounters a empty sequence?
Some Japanese mail subjects are *completely* garbled even if the empty sequence is at end of the subject.
Other encoders seem to fallback to U+FFFD. Why ONLY the Japanese encoder fails?

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Testcase using ESC ( J

Moreover, the fix is incomplete.
I could cause XSS attack against Firefox 2.0.0.9 using empty sequences.

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: Patch

This will fix both problems.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 289168 [details] [diff] [review]
Patch

Could you add an automated testcase to this patch, so this problem doesn't accidentally reappear in the future?

Comment 22

[Masatoshi Kimura [:emk]]

Attachment: automated test

Comment 23

[Mike Schroepfer]

Comment on attachment 289168 [details] [diff] [review]
Patch

Clearing the approval1.9 flag because reviews are not complete.  once you get code review please re-nominate

Comment 24

[Masatoshi Kimura [:emk]]

Attachment: patch incorporated into a test

Comment 25

[Masatoshi Kimura [:emk]]

Fx 2.0.0.10 released

Comment 26

[Simon Montagu :smontagu]

Comment on attachment 290136 [details] [diff] [review]
patch incorporated into a test

>             } else if ('J' == *src)  {
>               mState = mState_JISX0201_1976Roman;
>+              if (mRunLength == 0) {
>+                if((dest+1) >= destEnd)
>+                  goto error1;
>+                *dest++ = 0xFFFD;
>+              }

This is going to put in U+FFFD whenever we shift from ASCII to JIS-X Roman, no? That seems wrong to me. Maybe the condition should be
  if (mRunLength == 0 && mLastLegalState != mState_ASCII)
?

>             } else {
>               // XXX We need to  decide how to handle \ and ~ here
>               // we may need a if statement here for '\' and '~' 
>               // to map them to Yen and Overbar
>               *dest++ = (PRUnichar) *src;
>-              ++mRunLength;

And removing this line is going to cause U+FFFD whenever we shift back from JIS-X Roman to ASCII

Comment 27

[u217243]

Some Thunderbird users, including myself, have confirmed the garbled message subjects since 2.0.0.9.

Comment 28

[Masatoshi Kimura [:emk]]

(In reply to comment #26)
> (From update of attachment 290136 [details] [diff] [review])
> This is going to put in U+FFFD whenever we shift from ASCII to JIS-X Roman, no?
> That seems wrong to me. Maybe the condition should be
>   if (mRunLength == 0 && mLastLegalState != mState_ASCII)
> ?
I know it's technically wrong, but we have no choice to avoid XSS with sequnces such as "<" ESC "(" "J" "script".

> And removing this line is going to cause U+FFFD whenever we shift back from
> JIS-X Roman to ASCII
Ditto. it's required to avoid XSS with sequences such as ESC "(" "J" "<" ESC "(" "B" "script".

And of course, we need to avoid "JIS-X Roman to JIS-X Roman" sequences.
Do you have a good idea to prevent these attacks?

Comment 29

[Daniel Veditz [:dveditz]]

Microsoft found regressions in their patch and won't be releasing their fix until February. Please embargo until at least then.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 278301 [details] [diff] [review]
addressed review comments

Minusing this patch for the 1.8.0 branch due to Thunderbird regressions.

Comment 31

[Masatoshi Kimura [:emk]]

Attachment: resolved review comment

I removed a testcase because this version will FAIL the test.

Comment 32

[Masatoshi Kimura [:emk]]

Attachment: new manual testcase

New patch is vulnerable to this test.
I'll file a followup bug about this problem when this bug is resolved again.

Comment 33

[Simon Montagu :smontagu]

I'm sorry, I need more time to think about "the right thing" to do here. We should certainly fix the Thunderbird regression, but I'm not sure what to about the vulnerabilities with ASCII to JIS-X Roman sequences. Unlike the sequences that my original patch was designed to prevent, these sequences are completely legal ISO-2022-JP

Comment 34

[Masatoshi Kimura [:emk]]

(In reply to comment #33)
Your patch refuses ASCII to ASCII sequences that are also completely valid per BNF defined in RFC 1468.
There is no point to allow switching between JIS Roman and ASCII.
BTW, my new patch will fix only the Thunderbird regression which is urgent for Japanese users. So I proposed filing a new bug to consider about remaining problem.
Another BTW, bug 406777 may become a solution.

Comment 35

[Simon Montagu :smontagu]

Comment on attachment 291406 [details] [diff] [review]
resolved review comment

OK, that's fair enough. r=smontagu

Comment 36

[Daniel Veditz [:dveditz]]

Covering the regression in this bug obscures the fact that real problems were fixed in previous releases, including charsets that do not appear to suffer from this regression. I filed bug 407161 to cover the regression and to track landing the patch we're reviewing in this bug (future patches, if necessary, should go in the regression bug).

(In reply to comment #32)
> New patch is vulnerable to this test.
> I'll file a followup bug about this problem when this bug is resolved again.

If it's not going to be handled along with this patch (now bug 407161) go ahead and file the new bug now and make it block 407161. And I guess in that case remove the 'XSS variant' bit from the bug 407161 title

Comment 37

[Daniel Veditz [:dveditz]]

Comment on attachment 291406 [details] [diff] [review]
resolved review comment

sr=dveditz

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 291406 [details] [diff] [review]
resolved review comment

copied patch into regression bug for purposes of approval, not sure it'll get found correctly here (see "Dan misses follow-up patch and causes FF2.0.0.11" debacle).

Comment 39

[u217243]

Any updates from other vendors?

Comment 40

[Masatoshi Kimura [:emk]]

(In reply to comment #39)
> Any updates from other vendors?
Before that, did we prepare to make the vulnerability public?
The issue I mentioned in comment #28 is not fixed yet.
Is it appropriate to filter out completely valid sequences?