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

RESOLVED FIXED in Firefox 48

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jorgk, Assigned: emk)

Tracking

({regression, sec-high})

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 wontfix, firefox46- unaffected, firefox47- unaffected, firefox48+ fixed, firefox-esr38 unaffected, firefox-esr45- unaffected)

Details

Attachments

(2 attachments)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
Blocks: 1251120
Reporter

Updated

3 years ago
Flags: needinfo?(smontagu)
Can you attach the reproducible example (or email it to this address if it's sensitive data)?
Flags: needinfo?(smontagu) → needinfo?(mozilla)
Reporter

Comment 2

3 years ago
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.
Flags: needinfo?(mozilla)
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)
Group: core-security
Marking security sensitive because of the buffer-overrun, though that may be locking the stable door after the horse has bolted in this case.
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!
Assignee: nobody → smontagu
Keywords: regression
Keywords: sec-high
Blocks: 1170932
Group: core-security → core-security-release
Any luck here Simon? We are heading into beta 7 soon. (Or, beta 6 build 2....)
Flags: needinfo?(smontagu)
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...
Flags: needinfo?(VYV03354)
Assignee

Updated

3 years ago
Flags: needinfo?(VYV03354)
Assignee

Comment 10

3 years ago
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.
Attachment #8737624 - Flags: review?(smontagu)
Attachment #8737624 - Flags: review?(hsivonen)
Assignee

Comment 11

3 years ago
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 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.
Attachment #8737624 - Flags: review?(hsivonen) → review+
Assignee

Comment 13

3 years ago
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.
Attachment #8737624 - Flags: sec-approval?
Comment on attachment 8737624 [details] [diff] [review]
Stop using nsTableEncoderSupport subclasses in GBK/GB18030 encoders

sec-approval+ for trunk.
Attachment #8737624 - Flags: sec-approval? → sec-approval+
Assignee

Comment 15

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/38204fd2ece962c9504ae59d58c99bf38b5626bd
Bug 1255863 - Stop using nsTableEncoderSupport subclasses in GBK/GB18030 encoders. r=hsivonen
https://hg.mozilla.org/mozilla-central/rev/38204fd2ece9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Reporter

Comment 17

3 years ago
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(hsivonen)
Flags: needinfo?(VYV03354)
Resolution: FIXED → ---
Assignee

Comment 18

3 years ago
Now *aOutLen can be smaller than 4 on input, so ConvertByTable can set *aOutLen smaller value than 4 and return NS_OK_UENC_MOREOUTPUT.
Flags: needinfo?(hsivonen)
Flags: needinfo?(VYV03354)
Attachment #8739235 - Flags: review?(hsivonen)
Assignee

Comment 19

3 years ago
@Jorg K: Again, please consider to use GetMaxLength in favor of a fixed-size buffer.
Flags: needinfo?(mozilla)
Reporter

Comment 20

3 years ago
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?
Flags: needinfo?(mozilla)
Assignee

Comment 21

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

Comment 22

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

Comment 23

3 years ago
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?
Assignee

Comment 24

3 years ago
If you do not handle NS_OK_UENC_MOREOUTPUT, some output characters may be silently lost.
Assignee: smontagu → VYV03354
Severity: normal → critical
Flags: needinfo?(smontagu)
OS: Unspecified → All
Hardware: Unspecified → All
Attachment #8739235 - Flags: review?(hsivonen) → review+
Since this doesn't affect Firefox, I don't think we need to keep tracking the bug.
https://hg.mozilla.org/mozilla-central/rev/01cd641cc9b9
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Group: core-security-release
Assignee

Updated

3 years ago
Attachment #8737624 - Flags: review?(smontagu)
You need to log in before you can comment on or make changes to this bug.