1431142 - Crash in xul!icu_58::DecimalFormat::updatePrecision (likely OOM)

Status: NEW

(Core :: JavaScript: Internationalization API, defect, P2)

Description

[A.]

Attachment: crash.log

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20171208114127

Steps to reproduce:

Tested on Firefox ESR 52.5.3 (32-bit) on Windows 10 x32

Firefox crashes reliably when executing the following Javascript:

function test() {
    o = new Uint8ClampedArray(2**16) 

    for ( var k = 0; k < 10; k++ ) {
        o.toLocaleString('hi')
   }
}

The primary crash is due to a near null dereference in xul!icu_58::DecimalFormatImpl::updatePrecision (see crash.log). In some cases (~10%) the same test case triggers a mozalloc_abort, suggesting that the root cause may be an uncaught OOM. (oom.log)

(e54.109c): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=00000001 ebx=008fe114 ecx=00000000 edx=00000001 esi=7fd8ace0 edi=0dad2640
eip=5672971f esp=008fe004 ebp=008fe004 iopl=0         nv up ei ng nz na po cy
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00210283
xul!icu_58::DecimalFormatImpl::updatePrecision [inlined in xul!icu_58::DecimalFormat::setMinimumIntegerDigits+0x22]:
5672971f 80b98800000000  cmp     byte ptr [ecx+88h],0       ds:0023:00000088=??
0:000> k
 # ChildEBP RetAddr  Args to Child              
00 (Inline) -------- -------- -------- -------- xul!icu_58::DecimalFormatImpl::updatePrecision c:\builds\moz2_slave\m-esr52-w32-000000000000000000\build\src\intl\icu\source\i18n\decimfmtimpl.cpp @ 892]
01 004fe0c4 5c198199 00000001 004fe1d4 00000004 xul!icu_58::DecimalFormat::setMinimumIntegerDigits+0x22 c:\builds\moz2_slave\m-esr52-w32-000000000000000000\build\src\intl\icu\source\i18n\decimfmt.cpp @ 2895]
02 004fe0d8 5c1d3a60 00000004 00000001 004fe0f0 xul!icu_58::DecimalFormat::setAttribute+0x9c c:\builds\moz2_slave\m-esr52-w32-000000000000000000\build\src\intl\icu\source\i18n\decimfmt.cpp @ 3100]
03 004fe0f4 5d4a7cab 00000001 004fe248 00000000 xul!unum_setAttribute_58+0x38 c:\builds\moz2_slave\m-esr52-w32-000000000000000000\build\src\intl\icu\source\i18n\unum.cpp @ 514]
04 004fe1d4 5d4ac67d 004fe204 03d55000 7d493000 xul!NewUNumberFormat+0x58f c:\builds\moz2_slave\m-esr52-w32-000000000000000000\build\src\js\src\builtin\intl.cpp @ 1590]
05 004fe21c 048abd46 03d55000 00000002 004fe248 xul!js::intl_FormatNumber+0x5c c:\builds\moz2_slave\m-esr52-w32-000000000000000000\build\src\js\src\builtin\intl.cpp @ 1650]
... js stack or JIT ...

What happens here is the test case fetches dozens of DecimalFormat instances from the UnifiedCache (via intl_FormatNumber->...->NumberFormat::createInstance), and at some point it receives a malformed instance which has fImpl set to zero. Then, ::updatePrecision tries to dereference the pointer to the DecimalFormatImpl member of the malformed DecimalFormat instance (which is fImpl) and crashes. 

I tried to confirm the root cause, but got lost in the internals of caching. Heuristically, it seems that the evil DecimalFormat instance was not properly initialized by the constructor, i.e. created and fImpl set to zero, but the DecimalFormatImpl member probably failed to allocate while throwing no error.  

//-----------------------------------------------------------------------------
// Common DecimalFormat initialization.
//    Put all fields of an uninitialized object into a known state.
//    Common code, shared by all constructors.
//    Can not fail. Leave the object in good enough shape that the destructor
//    or assignment operator can run successfully.
void
DecimalFormat::init() {
    fBoolFlags.clear();
    fStyle = UNUM_DECIMAL;
    fAffixPatternsForCurrency = NULL;
    fCurrencyPluralInfo = NULL;
#if UCONFIG_HAVE_PARSEALLINPUT
    fParseAllInput = UNUM_MAYBE;
#endif

    fStaticSets = NULL;
    fImpl = NULL;  <<<<<<<<<<<<<<<<<< this is supposed to hold a ptr to DecimalFormatImpl before using the instance
}

Marking this closed just in case, since I did not exclude possible exploitability without precisely understanding the root cause, while OOM bugs may be exploitable in theory (see for instance, #635705, #1201183, #1220450).

I will follow up on your investigation and check again if this might be exploitable, once there is enough information available.


Actual results:

Crash


Expected results:

No crash

Comment 1

[A.]

Attachment: oom.log

Comment 2

[Andrew McCreight [:mccr8]]

Jeff, could you look into this to see if there's a security issue here?

Comment 3

[Jeff Walden [:Waldo]]

Hrm.  If comment 0 is to be believed, and the analysis seems plausible, a failure to set |fImpl| on OOM might be at fault.  Looking through places that use that field, there appear to only be a few places that set it.

The one in DecimalFormat::construct properly handles failure via a |status| outparam:

https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/intl/icu/source/i18n/decimfmt.cpp#407

And there's only one other, DecimalFormat::operator=:

https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/intl/icu/source/i18n/decimfmt.cpp#627

But that case is only called in CompactDecimalFormat::operator= -- and unless searchfox is mistaken, that function isn't even called anywhere.

So, still not sure what's up here.  Continuing to investigate.

Comment 4

[Jeff Walden [:Waldo]]

So, there's kind of a big problem with all this code.

ICU's APIs that can fail, return the thing you want, and they have a |UErrorCode&| inoutparam.  It's a general rule that on entry the inoutparam is checked for failure, and if so the call returns.  So *even in the failure-handling |fImpl|-assigning code*, you can partially initialize a DecimalFormat.  And that partially initialized DecimalFormat can permissibly be used, as long as those uses check |status| for failure before doing anything.

Problem is, some places don't check for failure before dereferencing |fImpl|.  And probably same for other classes.  Consider, for example, this code:

  // This does
  //    init();
  //    UParseError parseError;
  //    construct(status, parseError);
  // where |init()| safely initializes things to destructible/assignable
  // state, then |construct| performs a long series of to init fields
  // for real.
  UErrorCode status = U_ZERO_ERROR;
  DecimalFormat fmt(status);

  // Suppose |construct()| failed before initializing any fields (as it
  // may) -- in whic case |!fImpl|.  Inside |format()| we have
  //    return fImpl->format(number, appendTo, fieldPosition, status);
  // which -- despite that DecimalFormatImpl::format has a status-check --
  // will dereference |fImpl| first, which is UB.  Given a smart enough
  // compiler/linker, it might thus assume |fImpl| is non-null.
  UnicodeString str;
  FieldPosition fp;
  fmt.format(1.0, str, fp, status);

I don't doubt there are a bunch of other places like this.  The code is not easily audited for this problem in its most general form.  :-\

Fixing this is doable.  It is very much not a fix I want to undertake in our code, without it being upstreamed.  Upstream would have to review it and be involved in fixing it, for sure, because this is a lot of understanding of how the fields of DecimalFormat are initialized and used, to account for.

So is this a security issue?  ¯\_(ツ)_/¯  I would guess -- *guess* -- it is likelier to be just a null-deref than not.  I am not willing to stake anything on that assessment, and neither should you.  A very stupid hack-"fix" would be to make the various failure possibilities initializing DecimalFormat fields just crash.  But I don't know that all of them are purely due to ICU internal errors like OOM, none influenced by user-controllable arguments.  And that does nothing for system-ICU users.

I think about the best we can do about this is spend awhile writing a patch for this upstream, getting it reviewed there and then backporting it into our tree, or file a bug with them and hope they fix it.  It isn't a great use of time, IMO, for me to spend awhile neck-deep in this fixing it.  Although that *is* possible, if we really think it necessary.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Jeff Walden [:Waldo] from comment #4)
> file a bug with them and hope they fix it.

Do we have a link to ICU bug tracker, where this issue is forwarded?

> It isn't a great use of time, IMO, for me to spend awhile neck-deep in this fixing it. 

Should this remain as a P1?
Otherwise, maybe, we can ask Steven if there would be a way to get a contractor to fix this issue upstream?

Comment 6

[Nicolas B. Pierron [:nbp]]

I was able to reproduce a different failure based on the test case from comment 0:


oomTest(function test() {
    o = new Uint8ClampedArray(2**8);
    o.toLocaleString('hi')
});



Assertion failure: cx->isExceptionPending() (Thunk execution failed but no exception was raised - missing call to js::ReportOutOfMemory()?), at …/js/src/builtin/TestingFunctions.cpp:1790

Comment 7

[Nicolas B. Pierron [:nbp]]

Steven, can you check with Waldo:
 - If we need a contractor to address this issue?
 - When it should be addressed? (should it remain as a P1)

Comment 8

[Jeff Walden [:Waldo]]

I don't think we need a contractor to address this; it doesn't require particular i18n- or ICU-relevant knowledge, just a willingness to add some error handling along relatively normal ICU styles to ICU upstream, including getting it through their review process.

With --enable-oom-breakpoint I find this as the proximate cause of comment 6's issue:

469	static ArrayBufferObject::BufferContents
470	AllocateArrayBufferContents(JSContext* cx, uint32_t nbytes)
471	{
472	    uint8_t* p = cx->pod_callocCanGC<uint8_t>(nbytes,
473	                                                      js::ArrayBufferContentsArena);
474	    return ArrayBufferObject::BufferContents::create<ArrayBufferObject::PLAIN>(p);
475	}

and at least some callers just act like an error was reported.  But that doesn't have anything to do with the ICU thingy.  I may as well fix it now, but definitely it deserves a separate bug.

Comment 9

[Jeff Walden [:Waldo]]

Filed bug 1498458 on comment 6 and comment 8.

Comment 10

[Nicolas B. Pierron [:nbp]]

Waldo, with Bug 1498458 out of the way, when would you be able to work on this ICU patch?

Comment 11

[Jason Orendorff [:jorendorff]]

Memo to myself to straighten out with Waldo how much this is a problem for ICU upstream, and how to proceed.

Comment 12

[Jason Orendorff [:jorendorff]]

This doesn't reproduce for me in Nightly. Instead, the test case just takes 10+ seconds to run in an optimized build. Filed open bug 1514851 about that (i.e. it being slow).

Waldo indicated that he is unlikely to get around to this any time soon. It's lower priority since it isn't in Nightly and isn't exploitable.

It bugs me to leave stuff like this for later, though, so I am leaving P1 for now and will revisit in the new year when I have more time.

Comment 13

[Jason Orendorff [:jorendorff]]

I don't actually have time for this. P2 at best.

Comment 14

[Jeff Walden [:Waldo]]

I think this is a duplicate of bug 1495244 (or the other way around, if you prefer). That, or that bug is roughly the same sort of issue but in different code. Bug 1495244 comment 5 reaches the same sort of conclusion as I did here: "I don't think we can (successfully) hack around this without changing dozens of places in ICU. :-/"

Arguably a new duplicate suggests importance should be bumped, but the volume of effort required for a proper fix for this still makes actually doing the work non-trivial.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

removing 4 years old NI.