Last Comment Bug 665934 - (CVE-2011-2987) GrowAtomTable() return code not checked, crash due to ANGLE
(CVE-2011-2987)
: GrowAtomTable() return code not checked, crash due to ANGLE
Status: RESOLVED FIXED
[sg:critical?][qa-]
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on: CVE-2011-3002
Blocks: 672761
  Show dependency treegraph
 
Reported: 2011-06-21 09:47 PDT by Daniel Veditz [:dveditz]
Modified: 2014-10-01 16:05 PDT (History)
13 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
fixed
+
fixed
wontfix
unaffected
unaffected


Attachments
PoC. load index.html, wait, in a new tab open fuzzshader.html (44.84 KB, application/java-archive)
2011-06-21 09:51 PDT, Daniel Veditz [:dveditz]
no flags Details
fix bug: arev was meant instead of amap (967 bytes, patch)
2011-06-23 08:37 PDT, Benoit Jacob [:bjacob] (mostly away)
jacob.benoit.1: review+
jpr: approval‑mozilla‑aurora+
Details | Diff | Review

Description Daniel Veditz [:dveditz] 2011-06-21 09:47:06 PDT
Michael Jordon of Context IS sent this bug to us, based on testing on a WinXP SP3 machine and Firefox 4.0.1 (I believe, based on the other bug). He writes:
Comment 1 Daniel Veditz [:dveditz] 2011-06-21 09:47:44 PDT
(comment 0 continued...):

This is really a bug in the ANGLE library, but as it isn't clear who you report security issues to with that so I am giving it to you as you include the vulnerable code. The file gfx/angle/src/compiler/preprocessor/atom.c has a couple of issues which could be used to cause a heap overflow. The function GrowAtomTable on line 320 has an incorrect assignment on line 338 (I am sure it should be atable->arev = newrev) exploiting that one would be difficult, although if you could you would alias atom allocations and also overflow the arev buffer (because that is never changed). 

Of course the real issue with this is the places which call GrowAtomTable don't check the return code (except in init and only then to check if the amap pointer is NULL). Therefore if you can get the allocations to fail then any new atoms added to the hash will start to overwrite the end of the arrays causing a heap overflow, the values are somewhat under the attacks control because it depends on the current atom number. 

The poc contains three files, index.html which hosts two copies of eatmemory.html. Run this first, this just allocates alot of memory (on my XP SP3 machine, around 1.3GB). When the page shows the "Wait" alert open a second tab and load the fuzzshader.html file. This builds a shader with alot of #define pre-processor lines, I then get the crash in the pre-processor code as shown in the including stack trace and crash dump. I am not sure if it is exactly where I would expect it to be, but it is worth pointing out that just running fuzzshader.html does not crash, therefore it seems almost likely it is exploiting the allocation issue. And based on analysis of the code I believe it is an problem which needs fixing.
Comment 2 Daniel Veditz [:dveditz] 2011-06-21 09:51:31 PDT
Created attachment 540781 [details]
PoC. load index.html, wait, in a new tab open fuzzshader.html

Testcase sounds somewhat dependent on how much memory your machine has. Might have to play with eatmemory.html or limit a VM's available memory.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2011-06-21 10:13:20 PDT
Forwarded to ANGLE:
http://code.google.com/p/angleproject/issues/detail?id=173
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2011-06-21 10:26:10 PDT
Seems that on linux, |cgroup| could be used to limit available memory to reproduce the crash.
http://jlebar.com/2011/6/15/Limiting_the_amount_of_RAM_a_program_can_use.html
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2011-06-23 08:37:25 PDT
Created attachment 541388 [details] [diff] [review]
fix bug: arev was meant instead of amap

This is from the original report: "I am sure it should be atable->arev = newrev"  (by Michael Jordon). Looking at the surrounding code and the explanatory comment in struct AtomTable_Rec, that makes sense.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2011-06-23 08:50:36 PDT
Notice that the bug here only happens when malloc() fails to allocate memory. This page:
https://developer.mozilla.org/en/Infallible_memory_allocation

Says that malloc() "will become infallible in the relatively near future". Has this already happened in mozilla-central?
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-06-23 08:53:15 PDT
(There also is a path with realloc(), don't know its status either).
Comment 9 Jeff Muizelaar [:jrmuizel] 2011-06-23 09:20:53 PDT
(In reply to comment #7)
> Notice that the bug here only happens when malloc() fails to allocate
> memory. This page:
> https://developer.mozilla.org/en/Infallible_memory_allocation
> 
> Says that malloc() "will become infallible in the relatively near future".
> Has this already happened in mozilla-central?

I believe so yes.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-23 10:49:30 PDT
(In reply to comment #7)
> Notice that the bug here only happens when malloc() fails to allocate
> memory. This page:
> https://developer.mozilla.org/en/Infallible_memory_allocation
> 
> Says that malloc() "will become infallible in the relatively near future".
> Has this already happened in mozilla-central?

Not yet: malloc() is still fallible.
Comment 11 daniel-bzmz 2011-06-23 11:19:40 PDT
Comment on attachment 541388 [details] [diff] [review]
fix bug: arev was meant instead of amap

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

Seems like the right thing to do.  Go ahead and commit to ANGLE repository if you need to.   We'll continue to investigate other fixes related to GrowAtomTable return values not being checked.
Comment 12 daniel-bzmz 2011-06-24 07:39:38 PDT
Above patch was committed to ANGLE repo as r699
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2011-06-27 10:17:53 PDT
Comment on attachment 541388 [details] [diff] [review]
fix bug: arev was meant instead of amap

Marking as r+: the patch has been taken upstream.
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2011-06-27 10:47:23 PDT
Landed (same as ANGLE r699):
http://hg.mozilla.org/mozilla-central/rev/51e18435b793
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2011-06-29 21:04:28 PDT
Landed on Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/9eabe65eb25a
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-07 14:03:17 PDT
Does the fact that we took a patch for this on m-c and aurora mean that it's fixed on trunk and aurora, which means it got uplifted to beta?
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2011-07-07 14:06:18 PDT
Yes, unless there are more bugs here, which I don't know. I've not been able to reproduce the bug myself.
Comment 18 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-07 15:14:20 PDT
Ok, thanks. Would it make sense to close this bug out then? I'm suggesting we do so, and open new bugs as more stuff is found, if that's even the case. I'll leave closing this bug for trunk up to you tho, but I doubt we'll do any more than what we've done here for 6 and 7.
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2011-07-07 16:02:57 PDT
Sure, seems reasonable.
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 15:24:49 PDT
qa- as no QA fix verification needed
Comment 21 Raymond Forbes[:rforbes] 2013-07-19 18:18:54 PDT
rforbes-bugspam-for-setting-that-bounty-flag-20130719

Note You need to log in before you can comment on or make changes to this bug.