890277 - (CVE-2013-1721) ANGLE libGLESv2 Integer Overflow

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Alex Chapman]

Attachment: Crash trigger

Description:		Almost Native Graphics Layer Engine (ANGLE) library Integer Overflow
Versions Affected: 	Mozilla Firefox 22.0, Mozilla Nightly 25.0a1 (2013-07-03)
Category: 		Memory Corruption
Reporter:		Alex Chapman of Context Information Security 

Summary:
--------

The Almost Native Graphics Layer Engine (ANGLE) library as used in Mozilla Firefox is vulnerable to an integer overflow vulnerability leading to memory corruption which may allow an attacker to exploit the browser process. The ANGLE library is used by Firefox on Windows as a bridge between the OpenGL ES 2.0 API and DirectX.

Note: As this is a bug in the ANGLE library a bug report has also been submitted to Google.

Analysis:
---------

The issue occurs due to insufficient bounds checking prior to integer multiplication within the Context::drawLineLoop function (libGLESv2/Context.cpp):

    const int spaceNeeded = (count + 1) * sizeof(unsigned int);

    if (!mLineLoopIB)
    {
        mLineLoopIB = new StreamingIndexBuffer(mDevice, INITIAL_INDEX_BUFFER_SIZE, D3DFMT_INDEX32);
    }

    if (mLineLoopIB)
    {
        mLineLoopIB->reserveSpace(spaceNeeded, GL_UNSIGNED_INT);

        UINT offset = 0;
        unsigned int *data = static_cast<unsigned int*>(mLineLoopIB->map(spaceNeeded, &offset));
        startIndex = offset / 4;
        
        if (data)
        {
            switch (type)
            {
              case GL_NONE:   // Non-indexed draw
                for (int i = 0; i < count; i++)
                {
                    data[i] = i;
                }
                data[count] = 0;
                break;

The count variable is directly controlled from javascript with no validation performed by the ANGLE library. Context::drawLineLoop proceeds to request a buffer using the overflowed spaceNeeded variable as the size, and fill the insufficiently allocated memory with data resulting in a heap overflow condition.

Example crash information from Firefox 22.0:

    (1788.17d4): Access violation - code c0000005 (first chance)
    First chance exceptions are reported before any exception handling.
    This exception may be expected and handled.
    eax=06723000 ebx=7fffffff ecx=00218800 edx=0057d160 esi=0286a368 edi=00000000
    eip=69075035 esp=0057d174 ebp=0057d188 iopl=0         nv up ei ng nz ac po cy
    cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00210293
    libGLESv2!gl::Context::drawLineLoop+0x123:
    69075035 890c88          mov     dword ptr [eax+ecx*4],ecx ds:002b:06f85000=????????


    ChildEBP RetAddr  Args to Child              
    0057d188 69074d23 00000000 00000000 00000000 libGLESv2!gl::Context::drawLineLoop+0x123 [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\angle\src\libglesv2\context.cpp @ 3250]
    0057d1b4 6907a5c7 00000002 00000000 7fffffff libGLESv2!gl::Context::drawArrays+0xee [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\angle\src\libglesv2\context.cpp @ 3105]
    0057d1ec 61c2d9a2 00000002 00000000 7fffffff libGLESv2!glDrawArrays+0x4d [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\angle\src\libglesv2\libglesv2.cpp @ 16707566]
    0057d200 62026f18 0fae7400 00000002 00000000 xul!mozilla::gl::GLContext::fDrawArrays+0x17 [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\obj-firefox\dist\include\glcontext.h @ 685]
    0057d238 62029022 0bf4a6a0 00000002 00000000 xul!mozilla::WebGLContext::DrawArrays+0x123 [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\content\canvas\src\webglcontextgl.cpp @ 1472]
    0057d25c 61e59774 0c064600 0057d29c 0bf4a6a0 xul!mozilla::dom::WebGLRenderingContextBinding::drawArrays+0x83 [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\obj-firefox\dom\bindings\webglrenderingcontextbinding.cpp @ 5502]
    0057d28c 6a9f9515 0c064600 00000003 057d4190 xul!mozilla::dom::WebGLRenderingContextBinding::genericMethod+0xcd [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\obj-firefox\dom\bindings\webglrenderingcontextbinding.cpp @ 10325]
    0057d2e8 6a9e75ea 0c064600 00000000 04ca0070 mozjs!js::InvokeKernel+0x115 [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\js\src\jsinterp.cpp @ 390]


In addition to the above issue, a bug in the NVIDIA graphics driver (version information in gfx_driver_version.txt) and insufficient validation of the size of allocated memory returned by a call to IDirect3DDevice9::CreateIndexBuffer in StreamingIndexBuffer::reserveSpace (libGLESv2/IndexDataManager.cpp) can also trigger a similar heap overflow condition. With the buggy NVIDIA driver, calls to IDirect3DDevice9::CreateIndexBuffer with large memory sizes (e.g. 0xFFFF0004) fail to allocate the requested amount of memory, but return a success result (HRESULT 0). Subsequent code in StreamingIndexBuffer::reserveSpace does not check the size of memory allocated before returning the buffer to the Context::drawLineLoop function (libGLESv2/Context.cpp) to be used. Context::drawLineLoop proceeds to fill the insufficiently allocated memory with data, assuming a successful memory allocation, resulting in a heap overflow condition: 

Example crash information from Friefox 22.0:

    (15b8.2280): Access violation - code c0000005 (first chance)
    First chance exceptions are reported before any exception handling.
    This exception may be expected and handled.
    eax=06756000 ebx=3fffc000 ecx=001c2c00 edx=004ed6a0 esi=02afa9f0 edi=00000000
    eip=690c5035 esp=004ed6b4 ebp=004ed6c8 iopl=0         nv up ei ng nz na pe cy
    cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00210287
    libGLESv2!gl::Context::drawLineLoop+0x123:
    690c5035 890c88          mov     dword ptr [eax+ecx*4],ecx ds:002b:06e61000=????????



    ChildEBP RetAddr  Args to Child              
    004ed6c8 690c4d23 00000000 00000000 00000000 libGLESv2!gl::Context::drawLineLoop+0x123 [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\angle\src\libglesv2\context.cpp @ 3250]
    004ed6f4 690ca5c7 00000002 00000000 3fffc000 libGLESv2!gl::Context::drawArrays+0xee [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\angle\src\libglesv2\context.cpp @ 3105]
    004ed72c 61c2d9a2 00000002 00000000 3fffc000 libGLESv2!glDrawArrays+0x4d [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\angle\src\libglesv2\libglesv2.cpp @ 16707566]
    004ed740 62026f18 0c069400 00000002 00000000 xul!mozilla::gl::GLContext::fDrawArrays+0x17 [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\obj-firefox\dist\include\glcontext.h @ 685]
    004ed778 62029022 0c1474f0 00000002 00000000 xul!mozilla::WebGLContext::DrawArrays+0x123 [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\content\canvas\src\webglcontextgl.cpp @ 1472]
    004ed79c 61e59774 0c264600 004ed7dc 0c1474f0 xul!mozilla::dom::WebGLRenderingContextBinding::drawArrays+0x83 [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\obj-firefox\dom\bindings\webglrenderingcontextbinding.cpp @ 5502]
    004ed7cc 6aa19515 0c264600 00000003 0552b2b0 xul!mozilla::dom::WebGLRenderingContextBinding::genericMethod+0xcd [e:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\obj-firefox\dom\bindings\webglrenderingcontextbinding.cpp @ 10325]



Proof of Concept:
-----------------

Enclosed in the zip file is a HTML file which triggers the interger overflow and subsequent heap corruption on affected browser versions on Windows systems.

* Expected Result of running angle_crash.html = No crash
* Actual Result of running angle_crash.html = Access violation - code c0000005 with heap corrupted

Comment 1

[Christoph Diehl [:posidron]]

Are we not running our crashtests on Windows machines. The testcase is identical to my produced fuzzer output and based on my local crashtests a call to gl.drawArrays() with a large integer as 3rd parameter occurred multiple times.

We first need to check some open WebGL bugs before setting any flags here!

Comment 2

[Daniel Veditz [:dveditz]]

jgilbert: can you answer Christoph's question about whether we're running our crashtests on windows regularly, and assuming we are why this wasn't detected?

Comment 3

[Kelsey Gilbert [:jgilbert]]

(In reply to Daniel Veditz [:dveditz] from comment #2)
> jgilbert: can you answer Christoph's question about whether we're running
> our crashtests on windows regularly, and assuming we are why this wasn't
> detected?

I would, but I can't. I didn't know we even had webgl crashtests, much less where they are and aren't run. It looks like we only have a couple, and none of them seem to do `drawArrays`, though maybe I missed one. (Having them not named usefully needs to change)

Comment 4

[Christoph Diehl [:posidron]]

The bloated testcase in https://bugzilla.mozilla.org/show_bug.cgi?id=636925 in had a
    gl.drawArrays(gl.TRIANGLES,0,4291051535)
in it but the testcase does not to trigger a crash (anymore)..

Comment 5

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

Milan is there anything actionable here? Can you find an owner?

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

I'm not sure if there is something actionable, or if the latest ANGLE fixes this.  Guillaume, can you try reproducing this?

Comment 7

[Guillaume Abadie]

Attachment: patch revision 1

Hi had some difficulties to recognize ANGLE. Searching for string "Google Inc." on GL_VENDOR doesn't work.

Comment 8

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

Comment on attachment 778632 [details] [diff] [review]
patch revision 1

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

Some questions:

::: content/canvas/src/WebGLContextGL.cpp
@@ +1383,5 @@
>          return ErrorInvalidOperation("drawArrays: bound vertex attribute buffers do not have sufficient size for given first and count");
>  
> +#ifdef XP_WIN
> +	if (gl->WorkAroundDriverBugs() &&
> +		mode == LOCAL_GL_LINE_LOOP)

bad indentation.

@@ +1385,5 @@
> +#ifdef XP_WIN
> +	if (gl->WorkAroundDriverBugs() &&
> +		mode == LOCAL_GL_LINE_LOOP)
> +	{// bug 890277
> +		uint64_t checkedSpaceNeeded = (uint64_t(count) + 1) * sizeof(unsigned int);

poor name. this is a count of bytes, so name it "checkedBytesNeeded"

@@ +1386,5 @@
> +	if (gl->WorkAroundDriverBugs() &&
> +		mode == LOCAL_GL_LINE_LOOP)
> +	{// bug 890277
> +		uint64_t checkedSpaceNeeded = (uint64_t(count) + 1) * sizeof(unsigned int);
> +		if (checkedSpaceNeeded > 0x7FFFFFFF) {

Please explain how we know that 0x7FFFFFFF is the highest allowable size? I couldn't find this information from a quick glance at the bug?

Comment 9

[Kelsey Gilbert [:jgilbert]]

I think there are two stages to fixing this. Stage one is that we incorrectly use CheckedUint32 instead of CheckedInt32 (or maybe CheckedInt<GLsizei>?).

Even still, ANGLE doesn't do bounds checking correctly here:
  const int spaceNeeded = (count + 1) * sizeof(unsigned int);

Interestingly, the spec doesn't define an error condition for too-large `count`. We appear to use INVALID_OPERATION, though I think INVALID_VALUE is more correct in this case.

While we can probably safely disallow drawing billions of prims, it is perhaps interesting to note that generally it's the size of the vertex attrib arrays that limits this number. However, for generic vertex attribs, we effectively have arbitrary length, but we can also just split the draw calls into reasonable chunks, as generic vertex attribs don't care about the index of their prim.

My vote:
Fix which CheckedInt type we use.
Patch ANGLE to generate its OOM in this case earlier. (And fix its math to be safe)

Comment 10

[Guillaume Abadie]

(In reply to Benoit Jacob [:bjacob] from comment #8)
> Comment on attachment 778632 [details] [diff] [review]
> patch revision 1
> 
> Review of attachment 778632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some questions:
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +1383,5 @@
> >          return ErrorInvalidOperation("drawArrays: bound vertex attribute buffers do not have sufficient size for given first and count");
> >  
> > +#ifdef XP_WIN
> > +	if (gl->WorkAroundDriverBugs() &&
> > +		mode == LOCAL_GL_LINE_LOOP)
> 
> bad indentation.
Oups ...
> 
> @@ +1385,5 @@
> > +#ifdef XP_WIN
> > +	if (gl->WorkAroundDriverBugs() &&
> > +		mode == LOCAL_GL_LINE_LOOP)
> > +	{// bug 890277
> > +		uint64_t checkedSpaceNeeded = (uint64_t(count) + 1) * sizeof(unsigned int);
> 
> poor name. this is a count of bytes, so name it "checkedBytesNeeded"
I have chosen checkedSpaceNeeded because the value in ANGLE that should check overflow is spaceNeeded :
const int spaceNeeded = (count + 1) * sizeof(unsigned int);
> 
> @@ +1386,5 @@
> > +	if (gl->WorkAroundDriverBugs() &&
> > +		mode == LOCAL_GL_LINE_LOOP)
> > +	{// bug 890277
> > +		uint64_t checkedSpaceNeeded = (uint64_t(count) + 1) * sizeof(unsigned int);
> > +		if (checkedSpaceNeeded > 0x7FFFFFFF) {
> 
> Please explain how we know that 0x7FFFFFFF is the highest allowable size? I
> couldn't find this information from a quick glance at the bug?
spaceNeeded in ANGLE is a integer.

Comment 11

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #9)
> I think there are two stages to fixing this. Stage one is that we
> incorrectly use CheckedUint32 instead of CheckedInt32 (or maybe
> CheckedInt<GLsizei>?).
> 
> Even still, ANGLE doesn't do bounds checking correctly here:
>   const int spaceNeeded = (count + 1) * sizeof(unsigned int);
> 
> Interestingly, the spec doesn't define an error condition for too-large
> `count`. We appear to use INVALID_OPERATION, though I think INVALID_VALUE is
> more correct in this case.
> 
> While we can probably safely disallow drawing billions of prims, it is
> perhaps interesting to note that generally it's the size of the vertex
> attrib arrays that limits this number. However, for generic vertex attribs,
> we effectively have arbitrary length, but we can also just split the draw
> calls into reasonable chunks, as generic vertex attribs don't care about the
> index of their prim.
> 
> My vote:
> Fix which CheckedInt type we use.
> Patch ANGLE to generate its OOM in this case earlier. (And fix its math to
> be safe)
It's true. But I already tried that and it replace the crash by a non-responding page, when this patch just remove the crash and let JavaScript keep-going.

Comment 12

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

Comment on attachment 778632 [details] [diff] [review]
patch revision 1

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

r- to clear my review queue

Comment 13

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

(In reply to Jeff Gilbert [:jgilbert] from comment #9)
> I think there are two stages to fixing this. Stage one is that we
> incorrectly use CheckedUint32 instead of CheckedInt32 (or maybe
> CheckedInt<GLsizei>?).

Oh, I didn't realize that. You're right, GLsizei is int32.

> 
> Even still, ANGLE doesn't do bounds checking correctly here:
>   const int spaceNeeded = (count + 1) * sizeof(unsigned int);

Ouch.

Comment 14

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

@ Alex: is there an ANGLE bug filed for this? Or a recent ANGLE changeset fixing this?

Comment 15

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

I agree with Jeff, it's best to fix this in ANGLE than work around the ANGLE bug in our code. So we're blocked on understanding: is this already being fixed in ANGLE, is there already a patch that we can use? One thing to do would be to check the current state of this code in ANGLE's trunk tree.

Comment 16

[Alex Chapman]

@Benoit After also reporting this bug to Chrome it looks like a fix has landed in the ANGLE project. More details here:
https://code.google.com/p/angleproject/issues/detail?id=444

Hope that helps

Comment 17

[Guillaume Abadie]

Attachment: patch revision 2

Comment 18

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

Comment on attachment 779278 [details] [diff] [review]
patch revision 2

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

This should use CheckedInt.

Comment 19

[Guillaume Abadie]

Attachment: patch revision 3

https://tbpl.mozilla.org/?tree=Try&rev=a0e795012e16

Comment 20

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

Ouch, sorry, little misunderstanding here. As said above, I believe that we should rather import the ANGLE fix. See Alex's comment 16.

Comment 21

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

Comment on attachment 779305 [details] [diff] [review]
patch revision 3

This patch is fine, but we'll rather import the ANGLE fix.

Comment 22

[Guillaume Abadie]

Attachment: patch revision 4

https://tbpl.mozilla.org/?tree=Try&rev=9b8c0ecdc65b
This patch apply the ANGLE patch.

Comment 24

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

Comment on attachment 779398 [details] [diff] [review]
patch revision 4

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

Yes, that is the same as

   https://code.google.com/p/angleproject/source/diff?spec=svn07dda9519cf4cd8558cfd8b5221259348fe20204&name=5d1cff&r=07dda9519cf4cd8558cfd8b5221259348fe20204&format=side&path=/src/libGLESv2/IndexDataManager.cpp

please keep identical byte-for-byte (the ANGLE patch doesn't seem to have the newline?) and record this patch as a .patch file in gfx/angle/, and mention it in gfx/angle/README.mozilla, like for other existing ANGLE patches that we have.

Comment 25

[Guillaume Abadie]

(In reply to Benoit Jacob [:bjacob] from comment #24)
> Comment on attachment 779398 [details] [diff] [review]
> patch revision 4
> 
> Review of attachment 779398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, that is the same as
> 
>   
> https://code.google.com/p/angleproject/source/
> diff?spec=svn07dda9519cf4cd8558cfd8b5221259348fe20204&name=5d1cff&r=07dda9519
> cf4cd8558cfd8b5221259348fe20204&format=side&path=/src/libGLESv2/
> IndexDataManager.cpp
> 
> please keep identical byte-for-byte (the ANGLE patch doesn't seem to have
> the newline?) and record this patch as a .patch file in gfx/angle/, and
> mention it in gfx/angle/README.mozilla, like for other existing ANGLE
> patches that we have.
Yes I just added the newline to don't have a to long line and be sure to get my r+. Should I undo this initiative ?

Comment 26

[Guillaume Abadie]

Attachment: patch revision 5

https://tbpl.mozilla.org/?tree=Try&rev=efaa2384b5d0

Comment 27

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

Comment on attachment 779996 [details] [diff] [review]
patch revision 5

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

r=me but it's important to add this information to the readme:

::: gfx/angle/README.mozilla
@@ +34,5 @@
>    angle-cross-compilation.patch
>      Fixes cross compilation on case sensitive OSes.
>  
> +  angle-line-loop-overflow.patch
> +    Fixes a forgotten overflow check on drawing line loop

This should mention the angle changeset id and issue number (444).

Comment 28

[Guillaume Abadie]

Fixed and added the changeset id. Waiting for tpbl results before landing.

Comment 29

[Guillaume Abadie]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dbcf7b6347a

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0dbcf7b6347a

Can we land a test for this?

Comment 31

[bhavana bajaj [:bajaj]]

Needinfo Guillaume Abadie, to help with the beta patch for Fx24.

Comment 32

[bhavana bajaj [:bajaj]]

bjacob, can you please help with the uplift request here ? I am unable to get in touch with :gadabie atm and would like to take this in Beta going to build ~tomorrow morning PT.

Comment 33

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

OK, I am in Europe atm so it's late for me today, but if Guillaume doesn't do it today, I will do it tomorrow morning (in Europe) so that will be well before tomorrow morning PT.

Comment 34

[bhavana bajaj [:bajaj]]

(In reply to Benoit Jacob [:bjacob] from comment #33)
> OK, I am in Europe atm so it's late for me today, but if Guillaume doesn't
> do it today, I will do it tomorrow morning (in Europe) so that will be well
> before tomorrow morning PT.

Sounds good ! thanks :)

Comment 35

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

https://hg.mozilla.org/releases/mozilla-beta/rev/229a2402debc

Note: I assumed a=bajaj, hope that's OK.

Comment 36

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

Is ESR or B2G affected by this?