The default bug view has changed. See this FAQ.
Bug 680840 (CVE-2011-3002)

GrowAtomTable() return code not checked

RESOLVED FIXED in Firefox 7

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: dveditz, Assigned: bjacob)

Tracking

unspecified
mozilla9
x86
Windows XP
Points:
---

Firefox Tracking Flags

(firefox6 affected, firefox7+ fixed, firefox8+ fixed, firefox9+ fixed, status1.9.2 unaffected, status1.9.1 unaffected)

Details

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

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
+++ 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
(Reporter)

Updated

6 years ago
status-firefox6: fixed → affected
status-firefox7: fixed → affected
status-firefox8: --- → affected
status-firefox9: --- → affected
tracking-firefox6: + → ---
tracking-firefox7: + → ---
Whiteboard: [sg:critical?]
(Reporter)

Updated

6 years ago
tracking-firefox7: --- → +
tracking-firefox8: --- → +
tracking-firefox9: --- → +
(Reporter)

Updated

6 years ago
Duplicate of this bug: 680498

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
@ 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?
(Reporter)

Comment 5

6 years ago
I'd be more comfortable having you write the patch, but I'm willing to review.
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
> 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.
Created attachment 557840 [details] [diff] [review]
use moz_xmalloc and moz_xrealloc there
Attachment #557840 - Flags: review?
(Assignee)

Updated

6 years ago
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.
(Reporter)

Comment 13

6 years ago
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+
(Assignee)

Updated

6 years ago
Attachment #557840 - Flags: approval-mozilla-beta?
Attachment #557840 - Flags: approval-mozilla-aurora?
http://hg.mozilla.org/mozilla-central/rev/e5c2f6587160
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Backed out because of linking issue on Windows, we need to link to $(MOZALLOC_LIB)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 559606 [details] [diff] [review]
Updated to actually link

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?
(Assignee)

Updated

6 years ago
Attachment #557840 - Flags: approval-mozilla-beta?
Attachment #557840 - Flags: approval-mozilla-aurora?
http://hg.mozilla.org/mozilla-central/rev/b35689d684f7
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Created attachment 559669 [details]
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.

Updated

6 years ago
Attachment #559606 - Flags: approval-mozilla-beta?
Attachment #559606 - Flags: approval-mozilla-beta+
Attachment #559606 - Flags: approval-mozilla-aurora?
Attachment #559606 - Flags: approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-beta/rev/c792a27dae06
http://hg.mozilla.org/releases/mozilla-aurora/rev/63d1f8b24911

Updated

6 years ago
status-firefox7: affected → fixed
status-firefox8: affected → fixed
status-firefox9: affected → fixed
Target Milestone: --- → mozilla9
qa- as no QA fix verification needed
Whiteboard: [sg:critical?] → [sg:critical?][qa-]
(Reporter)

Updated

6 years ago
Alias: CVE-2011-3002
(Reporter)

Updated

5 years ago
Group: core-security
You need to log in before you can comment on or make changes to this bug.