685793 - WebGL buffer overflow issue and access to out of range memory

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Gregg Tavares]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/13.0.782.220 Safari/535.1

Steps to reproduce:

Hey There,

I was looking into a memory access bug in Chrome and thought I'd check if the same bug exists in Firefox. It appears it does.

In content/canvas/src/WebGLContext.cpp in DoFakeVertexAttrib0 there are 2 math overflows and no check that glBufferData actually succeeds.

the 2 math overflows (sizeof(WebGLFloat) * 4 * vertexCount means that you can pass in a large count, the math will overflow, the glBufferData call will make a buffer that is too small. The resulting draw will end up accessing data outside the buffer.

Not checking for glBufferData failure also means that data past the end of buffer will be accessed.

You should probably synthesize a GL_OUT_OF_MEMORY error if either the math overflows or the glBufferData does not return NO_ERROR.

Comment 1

[Gregg Tavares]

PS: We found this bug in Chrome using address sanitizer running on top of osmesa
http://code.google.com/p/address-sanitizer/

The code that triggered the bug is here
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/extra/simulated-attrib-0-bug-test.html

Running that page you are unlikely to see any issues. The bug is a read access issue.

Comment 2

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

Wow, thanks a lot, again, for this report. Making a patch.

Comment 3

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

Attachment: catch integer overflow and glBufferData errors

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 559703 [details] [diff] [review]
catch integer overflow and glBufferData errors

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +1294,1 @@
>  

Why can't we just have Checked<WebGLuint>?

@@ +1507,1 @@
>  

Is NS_OK the right thing to be returning?

Comment 5

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

(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Comment on attachment 559703 [details] [diff] [review]
> catch integer overflow and glBufferData errors
> 
> Review of attachment 559703 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +1294,1 @@
> >  
> 
> Why can't we just have Checked<WebGLuint>?

In the IDL, WebGLuint is a typedef for unsigned long. XPIDL generates from that a header where it's a typedef for PRUint32. So yes, for sure we currently can have CheckedInt<WebGLuint> as it's the exact same thing. However I would rather first have CheckedInt be expanded to support std int types (there's a patch on bug 555798) and maybe also have the integer types unification and removal of PRInt types happen first (bug 579517).

Notice that constructing a CheckedInt<T> from an integer of a different type is safe: if the different type is unsupported by CheckedInt then you get a compile error, and if it is supported then it's checked to be in range for type T before getting converted.

> 
> @@ +1507,1 @@
> >  
> 
> Is NS_OK the right thing to be returning?

It is. Returning anything else will generate a JS exception. That's why WebGL functions almost always return NS_OK even in WebGL error cases.

Comment 6

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

Note that ErrorInvalidValue and friends also return NS_OK.

Comment 7

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

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

Comment 8

[Daniel Veditz [:dveditz]]

If it's a read access violation presumably worst case is you can steal bits of someone else's memory into your image if you don't crash? What keeps us from writing past the end of the too-small buffer? That could be an sg:critical bug if the answer is "nothing".

Comment 9

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

Oooooooops, forgot to land this baby. Doing it tonight.

Will also reply to comment 8 tonight.

Sorry sorry. Just slipped out of my head.

Comment 10

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

http://hg.mozilla.org/mozilla-central/rev/6935f0893612

Comment 11

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

(In reply to Daniel Veditz from comment #8)
> If it's a read access violation presumably worst case is you can steal bits
> of someone else's memory into your image if you don't crash? What keeps us
> from writing past the end of the too-small buffer? That could be an
> sg:critical bug if the answer is "nothing".

TL;DR: not readily exploitable, would need someone smarter than me to exploit.

This is an invalid-read bug. There is no invalid-write here as far as I can see. We just create a too small WebGL buffer, it's only being written to immediately with the same size, so no invalid write. The invalid read occurs in subsequent draw-call using this attrib buffer.

This bug affects only desktop OpenGL (mostly Mac and Linux).

It is not clear to me that it can be exploited at all. Since it's purely an invalid read bug, exploiting it would mean exploiting it to read video memory.

The first thing to notice is that this bug can only be triggered by drawArrays, not drawElements, since drawElements can only use 16bit indices, so they can't exceed 65535, so they can't trigger this integer overflow (4*4*65536=1048576, no overflow).

So this can only be triggered by a drawArrays call on at least 2^27 vertices. Indeed 4*4*2^27 == 2^31 is needed to overflow. To bypass drawArrays' check that enabled attrib arrays are large enough, all vertex attrib arrays have to be disabled, since it is practically impossible on any current hardware to allocate a WebGL buffer array of size 2^27 (even with 1 float per vertex this would still be 512 M). The illegal video memory reads would then be into the emulated vertex 0 attrib array (this emulation takes place to please a desktop OpenGL requirement that attrib 0 array must always be enabled).

This raises some difficulties:
 1) Since this is a drawArrays call and all attrib arrays have to be disabled, how would this vertex shader proceed to store the illegally read data (attrib 0) in exploitable form? Where "exploitable" means that it must allow to paint something that will reveal the contents of attrib 0.
 2) Is it actually feasible to paint 2^27 vertices in 1 draw-call? Wouldn't that take too long, DOSing the driver?

So, I don't consider this readily exploitable.

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 559703 [details] [diff] [review]
catch integer overflow and glBufferData errors

this can ride the trains.

Comment 13

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

Hm. Not even aurora? This is hard to exploit, but still a security issue. The risk is super low. I would like it to be taken in aurora.

Comment 14

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

Cedric who's sitting besides me at the Khronos meeting points out that while this is hard to exploit on mainstream hardware, it may be easier to exploit on high-end hardware, and security researchers looking for an opportunity to get attention could easily be watching mozilla-central where this has already landed, exploit the bug on some high end hardware and release a video...

Comment 15

[u279076]

Is there something QA can do to verify this fix?

Comment 16

[LegNeato]

(In reply to Benoit Jacob [:bjacob] from comment #13)
> Hm. Not even aurora? This is hard to exploit, but still a security issue.
> The risk is super low. I would like it to be taken in aurora.

Renominate and we'll look at it again! :-)