Closed Bug 685793 Opened 8 years ago Closed 8 years ago

WebGL buffer overflow issue and access to out of range memory

Categories

(Core :: Canvas: WebGL, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox7 - affected
firefox8 + affected
firefox9 + affected
status1.9.2 --- unaffected

People

(Reporter: mozilla2, Assigned: bjacob)

Details

(Whiteboard: [sg:moderate][qa?])

Attachments

(1 file)

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.
Summary: WebGL buffer overflow issue and access to our of range memory → WebGL buffer overflow issue and access to out of range memory
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.
Wow, thanks a lot, again, for this report. Making a patch.
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?
Attachment #559703 - Flags: review?(jmuizelaar) → review+
(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.
Note that ErrorInvalidValue and friends also return NS_OK.
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".
Assignee: nobody → bjacob
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:moderate]
Oooooooops, forgot to land this baby. Doing it tonight.

Will also reply to comment 8 tonight.

Sorry sorry. Just slipped out of my head.
http://hg.mozilla.org/mozilla-central/rev/6935f0893612
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #559703 - Flags: approval-mozilla-beta?
Attachment #559703 - Flags: approval-mozilla-aurora?
(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 on attachment 559703 [details] [diff] [review]
catch integer overflow and glBufferData errors

this can ride the trains.
Attachment #559703 - Flags: approval-mozilla-beta?
Attachment #559703 - Flags: approval-mozilla-beta-
Attachment #559703 - Flags: approval-mozilla-aurora?
Attachment #559703 - Flags: approval-mozilla-aurora-
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.
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...
Assignee: bjacob → nobody
Component: General → Canvas: WebGL
Product: Firefox → Core
QA Contact: general → canvas.webgl
Assignee: nobody → bjacob
Is there something QA can do to verify this fix?
Whiteboard: [sg:moderate] → [sg:moderate][qa?]
(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! :-)
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.