Closed Bug 993546 Opened 6 years ago Closed 6 years ago

memory reporter wrappers for realloc can use-after-free

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 + fixed
firefox30 + fixed
firefox31 + fixed
firefox-esr24 + fixed
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main29+][adv-esr24.5+][qa-])

Attachments

(6 files, 7 obsolete files)

23.64 KB, patch
froydnj
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
23.65 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
17.79 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.75 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
16.37 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
14.69 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
We have a number of memory reporter wrappers for realloc in nsXPComInit.cpp, here's the one for ICU as an example:

http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPComInit.cpp#365

This wrapper (and the other ones) don't correctly handle the case of being called with a zero size; |realloc| acts like |free| in such a case, and therefore the "realloc failed" case would be touching already-freed memory.

We should fix the wrappers and possibly backport.

Marking as security-sensitive, but I don't know how exploitable this really is.
Assignee: nobody → nfroyd
realloc, how I loathe thee.
> This wrapper (and the other ones) don't correctly handle the case of being
> called with a zero size; |realloc| acts like |free| in such a case

Except when it doesn't. This is a platform-dependent thing, horribly enough. From the man page on my Mac:

"If size is zero and ptr is not NULL, a new, minimum sized object is allocated and the original object is freed."

See comment 1.
(In reply to Nicholas Nethercote [:njn] from comment #2)
> > This wrapper (and the other ones) don't correctly handle the case of being
> > called with a zero size; |realloc| acts like |free| in such a case
> 
> Except when it doesn't. This is a platform-dependent thing, horribly enough.
> From the man page on my Mac:
> 
> "If size is zero and ptr is not NULL, a new, minimum sized object is
> allocated and the original object is freed."

This is annoying, but this beheavior is taken care of by the non-NULL return case.  The general case looks like:

  p = realloc(...);
  if (p) {
    // allocation succeeded
  } else if (size == 0) {
    // something got freed
  } else {
    // allocation failed
  }

It occurred to me this morning that maybe ASan builds are special insofar as they don't use jemalloc and just use whatever system malloc is available.  But it looks like jemalloc will return NULL for the size==0 case unless the passed in pointer is NULL, in which case a minimum-sized object is returned, just like Mac.  So I think this does apply to release builds. :(
I'm attaching these as separate patches in case we need to backport these.
They are copy-pasting the same bits in the appropriate locations.
Attachment #8403977 - Flags: review?(n.nethercote)
Attachment #8403978 - Flags: review?(n.nethercote)
Attachment #8403979 - Flags: review?(n.nethercote)
Keywords: sec-high
What about this case?

- We're on Mac, not using jemalloc, which means that realloc() never acts like free().

- We call realloc(p, 0), where p is non-NULL

- realloc attempts a minimum-sized allocation, fails, and returns NULL

We'll enter your "nothing to do here" case, and we will have incorrectly decremented sAmount.

If you don't know in advance if your realloc() can act like free(), you can't handle the above case.
I guess in practice this is unlikely, since realloc(p, 0) is always a same-sized or shrinking realloc, and so should never actually never. But still, comment 1 applies again! I think it's not a big deal if we get this case wrong, but a comment about it would be good.

Since this code is tricky, and it shows up in multiple places, could you factor it out somewhere? You'd probably need two versions, for atomic and non-atomic counters. Is that overkill?
(In reply to Nicholas Nethercote [:njn] from comment #7)
> If you don't know in advance if your realloc() can act like free(), you
> can't handle the above case.

Bleh.  I see that our Mac mozconfigs don't enable jemalloc, so this is non-theoretical.

> Since this code is tricky, and it shows up in multiple places, could you
> factor it out somewhere? You'd probably need two versions, for atomic and
> non-atomic counters. Is that overkill?

I think everything uses atomic counters, possibly unnecessarily.  But for practicality's sake, and because this code is unlikely to be terribly hot, I think it's OK if we do atomic counters always.

I'll rewrite to use common code tomorrow.
I threw everything together in one patch this time, given that we're
refactoring things anyway.  I'll separate things out for branch uplifts if
that's what we want to do.  I called the base class functions Counting$FOO so
they can be used for ICU-style/zlib-style allocators.  Requires a little
boilerplate for other allocator styles, but nothing horrendous.

I think we *could* detect whether the allocation of the minimum-sized object
failed by checking errno for ENOMEM.  Certainly Apple's malloc implementation
appears to set this if problems occur.  I don't know whether we want to be so
picky, but at least we only have to change it in one place if we decide to do
that!
Attachment #8403977 - Attachment is obsolete: true
Attachment #8403978 - Attachment is obsolete: true
Attachment #8403979 - Attachment is obsolete: true
Attachment #8403977 - Flags: review?(n.nethercote)
Attachment #8403978 - Flags: review?(n.nethercote)
Attachment #8403979 - Flags: review?(n.nethercote)
Attachment #8405449 - Flags: review?(n.nethercote)
I realized post-request that we can probably move the #ifdef DEBUG checks into MemoryReporterBase, where it makes more sense.  Not going to upload a new patch for that right now, though.
Comment on attachment 8405449 [details] [diff] [review]
refactor malloc-wrapping memory reporter implementations

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

This is great. Thanks for doing it.

Yes, moving the DEBUG checks would be good.

As well as hunspell, FreetypeReporter in gfx/thebes/gfxAndroidPlatform.cpp has the same issue. You can move the new class into the C++ section of xpcom/base/nsIMemoryReporter.idl to make it accessible in all the necessary places.

::: xpcom/build/nsXPComInit.cpp
@@ +341,5 @@
>      return NS_InitXPCOM2(result, binDirectory, nullptr);
>  }
>  
> +template<typename T>
> +class MemoryReporterBase

This name is too generic; it should be called CountingMemoryReporterBase or CountingMemoryReporter.

@@ +345,5 @@
> +class MemoryReporterBase
> +{
> +protected:
> +    static size_t
> +    GetAmount()

Does the |Get| prefix imply fallibility? I can't remember. If so, Amount() might be a better name.

@@ +370,5 @@
> +    static void*
> +    CountingRealloc(void* p, size_t size)
> +    {
> +        size_t psize = MallocSizeOfOnFree(p);
> +        sAmount -= psize;

Now that you've introduced |psize|, you could postpone the update of sAmount until after pnew/size checking occurs, which would avoid the decrement-then-reincrement case. Up to you.
Attachment #8405449 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #12)
> Yes, moving the DEBUG checks would be good.

OK.

> As well as hunspell, FreetypeReporter in gfx/thebes/gfxAndroidPlatform.cpp
> has the same issue. You can move the new class into the C++ section of
> xpcom/base/nsIMemoryReporter.idl to make it accessible in all the necessary
> places.

Yup.

> > +template<typename T>
> > +class MemoryReporterBase
> 
> This name is too generic; it should be called CountingMemoryReporterBase or
> CountingMemoryReporter.

Yeah, it's fine in the current context, but moving it out for a wider base to use needs a better name.  I'll flip a coin and pick one.

> @@ +345,5 @@
> > +class MemoryReporterBase
> > +{
> > +protected:
> > +    static size_t
> > +    GetAmount()
> 
> Does the |Get| prefix imply fallibility? I can't remember. If so, Amount()
> might be a better name.

It does.  I'll rename this.

> 
> @@ +370,5 @@
> > +    static void*
> > +    CountingRealloc(void* p, size_t size)
> > +    {
> > +        size_t psize = MallocSizeOfOnFree(p);
> > +        sAmount -= psize;
> 
> Now that you've introduced |psize|, you could postpone the update of sAmount
> until after pnew/size checking occurs, which would avoid the
> decrement-then-reincrement case. Up to you.

Good point.  I'll do that.

Do you want to see another patch with the move to nsIMemoryReporter.idl and the replacing of the other two places you mentioned?

I have seen things like sec-approval? being tossed around, but as this is my first security bug, I have no idea what actually goes on for these.  Do I need to ask for special approval prior to landing?
Flags: needinfo?(n.nethercote)
Flags: needinfo?(continuation)
> I have seen things like sec-approval? being tossed around, but as this is my first security bug, I have no idea what actually goes on for these.  Do I need to ask for special approval prior to landing?

For sec-high or sec-crit bugs that affect more than just trunk, you need to get sec-approval before landing.  So I think that would apply to this bug.  Just fill out the little form that appears when you add sec-approval? on the patch.  Al Billings checks the approvals once or twice a day during the week.
Flags: needinfo?(continuation)
> Do you want to see another patch

I'll take a look at the patch you upload for sec-approval, but no need ask for re-review.
Flags: needinfo?(n.nethercote)
OK, here's the patch that handles hunspell and gfxAndroidPlatform.  Things that
one could bikeshed:

- I called the class CountingAllocatorBase, since there's one place (and will
  be others, see below) where we're not attaching this to a memory reporter.

- I made the static memory wrapping functions in CountingAllocatorBase public.
  This removes one level of indirection for the common case and requires less
  boilerplate.

- The mozHunspellAllocator.h file is so that we don't have to include
  hunspell_alloc_hooks.h in mozHunspell.{h,cpp}.  I don't know that it really
  matters, but we can modify later if need be.  (I guess hunspell_alloc_hooks.h
  already gets force-included on mozHunspell.cpp anyway?)

My favorite color for the bikeshed is blue.

I grepped for MOZ_DEFINE_MALLOC_SIZE_OF_ON_ALLOC to see if there were other
places that needed attention.  SpdyZlibReporter and GfxMemoryImageReporter
don't wrap realloc.  mozStorageService.cpp does, but as it only wraps realloc
under MOZ_DMD, and therefore not in release builds, it's not critical to
address it.  I will address all of these in some follow-up bug.  (Feedback
welcomed on whether or not they should just be changed to have everything done
all at once.)

Builds on Linux64 and Android.  Carrying over njn's r+, will sec-approval?
shortly.
Attachment #8405449 - Attachment is obsolete: true
Attachment #8406193 - Flags: review?(n.nethercote)
Comment on attachment 8406193 [details] [diff] [review]
refactor malloc-wrapping memory reporter implementations

(Really carrying over r+ in passing...)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The problems are all related to third-party libraries, and exploiting the bug requires specific arguments requesting 0-sized allocations to be passed to any realloc calls in those libraries to trigger the use-after-free.  I plead ignorance on how difficult this actually is; my first instinct is to say difficult, but exploit writers are far cleverer than I.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The large comment in CountingAllocatorBase::CountingRealloc might indicate problems, yes.  But the branch that it's commenting on doesn't exist prior to the patch, so a sharp-eyed patch reviewer would probably figure out that is the interesting bit anyway.

Which older supported branches are affected by this flaw?

B2G18, ESR24, and all later branches exhibit the flaw in one way or another.  (The memory reporter bits for libogg and vpx are new in Firefox 30.  The memory reporting bits for ICU were only in...Firefox 26?  Not sure when the Android gfx memory reporter bits were added.  hunspell memory reporting has been around for a while.)

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

(Several different bugs were responsible for this.)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch cannot be backported as-is, as some of the features didn't not exist on older branches.  Backports are straightforward and non-risky to create.

How likely is this patch to cause regressions; how much testing does it need?

It is unlikely to cause regressions.  Full testing on linux64 for a similar sort of problem has been performed on try in service of a different bug (bug 682216).
Attachment #8406193 - Flags: sec-approval?
Attachment #8406193 - Flags: review?(n.nethercote)
Attachment #8406193 - Flags: review+
Comment on attachment 8406193 [details] [diff] [review]
refactor malloc-wrapping memory reporter implementations

sec-approval+ to land on trunk.

Please create and nominate Aurora, Beta, and ESR24 patches. Once it is green on trunk and these are approved, you can land there. We're running out of time to do this (it is late in cycle) so doing it ASAP would be best.
Attachment #8406193 - Flags: sec-approval? → sec-approval+
Attached patch ESR24 patch (obsolete) — Splinter Review
Attachment #8406317 - Flags: review+
Attached patch Beta patch (obsolete) — Splinter Review
Attachment #8406319 - Flags: review+
Attached patch Aurora patch (obsolete) — Splinter Review
Attached patch Aurora patchSplinter Review
Ideally getting the author for c-n correct this time.
Attachment #8406320 - Attachment is obsolete: true
Attachment #8406328 - Flags: review+
Attached patch Beta patchSplinter Review
Attachment #8406319 - Attachment is obsolete: true
Attachment #8406334 - Flags: review+
Attached patch ESR24 patchSplinter Review
Attachment #8406317 - Attachment is obsolete: true
Attachment #8406335 - Flags: review+
Comment on attachment 8406335 [details] [diff] [review]
ESR24 patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible security risks
Fix Landed on Version: mozilla31
Risk to taking this patch (and alternatives if risky): Not a risky patch.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8406335 - Flags: approval-mozilla-esr24?
Comment on attachment 8406334 [details] [diff] [review]
Beta patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Many bugs.  bug 677653 and bug 682216 are the most recent.
User impact if declined: Possible security risk.
Testing completed (on m-c, etc.): Will land on trunk.  Local testing and testing on try for related problems.
Risk to taking this patch (and alternatives if risky): Low risk.
String or IDL/UUID changes made by this patch: None.
Attachment #8406334 - Flags: approval-mozilla-beta?
Comment on attachment 8406328 [details] [diff] [review]
Aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Many bugs.  bug 677653 and bug 682216 are the most recent.
User impact if declined: Possible security risk.
Testing completed (on m-c, etc.): Will land on trunk.  Local testing and testing on try for related problems.
Risk to taking this patch (and alternatives if risky): Low risk.
String or IDL/UUID changes made by this patch: None.
Attachment #8406328 - Flags: approval-mozilla-aurora?
(In reply to Nathan Froyd (:froydnj) from comment #27)
> Bug caused by (feature/regressing bug #): Many bugs.  bug 677653 and bug
> 682216 are the most recent.

Er.  Got my bug numbers scrambled.  That's supposed to be bug 682215.
Attachment #8406335 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Attachment #8406334 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8406328 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8406193 [details] [diff] [review]
refactor malloc-wrapping memory reporter implementations

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

::: extensions/spellcheck/hunspell/src/hunspell_alloc_hooks.h
@@ -94,5 @@
> -  char* result = moz_strdup(str);
> -  HunspellReportMemoryAllocation(result);
> -  return result;
> -}
> -#define strdup(str) hunspell_strdup(str)

I guess removing this is ok because hunspell seems to not actually use it, instead using mystrdup() throughout?

::: extensions/spellcheck/hunspell/src/mozHunspell.cpp
@@ +107,3 @@
>    static bool hasRun = false;
>    MOZ_ASSERT(!hasRun);
>    hasRun = true;

Is this needed? Does the check in HunspellAllocator not suffice? I can't see where that's instantiated, though.
(In reply to Nicholas Nethercote [:njn] from comment #29)
> Comment on attachment 8406193 [details] [diff] [review]
> refactor malloc-wrapping memory reporter implementations
> 
> Review of attachment 8406193 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: extensions/spellcheck/hunspell/src/hunspell_alloc_hooks.h
> @@ -94,5 @@
> > -  char* result = moz_strdup(str);
> > -  HunspellReportMemoryAllocation(result);
> > -  return result;
> > -}
> > -#define strdup(str) hunspell_strdup(str)
> 
> I guess removing this is ok because hunspell seems to not actually use it,
> instead using mystrdup() throughout?

Yes.

> ::: extensions/spellcheck/hunspell/src/mozHunspell.cpp
> @@ +107,3 @@
> >    static bool hasRun = false;
> >    MOZ_ASSERT(!hasRun);
> >    hasRun = true;
> 
> Is this needed? Does the check in HunspellAllocator not suffice? I can't see
> where that's instantiated, though.

Well.  HunspellAllocator is never actually instantiated.  And trying to making mozHunspell inherit from HunspellAllocator *and* have mozHunspell be visible from hunspell_alloc_hooks.h seemed potentially problematic.

Try, just to make sure: https://tbpl.mozilla.org/?tree=Try&rev=5cdf2aa87e1a
inbound: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&pusher=nfroyd@mozilla.com
Flags: in-testsuite-
Target Milestone: --- → mozilla31
Bah, got my inbound link scrambled.  How about:

https://hg.mozilla.org/integration/mozilla-inbound/rev/00b716b32cda
Looks like this is sticking on inbound, even if it's not on central yet.  Marking as c-n.
Keywords: checkin-needed
Attached patch B2G28 patchSplinter Review
Compiles locally --enable-optimize --enable-debug on Linux64.
Attachment #8406909 - Flags: review+
Attached patch B2G26 patchSplinter Review
Attachment #8406946 - Flags: review+
Whiteboard: [adv-main29+][adv-esr24.5+]
Marking [qa-] due to lack of test case. Feel free to provide one if you'd like verification, thanks.
Whiteboard: [adv-main29+][adv-esr24.5+] → [adv-main29+][adv-esr24.5+][qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.