Last Comment Bug 449578 - Wrong interpretation of escape sequence in charset iso-2022-kr
: Wrong interpretation of escape sequence in charset iso-2022-kr
Status: RESOLVED FIXED
: fixed1.8.1.17, fixed1.9.0.2, regression
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: unspecified
: x86 Linux
: -- minor (vote)
: mozilla1.9.1a2
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on:
Blocks: 381412
  Show dependency treegraph
 
Reported: 2008-08-07 05:17 PDT by Christoph Garst
Modified: 2008-08-26 14:46 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.36 KB, patch)
2008-08-10 13:03 PDT, Simon Montagu :smontagu
no flags Details | Diff | Review
Improved patch (5.20 KB, patch)
2008-08-11 10:43 PDT, Simon Montagu :smontagu
VYV03354: review+
dveditz: approval1.8.1.17+
dveditz: approval1.9.0.2+
Details | Diff | Review

Description Christoph Garst 2008-08-07 05:17:32 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.14) Gecko/20061201 Firefox/2.0.0.14 (Ubuntu-feisty)
Build Identifier: Thunderbird 2.0.0.16 (20080724)

Thunderbird displays an unicode replacement character in front of each e-mail-header which has charset iso-2022-kr.

Reproducible: Always

Steps to Reproduce:
1. Compose an email with following To-Header: =?iso-2022-kr?q?=1B=24=29C=0E "some korean characters" =0F?= <foo@example.com>
2. View that mail in Thunderbird

Actual Results:  
There will be an additional unicode replacement character in front of the korean text

Expected Results:  
Don't show that character

In intl/uconv/ucvko/nsISO2022KRToUnicode.cpp at lines 107 and 132 there is a check if mRunLength == 0 (Length of non-ASCII run). The unicode replacement character is added if the decoder finds the escape sequence (ESC "$" ")" "C") in front of a line (where it should be, as rfc1557 states).
Comment 1 Simon Montagu :smontagu 2008-08-10 13:03:47 PDT
Created attachment 333167 [details] [diff] [review]
Patch
Comment 2 Christoph Garst 2008-08-11 08:36:22 PDT
Works, thanks.
Comment 3 Masatoshi Kimura [:emk] 2008-08-11 09:28:13 PDT
Comment on attachment 333167 [details] [diff] [review]
Patch

How to prevent a XSS attack like bug 381412? Indeed, the sequence ESC "$" ")" "C" can occur any number of times per RFC 1557, I'm hesitating to allow appearing the designator twice (or more).
BTW I'm not a intl owner/peer. Is jshin inactive now?
Comment 4 Simon Montagu :smontagu 2008-08-11 10:43:52 PDT
Created attachment 333250 [details] [diff] [review]
Improved patch

What do you think of this approach, only allowing ESC $)C at the beginning of a line? I am a little hesitant because I don't have any examples of actual ISO-2022-KR mails to test against, and I don't know how well existing clients that send ISO-2022-KR mails conform to RFC 1557. Christoph, maybe you can attach some samples or forward them to me?
Comment 5 Christoph Garst 2008-08-12 02:19:50 PDT
This would comply to RFC 1557 and works for me. I use gmime to create mails which uses iconv to convert from the internal utf-8 representation to a best fitting charset (my characters are japanese, but gmime encodes to iso-2022-kr). I don't have another sample and i don't know how wide spread this charset really is (this bug would have been found before).
Comment 6 Masatoshi Kimura [:emk] 2008-08-19 05:31:05 PDT
Comment on attachment 333250 [details] [diff] [review]
Improved patch

Sorry for the late response.
I think it's a reasonable compromise. ISO-2022-KR is rarely used in the real world.
Comment 7 Reed Loden [:reed] (use needinfo?) 2008-08-19 21:37:16 PDT
Pushed as 17110:b17a3d22447e.
Comment 8 Simon Montagu :smontagu 2008-08-19 23:53:00 PDT
Comment on attachment 333250 [details] [diff] [review]
Improved patch

This is a regression from bug 381412, so we need the fix on the branches where that went in.
The testcase includes a test for the regression and for issues similar to those in bug 381412.
Comment 9 Daniel Veditz [:dveditz] 2008-08-20 15:13:23 PDT
Comment on attachment 333250 [details] [diff] [review]
Improved patch

Approved for 1.8.1.17 and 1.9.0.2, a=dveditz for release-drivers.

Note You need to log in before you can comment on or make changes to this bug.