470523 - ISO-2022-CN decoder is horribly broken

Status: RESOLVED WONTFIX

(Core :: Internationalization, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: Sample text from RFC 1922

It can't decode even RFC 1922 sample text correctly.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: overhauled ISO-2022-CN decoder

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: fixed some XSS vectors

Oops, the previous patch was still vulnerable to some XSS attacks.

Comment 3

[Simon Montagu :smontagu]

The patch looks technically correct, but I'm not really sure that we need it. I don't know how commonly used ISO-2022-CN is in the first place, and it must be very unusual to have traditional and simple Chinese mixed in the same message, let alone the same line.

FWIW, iconv doesn't decode the sample text correctly either (the expected decoding is 交换交換)

Comment 4

[Masatoshi Kimura [:emk]]

> The patch looks technically correct, but I'm not really sure that we need it.
If we don't need it, we should completely remove it rather than leaving it broken. Broken support is worse than no support. (Remember Nav4 CSS support!)
> I don't know how commonly used ISO-2022-CN is in the first place,
I'm afraid that it's rarely used. ISO-2022-JP is an only survivor of 7-bit ISO/IEC 2022 family encodings. But it's defined in RFC, registered by IANA, and supported by Microsoft Windows (CP50227 and 50229).
> and it must be
> very unusual to have traditional and simple Chinese mixed in the same message,
> let alone the same line.
The current decoder treats a single shift as it's a locking shift. So traditional Chinese only text will not be parsed correctly if it contains plane 2 or 3 characters.
> FWIW, iconv doesn't decode the sample text correctly either (the expected
decoding is 交换交換)
Which iconv? GNU iconv 1.11 which is included in MozillaBuild handled it correctly.

Comment 5

[Masatoshi Kimura [:emk]]

I'm going to consider dropping support for ISO-2022-CN because:
1. IE and WebKit do not support this encoding.
2. Our implementation has been broken for a long time.

Comment 6

[Masatoshi Kimura [:emk]]

Why ISO-2022-CN is defined in Encoding Standard?
http://dvcs.w3.org/hg/encoding/raw-file/78076fc4d607/Overview.html#iso-2022-cn

Comment 7

[Anne (:annevk)]

I'm open to removing it. I believe Chrome specifically added basic support (not backed by encoding tables though) for it to prevent XSS exploits.

Comment 8

[Anne (:annevk)]

http://code.google.com/p/chromium/issues/detail?id=21354 is what Chrome has done. For "iso-2022-cn" and "iso-2022-cn-ext".

Comment 9

[Glenn Maynard]

One idea was to always treat text/html; charset=iso-2022-cn as text/plain to prevent XSS problems.  That makes it explicitly not work at all, rather than be parsed as HTML with the wrong charset (leading to the XSS issue).

I don't know if that would complicate the parser; for example, meta charset could cause the content type to change instead of just the charset.

Comment 10

[Masatoshi Kimura [:emk]]

Hm, IE9 was vulnerable to this XSS attack.
http://i18nl10n.com/chrome/iso2022cn1.html
http://i18nl10n.com/chrome/iso2022cn2.html

Comment 11

[Anne (:annevk)]

Given that Internet Explorer does not support this encoding at all, maybe we should just remove it from all browsers (and the spec)?

Comment 12

[Masatoshi Kimura [:emk]]

The ISO-2022-CN decoder will soon be removed (bug 997115).
We have introduced the "replacement" encoding to avoid the security issue.