1374149 - Folded subject header using ISO-2022-JP doesn't get decoded properly

Status: RESOLVED FIXED

(MailNews Core :: Internationalization, defect)

Description

[Takanori MATSUURA]

Tabs between MIME encoding lines are improperly treated.

Maybe a regression of bug 1261841.
Which Product/Component is better for this bug, "MailNews Core/Internationalization" or "Core/Internationalization"?

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:56.0) Gecko/20100101 Thunderbird/56.0a1
Build ID: 20170618030210


Good:
Subject: =?UTF-8?B?44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC?=
 =?UTF-8?B?44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC?=
 =?UTF-8?B?44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC?=
 =?UTF-8?B?44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC?=
 =?UTF-8?B?44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC?=
 =?UTF-8?B?44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC44GC?=

-> ああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああ


NG:
Subject: =?ISO-2022-JP?B?GyRCJCIkIiQiJCIkIiQiJCIkIiQiJCIkIhsoQg==?=
	=?ISO-2022-JP?B?GyRCJCIkIiQiJCIkIiQiJCIkIiQiJCIkIiQiJCIbKEI=?=
	=?ISO-2022-JP?B?GyRCJCIkIiQiJCIkIiQiJCIkIiQiJCIkIiQiJCIbKEI=?=
	=?ISO-2022-JP?B?GyRCJCIkIiQiJCIkIiQiJCIkIiQiJCIkIiQiJCIbKEI=?=
	=?ISO-2022-JP?B?GyRCJCIkIiQiJCIkIiQiJCIkIiQiJCIkIiQiJCIbKEI=?=
	=?ISO-2022-JP?B?GyRCJCIkIiQiJCIkIiQiJCIkIiQiJCIkIiQiJCIbKEI=?=
	=?ISO-2022-JP?B?GyRCJCIkIiQiJCIkIiQiJCIkIiQiJCIkIiQiJCIbKEI=?=
	=?ISO-2022-JP?B?GyRCJCIkIiQiGyhC?=

-> あああああああああああ�あああああああああああああ�あああああああああああああ�あああああああああああああ�あああああああああああああ�あああああああああああああ�あああああああああああああ�あああ

Comment 1

[Jorg K (CEST = GMT+2)]

(In reply to Takanori MATSUURA from comment #0)
> Tabs between MIME encoding lines are improperly treated.
Where are the tabs? Oh, I see:
> Subject: =?ISO-2022-JP?B?GyRCJCIkIiQiJCIkIiQiJCIkIiQiJCIkIhsoQg==?=
> 	=?ISO-2022-JP?B?GyRCJCIkIiQiJCIkIiQiJCIkIiQiJCIkIiQiJCIbKEI=?=
  ^^^^^^
What happens when you swap these tabs for spaces? In fact, I tried, that doesn't make a difference.

> Maybe a regression of bug 1261841.
Did it work in previous versions of TB? Yes, I tested TB 55 and it shows correctly there.

> Which Product/Component is better for this bug, "MailNews
> Core/Internationalization" or "Core/Internationalization"?
Good question, if it's cause by bug 1261841, then "Core/Internationalization", otherwise "MailNews Core/Internationalization".

There are two possibilities here:
1) Either we assemble the subject line incorrectly and the new ISO-2022-JP decoder is less tolerant to bad input
2) There is a problem in the new ISO-2022-JP decoder, which I doubt.

I'll have to check what exactly gets passed to the decoder.

Comment 2

[Jorg K (CEST = GMT+2)]

I've reduced the test case to:
Subject: =?ISO-2022-JP?B?GyRCJCIkIiQiJCIkIiQiJCIkIiQiJCIkIhsoQg==?=
 =?ISO-2022-JP?B?GyRCJCIkIiQiGyhC?=
That still shows a � in the decoded subject.

The header decoding is done in JSMime and there I found this comment:
https://dxr.mozilla.org/comm-central/rev/0cac30b866ef093d76eadbe6d55f5d1c916f39d2/mailnews/mime/jsmime/jsmime.js#682
related to cutting multi-byte characters in the middle. Perhaps that's what happened here.

Takanori-san, which software produced this header? It wasn't TB since we do all headers in UTF-8.

Henri, do you have any idea what's going on here? As I said before, the decoding takes place in JSMime, using .decode on a TextDecoder. To port bug 1261841 in bug 1363281, I didn't make any changes to the MIME code.

Comment 3

[Masatoshi Kimura [:emk]]

var decoder = new TextDecoder("iso-2022-jp");
var encoder = new TextEncoder();
decoder.decode(encoder.encode(atob('GyRCJCIkIiQiJCIkIiQiJCIkIiQiJCIkIhsoQg==')),{stream:true});
decoder.decode(encoder.encode(atob('GyRCJCIkIiQiGyhC')));
Nightly 56: "\ufffdあああ"
Release 54: "あああ"
The encoding_rs iso-2022-jp decoder inserts "\ufffd" when it encounters the "\u001b(B\u001b$B" sequence. (probably in order to prevent XSS attacks.) Henri would know the answer. Chrome matches encoding_rs, by the way.

If we do *NOT* pass {streaming: true}, it works as expected because TextDecoder no longer encounters the "\u001b(B\u001b$B" sequence.

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #3)
> If we do *NOT* pass {streaming: true}, it works as expected because
> TextDecoder no longer encounters the "\u001b(B\u001b$B" sequence.

It will break the splitted multi-byte case, of course. Another workaround is to trim the trailing "\u001b(B\" and the leading "\u001b$B" manually before passing the buffer to TextDecoder.

Comment 5

[Henri Sivonen (:hsivonen)]

Each base64 segment ends with an escape that transitions back to the ASCII state. Each base64 segment starts with a escape that transitions to the JIS X 0208 state. When these are concatenated, they form a zero-length ASCII run, which is an error per spec.

The remedy would be to omit the {stream: true} parameter on the TextDecoder call, but per the comment there, that would break the decoding of RFC 2047-incompliant input. What emk says about trimming the escapes would work here but could potentially break other (incompliant) input.

I'm not sure what the rationale is for zero-length runs between escapes being errors considering all the mischief that's possible by transitioning from the ASCII state to the ASCII state or alternating between the ASCII and Roman states. annevk, why is a zero-length run between escapes an error?

Comment 6

[Henri Sivonen (:hsivonen)]

Probably the most robust hack would be to omit {stream: true} parameter if the encoding is ISO-2022-JP and the previous segment ended with ESC ( B.

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #6)
> Probably the most robust hack would be to omit {stream: true} parameter if
> the encoding is ISO-2022-JP and the previous segment ended with ESC ( B.

Correction: Omit {stream: true} if the encoding is ISO-2022-JP and the segment being decoded ends with ESC ( B. (And with any encoding when decoding the last segment.)

Basically, compliant producers produce output as if the consumers didn't do streaming decode. Thunderbird does streaming decode in case a producer is incompliant. ISO-2022-JP is the only encoding that's not indifferent to content produced for non-streaming consumption being consumed in the streaming mode. :-(

Comment 8

[Takanori MATSUURA]

Masatoshi and Henri have already started the tech discussion.
Thanks.

FYI, such subject line is generated by "Active! mail" which is one of the famous on-premises web mail system in Japan.

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #5)
> I'm not sure what the rationale is for zero-length runs between escapes
> being errors considering all the mischief that's possible by transitioning
> from the ASCII state to the ASCII state or alternating between the ASCII and
> Roman states. annevk, why is a zero-length run between escapes an error?

It seems the spec behavior came from https://www.w3.org/Bugs/Public/show_bug.cgi?id=27256 which claims Chrome had this behavior at the time.

Additionally, https://web.archive.org/web/20140912104045/http://upokecenter.dreamhosters.com/articles/2013/04/differences-in-the-iso-2022-jp-encoding-between-browsers/ claims Firefox had this behavior in the reverse case (switch to multibyte and then back to ASCII without anything in between).

Comment 10

[Henri Sivonen (:hsivonen)]

Given the Chrome behavior, it's probably the best not to tweak the Encoding Standard here and for Thunderbird to adopt the tweak from comment 7 to its Postel hack.

Comment 11

[Jorg K (CEST = GMT+2)]

Summarising: So basically this is caused by
return output + currentDecoder.decode(buffer, {stream: true});
https://dxr.mozilla.org/comm-central/rev/0cac30b866ef093d76eadbe6d55f5d1c916f39d2/mailnews/mime/jsmime/jsmime.js#686
where the streaming is used to be tolerant to multi-bytes broken in the middle.

For ISO-2022-JP that causes a problem since the input presented represents correct RCF2047 chunks, each started and terminated correctly, and those termination marks are now causing a problem in "streaming mode" since a zero-length run between them is now considered to be an error by the new decoder, but not the old. Hmm.

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Jorg K (GMT+2) from comment #11)
> Summarising: So basically this is caused by
> return output + currentDecoder.decode(buffer, {stream: true});
> https://dxr.mozilla.org/comm-central/rev/
> 0cac30b866ef093d76eadbe6d55f5d1c916f39d2/mailnews/mime/jsmime/jsmime.js#686
> where the streaming is used to be tolerant to multi-bytes broken in the
> middle.
> 
> For ISO-2022-JP that causes a problem since the input presented represents
> correct RCF2047 chunks, each started and terminated correctly, and those
> termination marks are now causing a problem in "streaming mode" since a
> zero-length run between them is now considered to be an error by the new
> decoder, but not the old. Hmm.

Right. Clearly, RFC 2047 tries to require producers to do things in such a way that consumption in either the streaming or non-streaming mode works, but RFC 2047 doesn't foresee empty sequences between escapes becoming an error for ISO-2022-JP. This would not be a problem if Thunderbird relied on producers being RFC 2047-compliant and each segment, therefore, being consumable in non-streaming mode. But when Thunderbird's attempt to deal with non-compliant producers is added to the mix, the combination of factors produces undesirable results.

Therefore, I suggest that Thunderbird turn off its attempt to deal with non-compliant consumers when the input looks compliant enough for the non-compliance workaround being unnecessary. I.e. when decoding iso-2022-jp and the segment does, in fact, end with ESC ( B like the RFC requires.

Additionally, passing {stream:true} unconditionally causes *another* bug for all (multibyte) encodings: It masks errors at the end of the last segment, i.e. the end of the stream is currently not signaled to the decoder.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1374149-decoding-streaming.patch (v1).

This addresses the ISO-2022-JP issue. This comment is not addressed so far:
> Additionally, passing {stream:true} unconditionally causes *another*
> bug for all (multibyte) encodings.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1374149-decoding-streaming.patch (v2).

OK, now taking care of not calling this with streaming on the last token.

Comment 15

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #10)
> Given the Chrome behavior, it's probably the best not to tweak the Encoding
> Standard here and for Thunderbird to adopt the tweak from comment 7 to its
> Postel hack.

After thinking about this some more, filed a spec bug anyway in order to have this properly considered:
https://github.com/whatwg/encoding/issues/115

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1374149-decoding-streaming.patch (v2a).

Tweaked comment and order to make the more likely clause execute first.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1374149-decoding-streaming.patch (v2b) + test

This adds a test.

Comment 19

[Anne (:annevk)]

https://github.com/whatwg/encoding/issues/115#issuecomment-309411747 seems like a good enough reason to treat this as an error and maybe special case in email?

Comment 20

[Henri Sivonen (:hsivonen)]

(In reply to Anne (:annevk) from comment #19)
> https://github.com/whatwg/encoding/issues/115#issuecomment-309411747 seems
> like a good enough reason to treat this as an error and maybe special case
> in email?

What's unclear to me is: What scenario makes zero-length sequences between escapes dangerous but doesn't make escape to ascii state, one character, another escape to ASCII state or alternating between ASCII and Roman states for a run of ASCII characters with at least one character in each state dangerous?

Comment 21

[Anne (:annevk)]

Not sure, let's ask emk: see comment 20.

Comment 22

[Masatoshi Kimura [:emk]]

Uconv considered ASCII-to-ASCII switches danger:
https://dxr.mozilla.org/mozilla-beta/rev/c836770a64c15192c42d33340abf861a1d201723/intl/uconv/ucvja/nsJapaneseToUnicode.cpp#511-516
Exactly because of the security reason (bug 407161).

If the Encoding Standard does not treat them danger, I think we should do it.

Comment 23

[Anne (:annevk)]

emk, what about ASCII-to-Roman?

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8879101 [details] [diff] [review]
1374149-decoding-streaming.patch (v2b) + test

Masatoshi-san, could you review this for me please. It's about time we fix this regressions which is affecting our Japanese Daily users since popular Japanese web services (see comment #8) are sending ISO-2022-JP encoded subject headers.

Comment 25

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/78a4fd6d79b50c334acfbae9220a92dc384be321

Comment 26

[Jorg K (CEST = GMT+2)]

Looks like this caused a test failure in test_jsmime_charset.js:
"�" == ""
Since the last RFC 2047 token is now decoded with no streaming, the encoder notices the wrong input immediately and returns a � instead of waiting for the next chunk that never arrives. So here we need to change the expected result.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1374149-follow-up.patch - Follow up to adjust test.

https://hg.mozilla.org/comm-central/rev/0f423916d61ee30891a8c7b419f678c7145dbe44

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 1374149-follow-up2.patch - Follow up to add tests for reassembling broken characters by streaming decode

https://hg.mozilla.org/comm-central/rev/ceb1142708c762273ba9436887b5ff7b8ef60f5d

OK, just to make it perfect I added some test that show that the streaming decode works.

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8879101 [details] [diff] [review]
1374149-decoding-streaming.patch (v2b) + test

Bug 1301989 depends on this one, so let's uplift this one, too.

Comment 30

[Jorg K (CEST = GMT+2)]

Beta (TB 55):
https://hg.mozilla.org/releases/comm-beta/rev/6aa1d5bc1ec8
https://hg.mozilla.org/releases/comm-beta/rev/674547528f72
https://hg.mozilla.org/releases/comm-beta/rev/55d9ee1f85e8

Comment 31

[Jorg K (CEST = GMT+2)]

TB 52 ESR
https://hg.mozilla.org/releases/comm-esr52/rev/b3cbdb955bd9b0bb19ac8cbe7c2a876dbefed0d3
https://hg.mozilla.org/releases/comm-esr52/rev/4ac8763c90f6388233f69ff207c9fab288cbe2a3
https://hg.mozilla.org/releases/comm-esr52/rev/ca3a1c967a22170ad8f8fc6e7ce3646fd3d38bc2