Closed
Bug 993546
Opened 10 years ago
Closed 10 years ago
memory reporter wrappers for realloc can use-after-free
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla31
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+
abillings
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
17.79 KB,
patch
|
froydnj
:
review+
abillings
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.75 KB,
patch
|
froydnj
:
review+
abillings
:
approval-mozilla-esr24+
|
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 | ||
Updated•10 years ago
|
Assignee: nobody → nfroyd
Comment 1•10 years ago
|
||
realloc, how I loathe thee.
Comment 2•10 years ago
|
||
> 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.
Assignee | ||
Comment 3•10 years ago
|
||
(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. :(
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8403978 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8403979 -
Flags: review?(n.nethercote)
Comment 7•10 years ago
|
||
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?
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
Also realized that http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/hunspell/src/hunspell_alloc_hooks.h#78 has the same issue...
Comment 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
(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)
Comment 14•10 years ago
|
||
> 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)
Comment 15•10 years ago
|
||
> 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)
Assignee | ||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•10 years ago
|
||
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+
Updated•10 years ago
|
status-firefox28:
--- → wontfix
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox-esr24:
--- → affected
tracking-firefox29:
--- → +
tracking-firefox30:
--- → +
tracking-firefox31:
--- → +
tracking-firefox-esr24:
--- → +
Comment 18•10 years ago
|
||
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+
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8406317 -
Flags: review+
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8406319 -
Flags: review+
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
Ideally getting the author for c-n correct this time.
Attachment #8406320 -
Attachment is obsolete: true
Attachment #8406328 -
Flags: review+
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8406319 -
Attachment is obsolete: true
Attachment #8406334 -
Flags: review+
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8406317 -
Attachment is obsolete: true
Attachment #8406335 -
Flags: review+
Assignee | ||
Comment 25•10 years ago
|
||
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?
Assignee | ||
Comment 26•10 years ago
|
||
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?
Assignee | ||
Comment 27•10 years ago
|
||
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?
Assignee | ||
Comment 28•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8406335 -
Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Updated•10 years ago
|
Attachment #8406334 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•10 years ago
|
Attachment #8406328 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 29•10 years ago
|
||
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.
Assignee | ||
Comment 30•10 years ago
|
||
(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
Assignee | ||
Comment 31•10 years ago
|
||
Bah, got my inbound link scrambled. How about: https://hg.mozilla.org/integration/mozilla-inbound/rev/00b716b32cda
Assignee | ||
Comment 32•10 years ago
|
||
Looks like this is sticking on inbound, even if it's not on central yet. Marking as c-n.
Keywords: checkin-needed
Assignee | ||
Comment 33•10 years ago
|
||
Compiles locally --enable-optimize --enable-debug on Linux64.
Attachment #8406909 -
Flags: review+
Comment 34•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/00b716b32cda
Status: NEW → RESOLVED
Closed: 10 years ago
status-b2g18:
--- → wontfix
status-b2g-v1.1hd:
--- → wontfix
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8406946 -
Flags: review+
Comment 36•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/58eb097f3554 https://hg.mozilla.org/releases/mozilla-beta/rev/c8f1a4f5ca4d https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/613e8309cfee https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/eeebb0257b9f https://hg.mozilla.org/releases/mozilla-esr24/rev/28b5a481fd1c
Updated•10 years ago
|
status-b2g-v1.3T:
--- → fixed
Updated•10 years ago
|
Whiteboard: [adv-main29+][adv-esr24.5+]
Comment 37•10 years ago
|
||
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-]
Updated•9 years ago
|
Group: core-security
Updated•7 years ago
|
Keywords: csectype-uaf
You need to log in
before you can comment on or make changes to this bug.
Description
•