1030667 - (CVE-2015-0828) AddressSanitizer: double-free with zero-length XHR, depending on behavior of realloc(p, 0)

Status: RESOLVED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: log

+++ This bug was initially created as a clone of Bug #1023758 +++

+++ This bug was initially created as a clone of Bug #1021612 +++

(The ASan fun never seems to stop, does it? ;-) )

I tested Steven's asan build ( http://people.mozilla.org/~stmichaud/bmo/firefox-asan.dmg ) from Jun 23 (bug 1023758 comment 42) and when I pressed "Earth" in a Google Maps page, the asan build crashed while attempting double-free on 0x6020000a1e90 with mozilla::ArrayBufferBuilder::setCapacity on the stack.

I've set dom.cycle_collector.incremental to false to bypass bug 1023758 for the moment.

Setting s-s and sec-critical until we know what is going on. Setting needinfo? from :Waldo who last worked on lots of ArrayBuffer stuff.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

My CLI arguments were something like:

ASAN_SYMBOLIZER_PATH=/Applications/Nightly.app/Contents/MacOS/llvm-symbolizer /Applications/Nightly.app/Contents/MacOS/firefox-bin 2>&1 | tee ~/Downloads/asanLog1.txt

Comment 2

[Steven Michaud [:smichaud] (Retired)]

I get exactly the same crash visiting http://maps.google.com and clicking on the Satellite button in the lower left (or clicking on that button and clicking again on the Map button, in the same location).  This only happens with the "new Google Maps".  The "old" one has the Satellite/Map button in the upper right.

So this bug is 100% reproducible.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> So this bug is 100% reproducible.

Though only in ASan builds :-(

Unfortunately m-c nightlies don't crash.  So we can't use them to get a regression range.

We have official Linux ASan builds going back a ways:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/

I wonder if these also crash.

Comment 4

[Andrew McCreight [:mccr8]]

Sounds like bug 988675.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> Sounds like bug 988675.

Could you CC me on that bug, Andrew?  I don't have permission to view it.  (And if I don't, I expect most others here don't, either.)

Or maybe the bug number is wrong.

Comment 6

[Robert Kaiser]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> Sounds like bug 988675.

That bug can't be important, I guess. I deem all bugs as unimportant that I don't have access to despite being in s-g.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Robert Kaiser (:[e-mail]) from comment #6)
> That bug can't be important, I guess. I deem all bugs as unimportant that I
> don't have access to despite being in s-g.

That bug has Security-Sensitive DOM Bug and Security-Sensitive Core Bug checked because it is a sec-critical bug, so you need both permissions.  Talk to Dan if you think you should have more access.  Sorry about the problems, I've cced both of you now.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Minimal testcase, may require GC/closing a window/etc. to hit asan failure

The issue here occurs when you do an XHR of a zero-length file and extract the response as an ArrayBuffer.

I may simply not have intuitive grasp of the state machine calls here, but just reading the code, I don't see what exactly would be going wrong here.  I almost wonder if the leak sanitizer stuff might be mishandling/misreporting realloc of a null pointer.

Anyway.  Pretty sure this isn't a JS issue, rather an issue with XHR's JSAPI use.  Or with the sanitizer stuff.  Maybe someone from that area with better XHR code understanding can take this now?

Comment 9

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

What's the ASAN log from that test case?

Comment 10

[Jesse Ruderman]

Attachment: ASan log for reduced testcase

Comment 11

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

Does JS_ReallocateArrayBufferContents free the 3rd param if allocating larger buffer fails?

Or does ASAN think js_free(nullptr); js_free(nullptr); is double free?
(I think Bug 1031920 leads to that)

Comment 12

[Jeff Walden [:Waldo]]

js_free straightforwardly calls free, so we'd just be talking the behavior of free(nullptr); free(nullptr).  Smacking this in main in nsBrowserApp.cpp suggests the answer is no:

  void* p = malloc(42);
  if (!p)
    return 1;
  free(p);
  p = nullptr;
  free(p);
  free(p);

Comment 13

[Jeff Walden [:Waldo]]

(In reply to Olli Pettay [:smaug] from comment #11)
> Does JS_ReallocateArrayBufferContents free the 3rd param if allocating
> larger buffer fails?

No.  It just calls js_realloc, or cx->reallocCanGC which calls that, which itself just calls realloc.  See AllocateArrayBufferContents, js/src/vm/ArrayBufferObject.cpp.  So only if ASAN realloc were to mishandle this would there be a problem.  And, modifying the code to this:

  void* p = malloc(42);
  if (!p)
    return 1;
  void* newp = realloc(p, 1ULL << 53);
  MOZ_ASSERT(!newp);
  free(p);
  p = nullptr;
  free(p);
  free(p);

and running with ASAN_OPTIONS=abort_on_error=1:allocator_may_return_null=1 I don't get any ASAN failures.

Comment 14

[Jeff Walden [:Waldo]]

Oho!

  void* p = nullptr;
  p = realloc(p, 0);
  if (!p)
    return 1;
  void* newp = realloc(p, 0);
  if (!newp)
  {
    free(p);
    return 1;
  }

With that, the first realloc returns a non-null pointer.  The *second* realloc returns nullptr.  This would be okay if |p| were left alone.  But this apparently *also*, with the ASAN replacement, frees |p|!  So we enter the |!newp| block, and the |free(p)| triggers ASAN complaints about double-free.  I don't believe that's okay.  Either realloc returns a non-null pointer, possibly deallocating the one passed in (*always* doing so with the special ASAN semantics), or it returns nullptr and leaves the incoming memory alone.

Perhaps someone reimplemented realloc based on something like http://linux.die.net/man/3/realloc which claims realloc(..., 0) will free the pointer.  But that flatly contradicts C99's *actual* specification for realloc, which says only "If memory for the new object cannot be allocated, the old object is not deallocated and its value is unchanged" and "The realloc function returns a pointer to the new object (which may have the same value as a pointer to the old object), or a null pointer if the new object could not be allocated."  If we observe a null pointer returned, then the new object could not be allocated, and therefore "the old object is not deallocated and its value is unchanged".

I'm not 100% sure this is functionally identical to the series of calls being performed in our code.  But this behavior looks like a clear ASAN bug to me in any event.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Possibly relevant:

https://codereview.appspot.com/10374044

(https://code.google.com/p/sawbuck/)

What do you think of this patch, Jeff?  (In other words, do you know LLVM/ASan code well enough to say whether or not this would work as a fix?)

The reason I ask is that I am, for the time being, maintaining a set of patches to an old version of LLVM/ASan as part of an effort to provide semi-official Mac ASan Firefox builds, here:

http://people.mozilla.org/~stmichaud/bmo/firefox-asan.dmg
http://people.mozilla.org/~stmichaud/bmo/firefox-asan-howto.txt

I wonder if I should add this patch (or something like it) to my set of patches.

Comment 17

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

inferno may have an opinion on comment 15.

Comment 19

[Jeff Walden [:Waldo]]

(In reply to Steven Michaud from comment #15)
> Possibly relevant:
> 
> https://codereview.appspot.com/10374044
> 
> (https://code.google.com/p/sawbuck/)
> 
> What do you think of this patch, Jeff?  (In other words, do you know
> LLVM/ASan code well enough to say whether or not this would work as a fix?)

I don't know LLVM/ASan code well at all.  But the comments and structure of that patch's changes seem pretty clearly to address the problems here.  So it looks like the correct cherrypick.

If that patch dates to a year ago, though, why wouldn't my all-but-tip clang have it?  Please explain to me what I don't know about ASan's integration into llvm and source builds, and why my build wouldn't have it.  :-)

Comment 20

[Andrew McCreight [:mccr8]]

That repo is for some weirdo Google version of ASan for Windows, not the actual ASan we're running anywhere.

Comment 21

[Andrew McCreight [:mccr8]]

At least, that's what I assume from the syzygy in the path.

Comment 22

[Jeff Walden [:Waldo]]

Bug 1019385 comment 8, me commenting on that being marked a dup of this:

> I'm not sure I agree with the dup here.  That bug's issue is a problem in
> ASan, only.  This is some sort of issue definitely not with ASan, possibly
> with the MSVC CRT.  It could perhaps be mishandling |void* p =
> realloc(nullptr, 0); void* p2 = realloc(p, 0); free(p2);| as well.
> 
> And in fact, if I consult the source of the 2010 runtime, it looks like
> realloc(nullptr, 0) will allocate memory (_heap_alloc converts size to 1 if
> size is zero), and realloc(thatPointer, 0) will free the memory and return
> nullptr.  It seems even likelier, now (particularly given a "Special ANSI
> Requirements" comment in realloc.c), that C89 realloc specified this
> behavior, and C99 decided it was a bug and fixed it.
> 
> All of which suggests, I think, that we probably need to apply extra care to
> realloc calls that might pass 0 as size.  At least, unless we want to
> totally give up on supporting non-jemalloc heaps.
> 
> And, um.  I just consulted memory/jemalloc/src/src/jemalloc.c.  And it has
> this in it, for the function that presumably becomes realloc:
> 
> void *
> je_realloc(void *ptr, size_t size)
> {
> 	void *ret;
> 	size_t usize JEMALLOC_CC_SILENCE_INIT(0);
> 	size_t old_usize = 0;
> 	UNUSED size_t old_rzsize JEMALLOC_CC_SILENCE_INIT(0);
> 
> 	if (size == 0) {
> 		if (ptr != NULL) {
> 			/* realloc(ptr, 0) is equivalent to free(ptr). */
> 			UTRACE(ptr, 0, 0);
> 			ifree(ptr);
> 			return (NULL);
> 		}
> 		size = 1;
> 	}
> 	...
> }
> 
> We should fix this to be C99-compliant, for safety.  But if this is really
> realloc, it seems like (*consults glibc on his system* *finds |2925 if
> (bytes == 0 && oldmem != NULL) { __libc_free(oldmem); return 0; }| in it*)
> everyone implements C89 (by hypothesis) realloc semantics.  So we should at
> least *try* to audit all of our reallocs, to be sure that a zero-sized
> malloc/realloc can't flow into a realloc(..., 0) that we haven't written to
> be careful about his issue.  :-(
> 
> So, um.  I guess I agree with the dup here.  But there is a real issue to be
> dealt with in bug 1030667, that we can't just "solve" with an ASan patch.

So I no longer think an ASan patch is enough to decide we've "fixed" this.  :-(

Comment 23

[Jeff Walden [:Waldo]]

Yeah, http://port70.net/~nsz/c/c89/c89-draft.html#4.10.3.4 claims realloc in C89 had the bad semantics.  :-(  Not that it truly matters, except for spec-wonking, I guess.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #15)

For what it's worth, I've now "fixed" this problem in my local copy of LLVM 185949, and I've used it to do another Mac ASan build (updated to yesterday's trunk):

http://people.mozilla.org/~stmichaud/bmo/firefox-asan.dmg
http://people.mozilla.org/~stmichaud/bmo/firefox-asan-howto.txt

The "original" LLVM doesn't have code directly corresponding to code that was patched in "Sawbuck", so I had to write my own patch.  You can see it in my Howto referenced above.

I'm not entirely sure I *should have* "fixed" this problem.  But at least my builds will now stop it from blocking the finding of other bugs.

Jeff notes in bug 1019385 comment #8 that jemalloc's realloc is currently non-C99-compliant.  So (as best I can tell by looking at the assembly code) is Apple's realloc implementation (in /usr/lib/system/libsystem_c.dylib).

Comment 25

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Steven Michaud from comment #24)
> For what it's worth, I've now "fixed" this problem in my local copy of LLVM
> 185949, and I've used it to do another Mac ASan build (updated to
> yesterday's trunk):

It would be nice to have a build from at least today morning's checkout because that should have the fix for bug 1023758 (plus I can turn on ICC again).

Comment 26

[Mike Hommey [:glandium]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22)
> Bug 1019385 comment 8, me commenting on that being marked a dup of this:
> 
> > I'm not sure I agree with the dup here.  That bug's issue is a problem in
> > ASan, only.  This is some sort of issue definitely not with ASan, possibly
> > with the MSVC CRT.  It could perhaps be mishandling |void* p =
> > realloc(nullptr, 0); void* p2 = realloc(p, 0); free(p2);| as well.
> > 
> > And in fact, if I consult the source of the 2010 runtime, it looks like
> > realloc(nullptr, 0) will allocate memory (_heap_alloc converts size to 1 if
> > size is zero), and realloc(thatPointer, 0) will free the memory and return
> > nullptr.  It seems even likelier, now (particularly given a "Special ANSI
> > Requirements" comment in realloc.c), that C89 realloc specified this
> > behavior, and C99 decided it was a bug and fixed it.
> > 
> > All of which suggests, I think, that we probably need to apply extra care to
> > realloc calls that might pass 0 as size.  At least, unless we want to
> > totally give up on supporting non-jemalloc heaps.
> > 
> > And, um.  I just consulted memory/jemalloc/src/src/jemalloc.c.  And it has
> > this in it, for the function that presumably becomes realloc:
> > 
> > void *
> > je_realloc(void *ptr, size_t size)
> > {
> > 	void *ret;
> > 	size_t usize JEMALLOC_CC_SILENCE_INIT(0);
> > 	size_t old_usize = 0;
> > 	UNUSED size_t old_rzsize JEMALLOC_CC_SILENCE_INIT(0);
> > 
> > 	if (size == 0) {
> > 		if (ptr != NULL) {
> > 			/* realloc(ptr, 0) is equivalent to free(ptr). */
> > 			UTRACE(ptr, 0, 0);
> > 			ifree(ptr);
> > 			return (NULL);
> > 		}
> > 		size = 1;
> > 	}
> > 	...
> > }

Note the jemalloc we *currently* use is the one in memory/mozjemalloc, and that one normalizes size == 0 to size = 1, although it has an optional support for the free() semantics. So there was a semantics change in jemalloc at some point.

(looking in jemalloc git blame)

That was changed in https://github.com/jemalloc/jemalloc/commit/f081b88dfbce94c3c7c8faf0b0f91b117fbdfcc6

Because of this thread:
http://www.canonware.com/pipermail/jemalloc-discuss/2012-February/000107.html

Based on:
http://pubs.opengroup.org/onlinepubs/000095399/functions/realloc.html

According to http://stackoverflow.com/a/16760080 , C11 specifies realloc(p, 0) as implementation defined with either behavior.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #25)

Oops, forgot about the patches for bug 1023758.  I'll do another build tomorrow, updated to tomorrow's trunk.

Comment 28

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

Ugh, we really don't want that behavior.  Can we convince Jason to change it back?  Or make it configurable?

Comment 29

[Mike Hommey [:glandium]]

I'm not convinced one behavior is better than the other, because both exist in the wild. And the fact is we need to support both, whether jemalloc consistently uses one, because not all builds are using jemalloc.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #27)

> I'll do another build tomorrow, updated to tomorrow's trunk.

Done:

http://people.mozilla.org/~stmichaud/bmo/firefox-asan.dmg
http://people.mozilla.org/~stmichaud/bmo/firefox-asan-howto.txt

Comment 31

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Steven Michaud from comment #30)
> http://people.mozilla.org/~stmichaud/bmo/firefox-asan.dmg
> http://people.mozilla.org/~stmichaud/bmo/firefox-asan-howto.txt

Thanks! I'm now changing to this build.

Comment 32

[Abhishek Arya]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #17)
> inferno may have an opinion on comment 15.

This looks like an ASAN bug to me (and apparently SyzyASAN (Windows) already fixed it). Can you please file it at https://code.google.com/p/address-sanitizer/issues/list. Kcc@ (Kostya) is pretty fast at responding to these.

Comment 33

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

If this is an ASan bug, this is unlikely to be sec-critical. Shall we open this up?

Comment 34

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Abhishek Arya from comment #32)
> This looks like an ASAN bug to me (and apparently SyzyASAN (Windows) already
> fixed it). Can you please file it at
> https://code.google.com/p/address-sanitizer/issues/list. Kcc@ (Kostya) is
> pretty fast at responding to these.

I filed https://code.google.com/p/address-sanitizer/issues/detail?id=323 .

Comment 35

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

No, this is not just an ASAN bug.

Comment 36

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

I filed bug 1035001 with a patch to our wrappers around realloc that makes realloc(nullptr, 0); always return nullptr.

Comment 37

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> > Can you please file it at
> > https://code.google.com/p/address-sanitizer/issues/list. Kcc@ (Kostya) is
> > pretty fast at responding to these.
> 
> I filed https://code.google.com/p/address-sanitizer/issues/detail?id=323 .

Kostya is requesting access to this bug, but (1) I don't know his bugmail - I tried to get his email but the Captcha challenge just wouldn't go through and (2) I'm not the best person to decide if he should be here.

He is also not yet convinced it's an ASan issue, see https://code.google.com/p/address-sanitizer/issues/detail?id=323#c2

Comment 38

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Abhishek: There is no way I can set that issue in Google Code to be restricted, can you please find a way to restrict it?

Comment 39

[Abhishek Arya]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #38)
> Abhishek: There is no way I can set that issue in Google Code to be
> restricted, can you please find a way to restrict it?

I don't have edit privileges to the ASAN issue tracker, but asked Kostya to do it.

Comment 40

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #36)
> I filed bug 1035001 with a patch to our wrappers around realloc that makes
> realloc(nullptr, 0); always return nullptr.

Kyle will bug 1035001 fix the gecko side security issue this bug represents?

Comment 41

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

(In reply to David Bolter [:davidb] from comment #40)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #36)
> > I filed bug 1035001 with a patch to our wrappers around realloc that makes
> > realloc(nullptr, 0); always return nullptr.
> 
> Kyle will bug 1035001 fix the gecko side security issue this bug represents?

I believe so ... although there may be other stuff lurking here.  Fixing bug 1035001 is non-trivial though.

Comment 43

[Andrew McCreight [:mccr8]]

This was forward duped from bug 988675, which is the original report of this issue.

Comment 44

[(no longer active)]

Abhishek, can you please CC me on https://code.google.com/p/address-sanitizer/issues/detail?id=323 ?  Thanks!

Comment 45

[Jeff Walden [:Waldo]]

I'd also like a CC on that, if possible.  I had it starred back when it was open, but I guess that doesn't carry over to being able to see it post-closure, perhaps not surprisingly.

Comment 46

[Abhishek Arya]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #44)
> Abhishek, can you please CC me on
> https://code.google.com/p/address-sanitizer/issues/detail?id=323 ?  Thanks!

Ok asked Kostya to add both of you. I don't have edit permission to addressanitizer repo.

Comment 47

[Andrew McCreight [:mccr8]]

I'm going to mark this sec-audit for now, because the test case hits a use-after-free because ASan's realloc behavior differs from what Firefox uses.  I'm going to look into places that pass in 0 for the second argument to realloc, and what they might expect.

Comment 48

[Andrew McCreight [:mccr8]]

I did a try run with a modified version of realloc that crashes when you pass in a size of 0.  This found three issues.

1) bug 1055322 - libnestegg expects the freeing behavior, so we leak with our jemalloc.  I'm working around this by hard coding the freeing-realloc behavior.

2) ArrayBufferBuilder::setCapacity. This is the thing found in the various dupes, though this code has changed quite a bit recently.  If aNewCap is 0, then with a freeing realloc mDataPtr becomes a dangling pointer, and we return.  This can only happen in nsXMLHttpRequest::OnStopRequest, as the other places guard against 0.  In that case, it sets a failure flag and returns.

3) In Machine::Code::Code (in graphite2), it can realloc to 0.  Here it does:
  _data = static_cast<byte *>(realloc(_data, _data_size*sizeof(byte)));
I think this is safe, because with a freeing realloc behavior, we'll just end up with _data set to null.

Personally, I'm in favor of fixing these places, then landing a patch to change our jemalloc to crash when 0 is passed in for the size.  This will let us find any other places in our code that have potentially dangerous behavior depending on which realloc you use.

Comment 50

[Abhishek Arya]

[e-mail] and [e-mail], you would need google accounts for me to cc you on https://code.google.com/p/address-sanitizer/issues/detail?id=323

Comment 51

[Daniel Veditz [:dveditz]]

We are, in fact, considering switching to jemalloc3 so I'm raising the severity back to sec-high. Not an immanent danger, but it's coming.

Comment 52

[Andrew McCreight [:mccr8]]

Attachment: Don't pass 0 to realloc in ArrayBufferBuilder::setCapacity.

Sorry this took so long for me to get to.

I did a green try L64 debug run a week or two ago.

Comment 53

[Andrew McCreight [:mccr8]]

Comment on attachment 8520691 [details] [diff] [review]
Don't pass 0 to realloc in ArrayBufferBuilder::setCapacity.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Probably not too difficult, but this only affects people who use an allocator that is different than the jemalloc we ship with Firefox.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes, the comment points out the problem, though I don't know how much more obvious it is than just the patch.

Which older supported branches are affected by this flaw? Everything, but it only affects people with non-standard builds, so I'm not sure where we should backport it.

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

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

How likely is this patch to cause regressions; how much testing does it need? very safe patch.  With jemalloc, it should not affect behavior at all.

Comment 54

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

Comment on attachment 8520691 [details] [diff] [review]
Don't pass 0 to realloc in ArrayBufferBuilder::setCapacity.

sec-approval+ for trunk. I'm not sure if we want to backport this. I've minused it for ESR31 though.

Comment 55

[Andrew McCreight [:mccr8]]

try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0490f86ffc9c

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af98d4bb0061

Comment 57

[Andrew McCreight [:mccr8]]

I think there's no urgent need to backport this without any evidence somebody is using another allocator.

Comment 58

[Andrew McCreight [:mccr8]]

I think it is even less likely that anybody is shipping B2G with a weird allocator.

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/af98d4bb0061