Closed Bug 680840 (CVE-2011-3002) Opened 9 years ago Closed 9 years ago

GrowAtomTable() return code not checked

Categories

(Core :: Canvas: WebGL, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9
Tracking Status
firefox6 --- affected
firefox7 + fixed
firefox8 + fixed
firefox9 + fixed
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: dveditz, Assigned: bjacob)

References

Details

(Whiteboard: [sg:critical?][qa-])

Attachments

(3 files)

+++ 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
Duplicate of this bug: 680498
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.
(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.
@ 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?
I'd be more comfortable having you write the patch, but I'm willing to review.
OK, will do.
Assignee: nobody → bjacob
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.
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.
> 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.
Attachment #557840 - Flags: review? → review?(dveditz)
Ping. Didn't you want this in as soon as possible?
...sorry, I didn't realize that today is a holiday in the US.
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)
Attachment #557840 - Flags: review?(dveditz) → review+
Attachment #557840 - Flags: approval-mozilla-beta?
Attachment #557840 - Flags: approval-mozilla-aurora?
http://hg.mozilla.org/mozilla-central/rev/e5c2f6587160
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out because of linking issue on Windows, we need to link to $(MOZALLOC_LIB)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Carrying forward r+ from dveditz.

On tryserver:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=13b261cc9780
Attachment #559606 - Flags: review+
Attachment #559606 - Flags: approval-mozilla-beta?
Attachment #559606 - Flags: approval-mozilla-aurora?
Attachment #557840 - Flags: approval-mozilla-beta?
Attachment #557840 - Flags: approval-mozilla-aurora?
http://hg.mozilla.org/mozilla-central/rev/b35689d684f7
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Attached file 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.
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.
Attachment #559606 - Flags: approval-mozilla-beta?
Attachment #559606 - Flags: approval-mozilla-beta+
Attachment #559606 - Flags: approval-mozilla-aurora?
Attachment #559606 - Flags: approval-mozilla-aurora+
Target Milestone: --- → mozilla9
qa- as no QA fix verification needed
Whiteboard: [sg:critical?] → [sg:critical?][qa-]
Alias: CVE-2011-3002
Group: core-security
You need to log in before you can comment on or make changes to this bug.