1443891 - (CVE-2018-5178) Integer overflow in nsScriptableUnicodeConverter::ConvertFromByteArray can cause a heap buffer overflow

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[A.]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180206200532

Steps to reproduce:

I. The bug 

In nsScriptableUnicodeConverter::ConvertFromByteArray (ff-esr52/intl/uconv/nsScriptableUConv.cpp), the malloc() size argument on line 148 is computed from an unbounded integer value, which can overflow:  

134:nsScriptableUnicodeConverter::ConvertFromByteArray(const uint8_t* aData,
135:                                                   uint32_t aCount,
136:                                                   nsAString& _retval)
137:{
138:  if (!mDecoder)
139:    return NS_ERROR_FAILURE;
140:
141:  nsresult rv = NS_OK;
142:  int32_t inLength = aCount;
143:  int32_t outLength;
144:  rv = mDecoder->GetMaxLength(reinterpret_cast<const char*>(aData),
145:                              inLength, &outLength);
146:  if (NS_SUCCEEDED(rv))
147:  {
148:    char16_t* buf = (char16_t*)malloc((outLength+1) * sizeof(char16_t));
149:    if (!buf)
150:      return NS_ERROR_OUT_OF_MEMORY;
151:
152:    rv = mDecoder->Convert(reinterpret_cast<const char*>(aData),
153:                           &inLength, buf, &outLength);
154:    if (NS_SUCCEEDED(rv))
155:    {
156:      buf[outLength] = 0;
157:      if (!_retval.Assign(buf, outLength, mozilla::fallible)) {
158:        rv = NS_ERROR_OUT_OF_MEMORY;
159:      }
160:    }
161:    free(buf);
162:    return rv;
163:  }
164:  return NS_ERROR_FAILURE;
165:
166:}

The outLength value is provided by GetMaxLength() (line 144), which is a member of nsIUnicodeDecoder class, and represented by nsUTF8ToUnicode::GetMaxLength in this case (see bug#1440926 for some background analysis of this family of classes). It is computed as the length of aData passed from the caller, plus 1.

Consider for instance, that the length of aData is INT32_MAX-1. Then outLength will be INT32_MAX and will overflow on the addition operation (line 148), and become -2147483648. Then the multiplication operation by 2 (sizeof(char16_t)) will overflow it again, and the result will be truncated to zero and passed as an argument to malloc(). 

Here is an output of a simple test program written in C and compiled with Clang (Apple LLVM version 8.0.0 (clang-800.0.42.1)), which demostrates the effect of the unchecked integer arithmetic in this code:

$ ./a.out
INT32_MAX + 1 = -2147483648 (0x80000000)
(INT32_MAX + 1) * 2 = 0 (0x0)

Next, nsUTF8ToUnicode::Convert() on line 152 will populate the zero-sized buffer with unicode data derived from aData, using the original value of outLength (which is still INT32_MAX) as the bound. That will lead to overwrite of unowned memory which immediately follows the buffer, with caller-controlled data. 

II. Reachability analysis

The only place where nsScriptableUnicodeConverter::ConvertFromByteArray is used in core code is nsScriptableUnicodeConverter::ConvertToUnicode. Both functions are directly exposed to privileged Javascript in Firefox. Therefore, I have identified 3 possible attack vectors:

1. Via a malicious extension

Privileged Javascript can obtain an instance of nsScriptableUnicodeConverter via Components.classes class factory, as such:

var converter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"]
                .createInstance(Components.interfaces.nsIScriptableUnicodeConverter);

And then call the ConvertToUnicode/ConvertFromByteArray functions directly with an arbitrary String or a TypedArray.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIScriptableUnicodeConverter

2. (1) implies that all Firefox installations which have installed an legitimate extension which uses nsIScriptableUnicodeConverter are affected. 

The specific attack vector in this case depends on how the extension uses the class, however it's likely to operate on user-controlled data. (See bug#289947 for example of such an extension and the user-controlled data which it processed)

3. There is a number of places in the components of Firefox written in Javascript, which instantiate and use nsScriptableUnicodeConverter for their own purposes.
Notably devtools, password manager, settings, and import of bookmarks:

nsScriptableUnicodeConverter::ConvertToUnicode:
./browser/base/content/test/urlbar/browser_urlbarCopying.js:12:  return converter.ConvertToUnicode(input);
./devtools/client/debugger/content/views/sources-view.js:307:      unicodeUrl = NetworkHelper.convertToUnicode(unescape(fullUrl));
./devtools/client/debugger/content/views/sources-view.js:638:    return NetworkHelper.convertToUnicode(unescape(source.url));
./devtools/client/debugger/utils.js:149:    let unicodeLabel = NetworkHelper.convertToUnicode(unescape(sourceLabel));
./devtools/client/debugger/utils.js:185:    let unicodeLabel = NetworkHelper.convertToUnicode(unescape(groupLabel));
./devtools/client/netmonitor/netmonitor-view.js:556:      let unicodeUrl = NetworkHelper.convertToUnicode(unescape(data.url));
./devtools/client/netmonitor/request-utils.js:136:  let name = NetworkHelper.convertToUnicode(
./devtools/client/netmonitor/request-utils.js:138:  let query = NetworkHelper.convertToUnicode(unescape(url.query));
./devtools/client/netmonitor/request-utils.js:147:  return NetworkHelper.convertToUnicode(unescape(url.hostPort));
./devtools/client/netmonitor/requests-menu-view.js:970:        let unicodeUrl = NetworkHelper.convertToUnicode(unescape(uri.spec));
./devtools/client/netmonitor/test/head.js:264:  let unicodeUrl = NetworkHelper.convertToUnicode(unescape(aUrl));
./devtools/client/netmonitor/test/head.js:265:  let name = NetworkHelper.convertToUnicode(unescape(uri.fileName || uri.filePath || "/"));
./devtools/client/netmonitor/test/head.js:266:  let query = NetworkHelper.convertToUnicode(unescape(uri.query));
./devtools/client/scratchpad/scratchpad.js:1126:          content = converter.ConvertToUnicode(aContent);
./devtools/server/actors/settings.js:35:  let rawstr = converter.ConvertToUnicode(NetUtil.readInputStreamToString(
./devtools/shared/DevToolsUtils.js:472:      // the guess is wrong, the conversion fails and convertToUnicode returns
./devtools/shared/DevToolsUtils.js:477:      let unicodeSource = NetworkHelper.convertToUnicode(source, charset);
./devtools/shared/transport/packets.js:165:    json = unicodeConverter.ConvertToUnicode(json);
./devtools/shared/webconsole/network-helper.js:87:  convertToUnicode: function (text, charset) {
./devtools/shared/webconsole/network-helper.js:92:      return conv.ConvertToUnicode(text);
./devtools/shared/webconsole/network-helper.js:110:      return this.convertToUnicode(text, charset);
./devtools/shared/webconsole/network-helper.js:789:          NetworkHelper.convertToUnicode(unescape(param[0])) : "",
./devtools/shared/webconsole/network-helper.js:791:          NetworkHelper.convertToUnicode(unescape(param[1])) : ""
./devtools/shared/webconsole/network-monitor.js:411:        NetworkHelper.convertToUnicode(data, request.contentCharset);
./dom/apps/AppsUtils.jsm:657:          let data = JSON.parse(converter.ConvertToUnicode(NetUtil.readInputStreamToString(aStream,
./dom/settings/SettingsDB.jsm:91:    let rawstr = converter.ConvertToUnicode(NetUtil.readInputStreamToString(
./services/common/utils.js:196:      str = this._utf8Converter.ConvertToUnicode(str);
./toolkit/components/passwordmgr/crypto-SDR.js:130:      plainText = this._utfConverter.ConvertToUnicode(plainOctet);
./toolkit/components/telemetry/tests/unit/head.js:144:    let utf8string = unicodeConverter.ConvertToUnicode(observer.buffer);
./toolkit/identity/tests/unit/test_crypto_service.js:107:    let result = utf8Converter.ConvertToUnicode(base64UrlDecode(target));

nsScriptableUnicodeConverter::ConvertFromByteArray:
./toolkit/components/places/BookmarkJSONUtils.jsm:203:            let jsonString = converter.convertFromByteArray(aResult,
./toolkit/components/places/BookmarkJSONUtils.jsm:240:      let jsonString = converter.convertFromByteArray(aResult, aResult.length);
./toolkit/components/places/tests/bookmarks/test_1016953-renaming-uncompressed.js:38:  let jsonString = converter.convertFromByteArray(result, result.length);
./toolkit/components/places/tests/bookmarks/test_1016953-renaming-uncompressed.js:68:  let jsonString = converter.convertFromByteArray(result, result.length);

For example, BookmarkJSONUtils.jsm calls convertFromByteArray while importing bookmarks, with the data obtained from either a compressed file or an URL, which can be provided by an attacker.

III. Exploitability

With an arbitrary heap overflow and some prior heap grooming, it's possible to overwrite metadata of some object (a storage slot of an Array, for example) and thereby obtain an arbitrary read-write exploitation primitive, which can be leveraged into both an info-leak and arbitrary code execution.

The bug was confirmed with the latest release source code of Firefox ESR.


Actual results:

code review

Comment 1

[:Gijs (he/him)]

Henri, can you take a look? Looks like this code also changed in bug 1261841, so it's not clear to me if 59 and later are vulnerable or not, and if this is serious enough to want to fix for esr52 given the comments about string length in bug 1440926.

Comment 2

[Henri Sivonen (:hsivonen)]

Filed bug 1444329 about the remaining trunk caller.

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to :Gijs (under the weather; responses will be slow) from comment #1)
> Henri, can you take a look? Looks like this code also changed in bug
> 1261841, so it's not clear to me if 59 and later are vulnerable or not

56 and later is already fixed.

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to :Gijs (under the weather; responses will be slow) from comment #1)
> if this is serious enough to want to fix for esr52 given the comments about
> string length in bug 1440926.

Given the comment there and what computation GetMaxLength can do in the bytes to UTF-16 direction, this is not exploitable using JS string input. There is so much abstraction in SpiderMonkey that Searchfox loses track. Hence, I timed out trying to figure out the possible max length of a Uint8Array. I'll try to find someone who knows where I should look.

Comment 5

[Henri Sivonen (:hsivonen)]

It seems that it's possible to have an Uint8Array whose length is INT32_MAX. To materialize one in practice, you need a 64-bit build, of course.

It looks like we no longer keep a searchable view of the add-ons on AMO--at least not at the previous location, so I can't search through the source of legacy add-ons for existence proof of a legacy add-on that used this API with an Uint8Array.

But if one existed and took 2 GB of attacker-supplied input into the Uint8Array somehow (in a 64-bit build), I don't see why the attack described in comment 0 wouldn't be possible.

Comment 6

[Masatoshi Kimura [:emk]]

Why legacy add-ons matter? Legacy add-ons can do almost anything they want without exploiting this bug.

Comment 7

[:Gijs (he/him)]

(In reply to Masatoshi Kimura [:emk] from comment #6)
> Why legacy add-ons matter? Legacy add-ons can do almost anything they want
> without exploiting this bug.

I think Henri's point is that well-intentioned legacy add-ons that wouldn't otherwise do anything untoward might be (and make us) vulnerable to exploitation because of this bug.

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Check for overflow

(In reply to :Gijs from comment #7)
> (In reply to Masatoshi Kimura [:emk] from comment #6)
> > Why legacy add-ons matter? Legacy add-ons can do almost anything they want
> > without exploiting this bug.
> 
> I think Henri's point is that well-intentioned legacy add-ons that wouldn't
> otherwise do anything untoward might be (and make us) vulnerable to
> exploitation because of this bug.

That's what I meant.

It takes less time to just fix this than to investigate whether a fix is really needed. Hence, here's a patch.

Comment 9

[Henri Sivonen (:hsivonen)]

Comment on attachment 8958370 [details] [diff] [review]
Check for overflow

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

This has been rated moderate and it's doubtful whether the conditions for exploiting the bug ever exist in the wild. Most notably, it's unclear if a legacy extension that calls nsIScriptableUnicodeConverter.convertFromByteArray() with Uint8Array argument exists. However, if the conditions existed in the wild, this would lead to sec-critical-like consequences.

> User impact if declined: 

If
 * The user is using a 64-bit ESR build
AND
 * A legacy extension that converts a JavaScript Uint8Array to a JS string using nsIScriptableUnicodeConverter.convertFromByteArray() exists.
AND
 * The user has the extension installed.
AND
 * The extension reads an unbounded amount of attacker-provided input into the Uint8Array.
AND
 * The attacker has previously caused the Firefox process to get a contiguous range of 1.3 GB or more of writable pages mapped to the addresses starting from the address that jemalloc returns for a zero-sized or tiny allocation. (AFAICT, otherwise the step of trying to convert 2 GB of bytes into UTF-16 gets the process terminated on accessing a write-only page or an unmapped page before anything worse can happen. I don't know how many write-only guard pages we sprinkle around.)

THEN

Memory-safety ensues, the attacker gets to overwrite a massive part of the heap with almost any data of the attackers choice (any byte pattern except unpaired surrogates) and the usual badness that comes with such overwriting is possible.

> Fix Landed on Version:

This was fixed in 56 in a different way. This ESR-only patch hasn't landed (and won't land) on any non-ESR branch.

> Risk to taking this patch (and alternatives if risky): 

Extremely low risk.

> String or UUID changes made by this patch: 

None.

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #9)
> Memory-safety ensues

unsafety

Comment 11

[Henri Sivonen (:hsivonen)]

Considering that this is not going to land on trunk regardless of ESR approval, are the request flags here such that this shows up on the right queries? (I.e. is the lack of either + or - here a matter of just not having decided yet or a matter of falling off queries, because there's no trunk landing status?)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

It shows up, we're just not looking at requests for 52.8 yet.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8958370 [details] [diff] [review]
Check for overflow

Harden ESR52 against possible overflow conditions. Approved for 52.8.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/17201199b18d

Comment 15

[Daniel Veditz [:dveditz]]

Going to minus for the bug bounty because there's no risk to most Firefox. Users would have to have installed an unintentionally malicious legacy add-on (it it's intentionally malicious it already has all the power) that uses this feature in a way content can influence.

Comment 16

[Henri Sivonen (:hsivonen)]

Can we unhide this, considering that the last ESR that required keeping this hidden was 52?