680840 - (CVE-2011-3002) GrowAtomTable() return code not checked

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Daniel Veditz [:dveditz]]

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

We fixed the code issue from the first paragraph of bug 665934 comment 1 but did NOT fix "the real issue" from the second paragraph of that comment (as well as the bug summary) -- the return value of GrowAtomTable() is still not checked. If malloc/realloc fails (they can!) then later code does stupid unsafe things.

First, GrowAtomTable() returns 0 for "good", -1 for "bad". Other int-returning functions (the various Init functions, for example) use 0 for bad and 1 for good. If it were checked this inconsistency is going to trip someone up.

  -> make GrowAtomTable() return 0 for failure, 1 for success.
  
  [InitPreProcessor() in cppstruct.c converts the InitAtomTable() return
  value into the opposite sense: 0 for success. Every other function
  in cppstruct.c returns 1 for success and 0 for failure]

InitAtomTable() checks the allocation of atable->amap instead of the GrowAtomTable return code, but it's possible that the amap allocation succeeded and the arev allocation failed. It also may be possible that there's already an allocated table when IncreaseHashTableSize() calls InitAtomTable, and if the realloc fails then the original amap would be in place.

634     GrowAtomTable(atable, INIT_ATOM_TABLE_SIZE);
635     if (!atable->amap)
636         return 0;

rewrite as 
        if (!GrowAtomTable(atable, INIT_ATOM_TABLE_SIZE))
            return 0;

       NB: I'm assuming you've swapped the return code as mentioned above

If GrowAtomTable() fails inside AllocateAtom() then it'll start overwriting past the end of the allocated amap and arev buffers. Not sure what you should do if it does fail, maybe return 0 as the atom number? The problem is that all the callers of LookUpAddString (the only caller of AllocateAtom) simply assume success.

Is there anything keeping a lid on nextFree or do we need to worry about integer overflow in GrowAtomTable(atable, atable->nextFree*2) ? There's further multiplication by sizeof(int) inside GrowAtomTable so we'd need a bit more than 500 Million atoms to overflow.

Similar heap overflow and maybe integer overflow potential in AddAtomFixed

Comment 2

[daniel-bzmz]

Note that the ANGLE preprocessor is being currently being completely re-written and thus we'd rather not spend effort patching up the old one unless absolutely necessary.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to daniel-bzmz from comment #2)
> Note that the ANGLE preprocessor is being currently being completely
> re-written and thus we'd rather not spend effort patching up the old one
> unless absolutely necessary.

Well, either there's a security issue here, or there isn't. If there is, then it is necessary to fix it, at least as a local patch on our side.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

@ Daniel Veditz: should I be working on a patch? You seem to have given this a great amount of thought already so I was wondering if you just wanted to patch it yourself?

Comment 5

[Daniel Veditz [:dveditz]]

I'd be more comfortable having you write the patch, but I'm willing to review.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

OK, will do.

Comment 7

[Johnny Stenback (:jst)]

Benoit, can you get a patch written for this in the next few days? We'd hate to release 7 w/o a fix for this, especially given that this is a followup fix for another sg:critical bug, and this issue was effectively reported by the same external reporter.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Here's this GrowAtomTable function:

static int GrowAtomTable(AtomTable *atable, int size)
{
    int *newmap, *newrev;

    if (atable->size < size) {
        if (atable->amap) {
            newmap = realloc(atable->amap, sizeof(int)*size);
            newrev = realloc(atable->arev, sizeof(int)*size);
        } else {
            newmap = malloc(sizeof(int)*size);
            newrev = malloc(sizeof(int)*size);
            atable->size = 0;
        }
        if (!newmap || !newrev) {
            /* failed to grow -- error */
            if (newmap)
                atable->amap = newmap;
            if (newrev)
                atable->arev = newrev;
            return -1;
        }
        memset(&newmap[atable->size], 0, (size - atable->size) * sizeof(int));
        memset(&newrev[atable->size], 0, (size - atable->size) * sizeof(int));
        atable->amap = newmap;
        atable->arev = newrev;
        atable->size = size;
    }
    return 0;
}


Memory is allocated by malloc() which indeed is still fallible. Since, as Daniel-from-Transgaming said, this whole code is going away soon, and the OOM condition here is hard to reproduce in practice, I would like to just use infallible memory allocation here.

And to prevent other such ANGLE bugs in the near future (until the new preprocessor lands) I would also like to land a patch to the Makefile replacing malloc by moz_xmalloc everywhere in ANGLE.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

> And to prevent other such ANGLE bugs in the near future (until the new
> preprocessor lands) I would also like to land a patch to the Makefile
> replacing malloc by moz_xmalloc everywhere in ANGLE.

Scrap that: the file where GrowAtomTable is defined has

#undef malloc
#undef realloc
#undef free

so it would not benefit from such a DEFINE-based solution. This shows that we have to patch each place that needs infallible malloc.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: use moz_xmalloc and moz_xrealloc there

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Ping. Didn't you want this in as soon as possible?

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

...sorry, I didn't realize that today is a holiday in the US.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 557840 [details] [diff] [review]
use moz_xmalloc and moz_xrealloc there

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

::: gfx/angle/src/compiler/preprocessor/atom.c
@@ +336,5 @@
>              /* failed to grow -- error */
>              if (newmap)
>                  atable->amap = newmap;
>              if (newrev)
>                  atable->arev = newrev;

Since moz_x versions are the infallible ones you don't need to check newmap and newrev... maybe assert() them if you don't trust moz_xmalloc? This whole chunk can now go away and GrowAtomTable() either succeeds or crashes. Icky choice, but
if this code is all going away then I guess this is all we need.

r=dveditz
 (prefer you remove the "if(!newmap || !newrev)" deadcode block but won't
  insist on it, especially for Firefox 7)

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/e5c2f6587160

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Backed out because of linking issue on Windows, we need to link to $(MOZALLOC_LIB)

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Updated to actually link

Carrying forward r+ from dveditz.

On tryserver:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=13b261cc9780

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/b35689d684f7

Comment 18

[Justin Wood (:Callek)]

Attachment: leak...

Hrm, I got leaks on M(o) for linux-debug on m-i when I merged this to there. [https://tbpl.mozilla.org/php/getParsedLog.php?id=6361520]

I suspect the leak is due to this based on what leaked (the following build on m-i didn't leak, so probably a new intermittent)

The short:
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1981359 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of AsyncStatement with size 48 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1744 instances of AtomImpl with size 20 bytes each (34880 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 24 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BasicCanvasLayer with size 324 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of BasicContainerLayer with size 340 bytes each (680 bytes total)

I am attaching the FULL leak log. If this is some other leak bug that I can't find the details for, I'm sorry for noise.

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Apparently this is a nsString leak?

nsStringStats
 => mAllocCount:        5116074
 => mReallocCount:       237165
 => mFreeCount:         5105690  --  LEAKED 10384 !!!
 => mShareCount:        7985848
 => mAdoptCount:         269146
 => mAdoptFreeCount:     269144  --  LEAKED 2 !!!

The code here does not use or create any nsString. It is purely in ANGLE which doesn't know about nsString. Moreover the patch is just replacing a malloc by a moz_xmalloc and a realloc by a moz_xrealloc. Since moreover this is an intermittent, I would suggest that this probably has nothing to do with this patch and it's just out of "luck" that this push was the first to get this intermittent orange.

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/releases/mozilla-beta/rev/c792a27dae06
http://hg.mozilla.org/releases/mozilla-aurora/rev/63d1f8b24911

Comment 21

[u279076]

qa- as no QA fix verification needed