1255863 - Conversion from Unicode to GBK (nsIUnicodeEncoder::Convert) overruns output buffer

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Jorg K (CEST = GMT+2)]

We found in bug 1251120 that nsIUnicodeEncoder::Convert() overruns the output buffer. A 512-byte buffer is passed in together with the correct length of 512, yet 514 bytes are returned.

We have a reproducible example.

Comment 1

[Simon Montagu :smontagu]

Can you attach the reproducible example (or email it to this address if it's sensitive data)?

Comment 2

[Jorg K (CEST = GMT+2)]

I've just e-mailed you a zipped mailbox file, I don't know how sensitive it is. Just reply to the message, press send without entering anything and bang ;-)
You might want to apply one of the patches from 1251120 for debugging.

Comment 3

[Simon Montagu :smontagu]

From code inspection I think that nsUnicodeToGBK.cpp is overrunning the output buffer if it reaches the call to Try4BytesEncoder near the end of ConvertNoBuffNoErr when there are fewer than 4 bytes left in the output buffer, because it doesn't make any check of output length.

(For the record, the problem character is not even a Chinese character but the humble  , which encodes to x81 x30 x84 x32 in GB18030. This can be seen as "?0,2" in bug 1251120 comment 19)

Comment 4

[Simon Montagu :smontagu]

Marking security sensitive because of the buffer-overrun, though that may be locking the stable door after the horse has bolted in this case.

Comment 5

[Simon Montagu :smontagu]

This is really my regression from bug 1170932: in bug 1170932 comment 1 I said "This check is not needed for gbk, where Try4BytesEncode will always fail", and then went ahead and removed the check for GB18030 as well, where it _is_ needed!

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Any luck here Simon? We are heading into beta 7 soon. (Or, beta 6 build 2....)

Comment 7

[Mats Palmgren (inactive)]

Masatoshi, perhaps you can tell from Simon's comments above what the problem is?

AFAICT, bug 1170932 removed two length checks (in nsUnicodeToGBK::ConvertNoBuffNoErr):
https://hg.mozilla.org/mozilla-central/rev/10dfb6455e4f#l2.201
https://hg.mozilla.org/mozilla-central/rev/10dfb6455e4f#l2.219

I'm guessing those are the missing checks (for GB18030) that comment 5 refers to?
I have no clue where to insert them though...

Comment 8

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cd85676cb6e

Comment 9

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed61a83251cd

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Stop using nsTableEncoderSupport subclasses in GBK/GB18030 encoders

Requesting a review to whoever comes first.

* aOutLen was always 2 or 4 that caused a buffer overrun. I changed it to an actual remaining buffer length.
* I stopped using nsTableEncoderSupport::Convert because the stateful behavior of nsTableEncoderSupport disturbed the buffer length check. This change is similar to bug 1176462.
* I added buffer length check *before* writing an ASCII output.
* It is too late to check a length after a buffer overrun. I removed the pointless check.

Comment 11

[Masatoshi Kimura [:emk]]

This bug would not be exploitable as long as the caller uses GetMaxLength and allocates the buffer dynamically. The buffer overrun can happen only if the caller uses a fixed-size buffer such as  nsMsgI18N.cpp.
Firefox would not be not affected because all m-c callers use GetMaxLength AFAICT.

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 8737624 [details] [diff] [review]
Stop using nsTableEncoderSupport subclasses in GBK/GB18030 encoders

Review of attachment 8737624 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK to me.

Comment 13

[Masatoshi Kimura [:emk]]

Comment on attachment 8737624 [details] [diff] [review]
Stop using nsTableEncoderSupport subclasses in GBK/GB18030 encoders

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- I believe this is not exploitable on Firefox.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- Hopefully the bug summary will obscure the security aspect of the patch.

Which older supported branches are affected by this flaw?
- No branches would be affected.

Comment 14

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

Comment on attachment 8737624 [details] [diff] [review]
Stop using nsTableEncoderSupport subclasses in GBK/GB18030 encoders

sec-approval+ for trunk.

Comment 15

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38204fd2ece962c9504ae59d58c99bf38b5626bd
Bug 1255863 - Stop using nsTableEncoderSupport subclasses in GBK/GB18030 encoders. r=hsivonen

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38204fd2ece9

Comment 17

[Jorg K (CEST = GMT+2)]

I originally raised this bug since we had a problem in Thunderbird with replying to a certain message.

I've just compiled Thunderbird with this M-C change and when repeating the action that before overran the buffer, I now get a crash here on the MOZ_ASSERT:

nsresult nsUnicodeToGB18030::Try4BytesEncoder(char16_t aChar,
                                              char* aOut,
                                              int32_t *aOutLen)
{
  int32_t len = 1;
  nsresult res = nsUnicodeEncodeHelper::ConvertByTable(
    &aChar, &len, aOut, aOutLen, u4BytesGB18030Charset, nullptr,
    (uMappingTable*) &g_uf_gb18030_4bytes);
  MOZ_ASSERT(NS_FAILED(res) || ((1 == len) && (4 == *aOutLen)),
             "unexpect conversion length");
  return res;
}

Here's the call stack:
xul.dll!nsUnicodeToGB18030::Try4BytesEncoder(wchar_t aChar, char * aOut, int * aOutLen) Line 57	C++
xul.dll!nsUnicodeToGBK::ConvertNoBuffNoErr(const wchar_t * aSrc, int * aSrcLength, char * aDest, int * aDestLength) Line 233	C++
xul.dll!nsEncoderSupport::ConvertNoBuff(const wchar_t * aSrc, int * aSrcLength, char * aDest, int * aDestLength) Line 356	C++
xul.dll!nsEncoderSupport::Convert(const wchar_t * aSrc, int * aSrcLength, char * aDest, int * aDestLength) Line 440	C++
xul.dll!nsMsgI18NConvertFromUnicode(const char * aCharset, const nsString & inString, nsACString_internal & outString, bool aIsCharsetCanonical, bool aReportUencNoMapping) Line 93	C++
xul.dll!nsMsgComposeAndSend::GetBodyFromEditor() Line 1599	C++
xul.dll!nsMsgComposeAndSend::ProcessMultipartRelated(int * aMailboxCount, int * aNewsCount) Line 1872	C++

So while you may not overrun the buffer any more, something else is going wrong.

We currently have a patch in Thunderbird that supplies a buffer that is 10 bytes larger than requested and with this hack, the action works. With the new patch here, the action doesn't work at all.

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: Followup to fix an assertion

Now *aOutLen can be smaller than 4 on input, so ConvertByTable can set *aOutLen smaller value than 4 and return NS_OK_UENC_MOREOUTPUT.

Comment 19

[Masatoshi Kimura [:emk]]

@Jorg K: Again, please consider to use GetMaxLength in favor of a fixed-size buffer.

Comment 20

[Jorg K (CEST = GMT+2)]

Thanks for the hint. I've already read it in comment #11. Our caller is here:
https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgI18N.cpp#91

So we should do it like here:
https://dxr.mozilla.org/mozilla-central/source/intl/uconv/nsConverterOutputStream.cpp#74
Or is there a better example?

In any case, what we are doing is a legal use case and should work, right?

Comment 21

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+2) from comment #20)
> Thanks for the hint. I've already read it in comment #11. Our caller is here:
> https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgI18N.
> cpp#91
> 
> So we should do it like here:
> https://dxr.mozilla.org/mozilla-central/source/intl/uconv/
> nsConverterOutputStream.cpp#74

Yes.

> In any case, what we are doing is a legal use case and should work, right?

Yes if you handle NS_OK_UENC_MOREOUTPUT correctly. The current nsMsgI18N code does not differentiate it with NS_OK, so it does not look correct for me. See also bug 1251120 comment #37.

Comment 22

[Jorg K (CEST = GMT+2)]

Thanks, I finally got the message ;-) and raised bug 1263088 for this work.

Comment 23

[Jorg K (CEST = GMT+2)]

I'm happy to report that my test case works with the additional patch even though the MailNews code doesn't handle NS_OK_UENC_MOREOUTPUT. Does the MailNews code work by accident in this case?

Comment 24

[Masatoshi Kimura [:emk]]

If you do not handle NS_OK_UENC_MOREOUTPUT, some output characters may be silently lost.

Comment 25

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01cd641cc9b9de4bcd440e7f95e1c441c126e176
Bug 1255863 - Followup to fix an assertion. r=hsivonen

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Since this doesn't affect Firefox, I don't think we need to keep tracking the bug.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01cd641cc9b9