Garbled Japanese after bug 381412, XSS variant still possible

VERIFIED FIXED

Status

()

Core
HTML: Parser
P2
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: dveditz, Assigned: emk)

Tracking

(4 keywords)

unspecified
fixed1.8.0.15, jp-critical, regression, verified1.8.1.12
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.12 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low] Seen in real-world Thunderbird subject lines)

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
This should block 1.9 but I am insufficiently powered to carry the flag over from bug 381412.
Flags: blocking1.9?
Flags: blocking1.8.1.12+
Flags: blocking1.8.0.15?
Summary: Garbled JA Tbird subjects after bug 381412, XSS variant still possible → Garbled Japanese after bug 381412, XSS variant still possible
Whiteboard: Seen in real-world Thunderbird subject lines
(Reporter)

Comment 2

11 years ago
(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?
(Reporter)

Comment 3

11 years ago
Created attachment 291904 [details] [diff] [review]
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=
Attachment #291904 - Flags: superreview+
Attachment #291904 - Flags: review+
Attachment #291904 - Flags: approval1.9?
Attachment #291904 - Flags: approval1.8.1.12?
Comment on attachment 291904 [details] [diff] [review]
regression patch copied from bug 381412

a=drivers for when trunk opens after Fx3 Beta 2 freeze
Attachment #291904 - Flags: approvalM10-
Attachment #291904 - Flags: approval1.9?
Attachment #291904 - Flags: approval1.9+
+'ing P3.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Can this be landed now?
(Assignee)

Comment 7

11 years ago
(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?
(Reporter)

Comment 8

11 years ago
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.
(Reporter)

Comment 9

11 years ago
Checked in r1.31 to trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite?
(Reporter)

Comment 10

11 years ago
Need to verify this fix on the trunk and check for regressions
Keywords: qawanted, verifyme
(Assignee)

Comment 11

11 years ago
-> v. using version 3.0a1pre (2007121903)
Status: RESOLVED → VERIFIED
(Assignee)

Updated

11 years ago
Keywords: verifyme
(Reporter)

Comment 12

11 years ago
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.
Attachment #291904 - Flags: approval1.8.1.12? → approval1.8.1.12+
(Reporter)

Updated

11 years ago
Flags: wanted1.8.1.x+
Whiteboard: Seen in real-world Thunderbird subject lines → [sg:low] Seen in real-world Thunderbird subject lines
(Assignee)

Comment 13

11 years ago
A Japanese tester reported the problem was fixed.
No regressions are reported. Would you land this on the branch?
(Reporter)

Updated

11 years ago
Keywords: checkin-needed
Keywords: qawanted
checked-in to 1.8 branch.
Keywords: checkin-needed → fixed1.8.1.12
Who can check this in the 1.8 branch?
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. 
(Assignee)

Comment 18

11 years ago
See bug 381412 commment #18.
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
Keywords: fixed1.8.1.12 → verified1.8.1.12
Flags: blocking1.8.0.15? → blocking1.8.0.15+

Comment 20

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

a=asac for 1.8.0.15

(unmodified distro patch)
Attachment #291904 - Flags: approval1.8.0.15+
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
Keywords: fixed1.8.0.15
(Reporter)

Updated

10 years ago
Group: security
You need to log in before you can comment on or make changes to this bug.