1182277 - Memory-safety bug in nsDOMMutationObserver (via nsTArray)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[q1]

Class nsDOMMutationObserver (dom\base\nsDOMMutationObserver.cpp) hasa member of the type:

  static nsAutoTArray<nsAutoTArray<nsRefPtr<nsDOMMutationObserver>, 4>, 4> *
     sCurrentlyHandlingObservers;

This means that both the outer nsAutoTArray and the inner nsAutoTArrays have inline storage, to which their respective mHdr pointers point.

If a 5th element is added to the outer nsAutoTArray, nsAutoTArray reallocates the inner nsAutoTArrays from inline storage to heap storage ("the new heap block"). However, it does this by using memcpy. This means that the inner nsAutoTArrays' mHdr pointers still point to their old inline storage, not to their new inline storage inside the newly-allocated heap block. This eventually causes the heap manager to overwrite memory that it shouldn't, and then crash. The overwriting could affect data used by other threads that might malfunction insecurely before the crash.

The details are:

When an element in the range 0-3 is removed from the outer nsAutoTArray, as by nsDOMMutationObserver::LeaveMutationHandling, RemoveElementAt is called for the outer element that is removed. Imagine this to be element 3. RemoveElementAt calls DestructRange (3,1), which eventually results in a call to ~nsTArray_base for element 3.

~nsTArray_base contains this code:

  if (mHdr != EmptyHdr() && !UsesAutoArrayBuffer()) {
    Alloc::Free(mHdr);
  }
  MOZ_COUNT_DTOR(nsTArray_base);

Now mHdr points back at element 3's original inline storage, _not_ at its unused inline storage inside the new heap block.

The first clause is true, so control enter UsesAutoArrayBuffer, which contains this code:

  if (!mHdr->mIsAutoArray) {
    return false;
  }
  ...
  return mHdr == GetAutoArrayBuffer(4) || mHdr == GetAutoArrayBuffer(8);

But GetAutoArrayBuffer returns a (potentially slightly doctored) pointer to the (unused) inline storage that's relative to |this|. The resulting pointer is of course different from mHdr, which still points back at the original inline storage. So UsesAutoArrayBuffer returns false.

At this point, ~nsTArray_base calls Alloc::Free(mHdr), with mHdr pointing back at the original inline storage. This storage was never allocated on its own, but only as part of the larger block that contained the entire 2-dimensional nsAutoTArray. The heap manager then fills memory, beginning at mHdr, with 0x80 bytes of 0x5a (the original allocation was ~0x80 bytes), probably stomping some other data structures that could be used by other threads, then (too late!) calls jemalloc_crash on the line:

	RELEASE_ASSERT(diff == regind * size);

in memory\mozjemalloc\jemalloc.c.

I have verified all of the above by doctoring execution in the debugger, but don't know whether it is possible, in practice, to cause nsDOMMutationObserver to add a 5th element to sCurrentlyHandlingObservers, which is necessary to trigger the problem.

The basic cause of the problem is that nsTArray defaults to copying elements with memcpy instead of using copy constructors.

This bug would have caused greater problems (corruption without a crash) had nsDOMMutationObserver inserted or removed elements from the outer nsAutoTArray at other than the end.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Would have been caught by bug 1159433 :/

Comment 2

[Boris Zbarsky [:bzbarsky]]

Caused by bug 976367.

We need to either revert part of that or add a CopyChooser specialization for nsAutoTArray, right?

Comment 3

[Nathan Froyd [:froydnj]]

If we do add CopyChooser specializations, we probably need to take some thought and decide how to minimally impact all the other places that use nsTArray<nsRefPtr<T>>...nsCOMPtr<T> elements, too..

Comment 4

[q1]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #1)
> Would have been caught by bug 1159433 :/

Yeah. I didn't see that. Technically, it invokes undefined behavior to memmove anything but standard layout objects, and even those aren't necessarily safe because they can contain self-pointers (e.g., linked lists). I would take the safest route -- always using copy/move constructors/assignment operators -- at potentially some performance cost.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I'm rather worried we have similar issues elsewhere.

Comment 6

[Andrew McCreight [:mccr8]]

Yeah, the description in comment 0 sounds a little close to bug 1162024 (formerly bug 997908).

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

my testcase is

  nsAutoTArray<nsAutoTArray<nsRefPtr<nsDOMMutationObserver>, 4>, 4> test;
  test.AppendElement();
  printf("0: %p \n", test.Elements());
  printf("0:0, %p \n", test[0].Elements());
  test.AppendElement();
  printf("1: %p \n", test.Elements());
  printf("1:0, %p \n", test[0].Elements());
  test.AppendElement();
  printf("2: %p \n", test.Elements());
  printf("2:0, %p \n", test[0].Elements());
  test.AppendElement();
  printf("3: %p \n", test.Elements());
  printf("3:0, %p \n", test[0].Elements());
  test.AppendElement();
  printf("4: %p \n", test.Elements());
  printf("4:0, %p \n", test[0].Elements());

and on current trunk that crashes when test goes out of scope.
Also, printf("4:0, %p \n", test[0].Elements()); prints the same value as 
printf("3:0, %p \n", test[0].Elements());, so the same storage is used even after making the outer nsAutoTArray to use heap storage.

+template<class E, size_t N>
+struct nsTArray_CopyChooser<nsAutoTArray<E, N>>
+{
+  typedef nsTArray_CopyWithConstructors<nsAutoTArray<E, N>> Type;
+};
+
seems to fix the issue.


Note, it is not clear to me how to get sCurrentlyHandlingObservers to actually use heap storage, since it needs nested DOM mutations.
We do have some nested mutations, and MutationObserver treats also .innerHTML setter and such as a DOM Mutation, so the level might be 2, but I don't know now how to get >4 level deep mutations.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch v1

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(CCing jonco since he wrote some of that copy handling in Bug 1154796 )

Comment 10

[(no longer active)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #1)
> Would have been caught by bug 1159433 :/

Yes, looking at the code I think so too, but I have asked Michael to look at why not...

Comment 11

[Nika Layzell [:nika] (ni? for response)]

So, the reason why this wasn't caught by 1159433 was because that patch only annotates nsTAutoString_CharT as MOZ_NON_MEMMOVABLE, and thus doesn't annotate nsAutoArrayBase as MOZ_NON_MEMMOVABLE. As far as I know, there is no good way to determine if a type is memmovable without annotating it as such, so we definitely need to do that.

Before bug 1159433 lands, we should probably make a point of going through the base and discovering the values which are not memmovable for various reasons, and marking them as such.  If we miss a type which is not memmovable, but should be marked as not memmovable, then that is a relatively serious problem.

The other side of the coin which we could try for is switching MOZ_NON_MEMMOVABLE with MOZ_MEMMOVABLE. We could then add the annotation to values which we know are memmovable (non-class values would probably be assumed to be memmovable).  This would mean that it would take more time to validate every use of nsTArray before we could land the patch, which could be unfortunate, and also mean that people can't use nsTArray without special casing the value which they want to use as an element (unless the element has been used before).

Comment 12

[Jon Coppeard (:jonco)]

The fix looks good.  I think we want the same thing for AutoFallibleTArray too.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I thought fallible TArrays are about to go away.
(and that we'd just have fallible methods)

Comment 14

[Nika Layzell [:nika] (ni? for response)]

I marked nsAutoArrayBase as MOZ_NON_MEMMOVABLE, and it caught this bug with the following error:

 9:29.69 In file included from /Volumes/Devel/Code/mozilla/bug1159433/obj-clang-x86_64-apple-darwin14.3.0/dom/base/Unified_cpp_dom_base4.cpp:2:
 9:29.69 In file included from /Volumes/Devel/Code/mozilla/bug1159433/dom/base/nsContentSink.cpp:13:
 9:29.69 In file included from /Volumes/Devel/Code/mozilla/bug1159433/dom/base/nsScriptLoader.h:15:
 9:29.69 In file included from /Volumes/Devel/Code/mozilla/bug1159433/dom/base/nsIScriptElement.h:15:
 9:29.69 In file included from ../../dist/include/nsIParser.h:25:
 9:29.69 ../../dist/include/nsTArray.h:664:34: error: Cannot instantiate 'nsTArray_CopyChooser<nsAutoTArray<nsRefPtr<nsDOMMutationObserver>, 4> >' with non-memmovable template argument 'nsAutoTArray<nsRefPtr<nsDOMMutationObserver>, 4>'
 9:29.69 struct MOZ_NEEDS_MEMMOVABLE_TYPE nsTArray_CopyChooser
 9:29.69                                  ^
 9:29.69 ../../dist/include/nsTArray.h:795:42: note: instantiation of 'nsTArray_CopyChooser<nsAutoTArray<nsRefPtr<nsDOMMutationObserver>, 4> >' requested here
 9:29.69   : public nsTArray_base<Alloc, typename nsTArray_CopyChooser<E>::Type>
 9:29.69                                          ^
 9:29.69 ../../dist/include/nsTArray.h:2162:26: note: 'nsAutoTArray<nsRefPtr<nsDOMMutationObserver>, 4>' is non-memmovable because of the MOZ_NON_MEMMOVABLE annotation on 'nsAutoArrayBase<nsTArray<nsRefPtr<nsDOMMutationObserver> >, 4>'
 9:29.69 class MOZ_NON_MEMMOVABLE nsAutoArrayBase : public TArrayBase
 9:29.69                          ^

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8631884 [details] [diff] [review]
patch v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I don't know how to trigger the crash without some manual C++ tests, but perhaps there is some way to have deep nested DOM mutations.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Commit message could be something vague: "Bug 1182277, don't leak when using nsAutoTArray inside nsTArray, r=nfroyd"


Which older supported branches are affected by this flaw?
Everything after 30

If not all supported branches, which bug introduced the flaw? bug 976367

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch applies esr38-...-trunk

How likely is this patch to cause regressions; how much testing does it need?
Regressions should be unlikely. I tested locally various mutation observer tests, and also wrote some C++ to trigger the crash and then ensured the patch fixed that.


Approval Request Comment
[Feature/regressing bug #]: bug 976367
[User impact if declined]: Security sensitive crash, at least in theory. I don't know how to actually trigger the crash (except manually writing some C++).
[Describe test coverage new/current, TreeHerder]: manually tested on trunk and esr38
[Risks and why]:  Should be low risk given that we should see crashes all the time if we actually increased the size of the outer array to be > 4
[String/UUID change made/needed]: NA

Comment 16

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #11)
> So, the reason why this wasn't caught by 1159433 was because that patch only
> annotates nsTAutoString_CharT as MOZ_NON_MEMMOVABLE, and thus doesn't
> annotate nsAutoArrayBase as MOZ_NON_MEMMOVABLE. As far as I know, there is
> no good way to determine if a type is memmovable without annotating it as
> such, so we definitely need to do that.

Oh, yes makes sense!

> Before bug 1159433 lands, we should probably make a point of going through
> the base and discovering the values which are not memmovable for various
> reasons, and marking them as such.  If we miss a type which is not
> memmovable, but should be marked as not memmovable, then that is a
> relatively serious problem.

Let's fix the cases that we find.  When we land our fixes and the analysis, we can ask people to add the annotations in their own code.  There is no way that one person can know what all of the nonmovable types in the code base are!  :-)

Comment 17

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

Olli, do you have any ideas on a suggested security rating for this, since we don't know how to trigger it?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I would say sec-other, based on the information we have now.

Comment 19

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

Comment on attachment 8631884 [details] [diff] [review]
patch v1

Clearing sec-approval (since it isn't needed).

We'd probably just let this ride the trains from trunk.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Never remember the bot doesn't update security bugs automatically
https://hg.mozilla.org/integration/mozilla-inbound/rev/92c9a0d98ea9

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/92c9a0d98ea9

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8631884 [details] [diff] [review]
patch v1

Based on Al's comment we will let this patch ride the train. Rejecting uplift request.