407161 - Garbled Japanese after bug 381412, XSS variant still possible

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[Daniel Veditz [:dveditz]]

This bug covers regressions from bug 381412 where Japanese Thunderbird users are suffering from garbled subject lines (bug 381412 comment 18). There is also still an XSS variant still present (bug 381412 comment 19) that I'm unsure is best handled as part of fixing real-world Japanese mail subjects (this regression) or in a separate bug. It sounds like (bug 381412 comment 32) that it should be a separate bug.

Sorry to duplicate so many comments, but the bug-morph was making it hard to track what was and was not fixed by the other bug.

-----------------------------------------------------------
--Comment #19 Masatoshi Kimura (:emk)   2007-11-17 11:58:12 PST--

Created an attachment (id=289166) [details]
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 [reply] Masatoshi Kimura (:emk)  2007-11-17 11:59:06 PST--

Created an attachment (id=289167) [details]
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 [reply] Masatoshi Kimura (:emk)  2007-11-17 12:00:52 PST--

Created an attachment (id=289168) [details]
Patch

This will fix both problems.

--Comment #26 [reply] Simon Montagu  2007-11-28 13:09:56 PST--

(From update of attachment 290136 [details] [diff] [review])
>             } 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 [reply] Kohei Yoshino  2007-11-28 21:05:58 PST--

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

--Comment #28 [reply] Masatoshi Kimura (:emk)  2007-11-29 04:38:46 PST--

(In reply to comment #26)
> (From update of attachment 290136 [details] [diff] [review] [details])
> 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 #31 [reply] Masatoshi Kimura (:emk)  2007-12-04 03:11:03 PST--

Created an attachment (id=291406) [details]
resolved review comment

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

--Comment #32 [reply] Masatoshi Kimura (:emk)  2007-12-04 03:13:19 PST--

Created an attachment (id=291407) [details]
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 [reply] Simon Montagu  2007-12-04 13:34:46 PST--

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 [reply] Masatoshi Kimura (:emk)  2007-12-05 04:31:42 PST--

(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 [reply] Simon Montagu  2007-12-05 12:10:25 PST--

(From update of attachment 291406 [details] [diff] [review])
OK, that's fair enough. r=smontagu

Comment 1

[Daniel Veditz [:dveditz]]

This should block 1.9 but I am insufficiently powered to carry the flag over from bug 381412.

Comment 2

[Daniel Veditz [:dveditz]]

(In reply to bug 381412 comment #34)
> Another BTW, bug 406777 may become a solution.

How would preventing a parent frame's user-selected charset from overriding a child-frame's charset have anything to do with this bug? Or even the suggested "remove UTF-7 support" incomplete solution?

Comment 3

[Daniel Veditz [:dveditz]]

Attachment: regression patch copied from bug 381412

Copying emk's patch for the regression from the original bug to this bug so approval requests will be visible to trunk triagers. Carrying over smontagu's r= and my sr=

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 291904 [details] [diff] [review]
regression patch copied from bug 381412

a=drivers for when trunk opens after Fx3 Beta 2 freeze

Comment 5

[Damon Sicore (:damons)]

+'ing P3.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can this be landed now?

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to comment #6)
Tree is open. It can be landed on trunk and 1.9 branch (It is not approved for 1.8.1.12 yet). Would you check in the patch?

Comment 8

[Daniel Veditz [:dveditz]]

Needs to land on trunk so we can check for regressions before we land on the branch. Although this is intended to fix mail subject lines any unintended side-effects would potentially be visible in the browser.

Comment 9

[Daniel Veditz [:dveditz]]

Checked in r1.31 to trunk.

Comment 10

[Daniel Veditz [:dveditz]]

Need to verify this fix on the trunk and check for regressions

Comment 11

[Masatoshi Kimura [:emk]]

-> v. using version 3.0a1pre (2007121903)

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 291904 [details] [diff] [review]
regression patch copied from bug 381412

approved for 1.8.1.12, a=dveditz for release-drivers

Let's get this landed and have the Japanese guys use nightlies for general surfing to make sure this doesn't regress anything with normal Japanese sites.

Comment 13

[Masatoshi Kimura [:emk]]

A Japanese tester reported the problem was fixed.
No regressions are reported. Would you land this on the branch?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to 1.8 branch.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Who can check this in the 1.8 branch?

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to comment #15)
> Who can check this in the 1.8 branch?
It was already landed on the branch.
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/intl/uconv/ucvja&command=DIFF_FRAMESET&file=nsJapaneseToUnicode.cpp&rev1=1.29.8.1&rev2=1.29.8.2&root=/cvsroot
Did you mean 1.8.0 branch?

Comment 17

[Al Billings [:abillings - ex-MoCo]]

I mean verify that the fix works ("check the fix") as opposed to checking in the fix.

The test cases aren't clear in comment 0. 

Comment 18

[Masatoshi Kimura [:emk]]

See bug 381412 commment #18.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

I've verified with the testmail.eml file from bug 381412 for 1.8.1.12.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12pre) Gecko/2008020403 Thunderbird/2.0.0.12pre

Comment 20

[Alexander Sack]

Comment on attachment 291904 [details] [diff] [review]
regression patch copied from bug 381412

a=asac for 1.8.0.15

(unmodified distro patch)

Comment 21

[Reed Loden [:reed]]

MOZILLA_1_8_0_BRANCH:

Checking in intl/uconv/ucvja/nsJapaneseToUnicode.cpp;
/cvsroot/mozilla/intl/uconv/ucvja/nsJapaneseToUnicode.cpp,v  <--  nsJapaneseToUnicode.cpp
new revision: 1.29.16.2; previous revision: 1.29.16.1
done