665070 - WebGL functions allocating buffers/textures must check that the GL call succeeded, to prevent allowing context to make out-of-bounds accesses

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

Thanks a lot to Gregg Tavares at Google for reporting this:

=== BEGIN EMAIL ===

Hi Guys,

While I was looking into some perf issues I found what I think is a security issue in both Firefox and WebKit implementations of WebGL

The issue is that neither of these implementations check for errors on glBufferData, glTexImage2D or glCopyTexImage2D

So for example if a user does this

    buf = gl.createBuffer();
    gl.bindBuffer(gl.ARRAY_BUFFER, buf);
    gl.bufferData(gl.ARRAY_BUFFER, veryLargeSizeThatRunsOutOfMemoryOrIsTooLargeForDriver, gl.STATIC_DRAW);     // assume this fails


At this point, because neither WebKit nor Firefox check for error they think the buffer is veryLargeSize...

So, for example, validating indices when calling gl.drawElements will be wrong.

For textures, the same issues would exist. For example, create a texture that is too big, creation fails but browser thinks the texture is large since it did not check for error. Now bind that texture to an FBO and call readPixels. You can now read pixels outside the bounds of the texture since the implementation will not be clearing past the bounds.

=== END EMAIL ===

Comment 1

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

WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=62902

Comment 2

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

Attachment: part 1: implement GLContext::GetAndClearError

Extra thin patches to maximize your reviewing pleasure.

Comment 3

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

Attachment: part 2: use GetAndClearError in WebGL initialization

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 541174 [details] [diff] [review]
part 1: implement GLContext::GetAndClearError


> 
> public:
>+ 
>+    /** \returns the first GL error, and guarantees that all GL error flags are cleared,
>+      * i.e. that a subsequent GetError call will return NO_ERROR
>+      */
>+    GLenum GetAndClearError() {
>+        // the first error is what we want to return
>+        GLenum error = fGetError();
>+        
>+        if (error) {
>+            // clear all pending errors
>+            while(fGetError()) {}

add a space after 'while'


>+        }
>+        
>+        return error;
>+    }
> 

>     GLenum mGLError;
> 
> public:
>+

gratuitous whitespace change.

Comment 5

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

Attachment: part 3: use GetAndClearError in WebGLContext::GetError

Comment 6

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

Attachment: part 4: rename mSynthesizedGLError to mWebGLError

mSynthesizedGLError started as something only for webgl-specific errors that we generated by hand. But really what we need it to be is a data member that stores the WebGL error state irrespective of GL error state.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 541175 [details] [diff] [review]
part 2: use GetAndClearError in WebGL initialization

It looks like there's a bunch of unrelated change in this patch.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 541176 [details] [diff] [review]
part 3: use GetAndClearError in WebGLContext::GetError

> 
>-    // Always call glGetError to clear any pending
>+    // Always call GetAndClearError to clear any pending
>     // real GL error.

to clear all pending real GL errors.

Comment 9

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

Attachment: part 5: implement WebGLContext::UpdateWebGLErrorAndClearGLError

Explained in a comment. This function's semantics are very convenient: you can call it anywhere, as many times as you want, the WebGL error state remains coherent. The signature taking a GLenum* is useful when you want to know if a new GL error showed up.

Comment 10

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

Attachment: part 6: update SynthesizeGLError

Comment 11

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

Attachment: part 2: use GetAndClearError in WebGL initialization (v2)

Sorry... I'm starting to wonder if there's a bug in my version of hg/mq.

Comment 12

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

Attachment: part 7: add WebGLScopedConditionalCheck

This RAII device limits somewhat the risks of bugs, given that we will have to use it at least 6 times.

The 'conditional' part is because we will try to avoid doing this check based on certain conditions, to avoid losing performance when we can. For example, in TexImage2D, if the texture dimensions and format doesn't change, there's no need to do the check as out-of-bounds access would not happen anyway. The check is really expensive, as I'm afraid that in order to know if we're out of video memory the driver needs to sync with the GPU. The idea of this optimization comes from Ken.

Comment 13

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

Attachment: part 8: add ErrorName helper

Comment 14

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

Attachment: part 9: check error in bufferData

Comment 15

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

Attachment: part 10: check error in renderbufferStorage

Comment 16

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

Attachment: part 11: check error in texImage2D

Comment 17

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

Note: copyTexImage2D still missing, coming tomorrow, requires a little bit more work as I found another bug we have: we don't properly record the format of textures created by copyTexImage2D.

Comment 18

[Gregg Tavares]

I noticed another issue.

The error system in OpenGL is a set of bits. One bit for each type of error. (INVALID_ENUM, INVALID_VALUE, etc...)

glGetError returns one of those errors and the clears the bit for that error which is not what your implementation is emulating

Note: this was news to me too when I was implementing the Chrome version.

I added a WebGL conformance test to check for this when I saw your patch.
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/gl-geterror.html

Maybe you want a separate issue for that?

Comment 19

[Gregg Tavares]

And one more? Sorry, Benoit's comment about recording the format of textures created by copyTexImage2D scared me since you need to copy the format of each level of a texture. There is no overall format for the texture. So I checked the code just to double check. Yea, you guys are storing a format per level

but....

Assuming this is the latest code
http://hg.mozilla.org/releases/mozilla-aurora/file/c64b52363a1b/content/canvas/src/WebGLContextGL.cpp

I just thought I'd ask, what's line 4277

     tex->setDimensions(width, height);

for? It looks like that line is called for every level of the texture but it's only stored once per texture. That means that value is rarely valid.  For example if I had a 4x4 texture and set levels 0, 1, 2 the width and height would be set to 1x1 where as if I set levels 2, 1, 0 it would be 4x4!?!?

Is it used for something that I'm not understanding?

Comment 21

[Daniel Veditz [:dveditz]]

Can this be worse than reading from/writing to someone else's GPU memory? If it can interfere with executable code or data on the heap it's probably sg:critical instead of sg:high.

Comment 22

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

(In reply to comment #19)
> And one more? Sorry, Benoit's comment about recording the format of textures
> created by copyTexImage2D scared me since you need to copy the format of
> each level of a texture. There is no overall format for the texture. So I
> checked the code just to double check. Yea, you guys are storing a format
> per level
> 
> but....
> 
> Assuming this is the latest code
> http://hg.mozilla.org/releases/mozilla-aurora/file/c64b52363a1b/content/
> canvas/src/WebGLContextGL.cpp
> 
> I just thought I'd ask, what's line 4277
> 
>      tex->setDimensions(width, height);
> 
> for? It looks like that line is called for every level of the texture but
> it's only stored once per texture.

Yep, good catch, actually I had noticed the same thing today while working on this code. This is a remnant of old code, will take care of it ASAP.

Comment 23

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

(In reply to comment #18)
> I noticed another issue.
> 
> The error system in OpenGL is a set of bits. One bit for each type of error.
> (INVALID_ENUM, INVALID_VALUE, etc...)
> 
> glGetError returns one of those errors and the clears the bit for that error
> which is not what your implementation is emulating
> 
> Note: this was news to me too when I was implementing the Chrome version.
> 
> I added a WebGL conformance test to check for this when I saw your patch.
> https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/
> conformance/gl-geterror.html
> 
> Maybe you want a separate issue for that?

Thanks a lot for that, this had totally escaped me (I never understood what the 'there may be multiple error flags' story really was about).

Comment 24

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

(In reply to comment #21)
> Can this be worse than reading from/writing to someone else's GPU memory? If
> it can interfere with executable code or data on the heap it's probably
> sg:critical instead of sg:high.

No, at this point there is no reason to believe there that might be a code execution risk here. "Only" accessing video memory. (That's already a terrible thing to happen)

Comment 25

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

Attachment: part 12: textures are not RectangleObjects, but framebuffer attachments are

As discussed above with Gregg. It doesn't make sense to talk about the width and height of a texture because that can be different for each (mipmap) level and (cube map) face. However, framebuffer attachments have well-defined dimensions. When a texture is attached to a framebuffer, it's a specific level and face that's attached. So the WebGLRectangleObject inheritance moves to WebGLFramebufferAttachment.

Regarding WebGLFramebuffer, well it does have the dimensions of its color attachment whenever it's complete, but I don't want them to be stored a second time as that adds redundant state, so width() and height() just return the dimensions of the color attachment.

Comment 26

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

Attachment: part 12: textures are not RectangleObjects, but framebuffer attachments are

unwanted stuff in previous patch

Comment 27

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

Attachment: part 13: require all framebuffer attachments to have same size

While I'm at it, it seems like a good idea to explicitly require all framebuffer attachments to have the same size in order for the framebuffer to be considered complete. IMO, OpenGL framebuffer completeness rules are too foggy to rely on for something that could conceivably be an area where driver bugs allow reading back video memory.

Comment 28

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

(In reply to comment #18)
> I added a WebGL conformance test to check for this when I saw your patch.
> https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/
> conformance/gl-geterror.html

Thanks a lot for writing the test.

Is this intentional:
shouldBeTrue('err1 != err2 != err3');

Do the semantics of != in JS really allow that?

> Maybe you want a separate issue for that?

Filed bug 667084.

Comment 29

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

Attachment: part 14: check error in copyTexImage2D

And this completes the patch series. This part was easier than I somehow feared, because the texture format is just the |internalformat| parameter and the type is always GL_UNSIGNED_BYTE.

This patch also has some changes on WebGLTexture::ImageInfo: it was allowing not specifying format and type, leaving them as '0', and that doesn't make any sense.

Good news: we now pass 2 more WebGL test pages!

Comment 30

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

Gregg, do you have a test for this issue already, or should I write one?

Comment 31

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

(the 2 test pages that we now pass are not related to the actual security issue)

Comment 32

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #28)
> shouldBeTrue('err1 != err2 != err3');
> 
> Do the semantics of != in JS really allow that?

No, they don't - that looks broken. (I also wonder why this test harness seems to use eval()ed strings, seems horribly hacky)

Comment 33

[Gregg Tavares]

(In reply to comment #30)
> Gregg, do you have a test for this issue already, or should I write one?

For which one? I wrote a test that getError works. I'll fix the err1 != err2 != err3 if that's not valid jS.

I'll try writing one to test the other issues.

Comment 34

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

I mean a test for the original bug reported here i.e. the need to get for errors after glTexImage2D and friends.

Comment 35

[Gregg Tavares]

I'm not sure how I would test that. The only way I can think of is to allocate textures until WebGL runs out of memory but that doesn't seem like something you can do in a conformance test. It could run out of memory for other reasons.

I can't try allocating a texture that's too large since it shouldn't even get to glTexImage2D in that case (assuming the WebGL implementation is validating width and height before calling glTexImage2D which I think it's already doing. Similarly any other error (mis-matching internal format with format or type, border != 0, level out of range, all of those are validated before calling glTexImage2D.

I can write the out of memory test just to do it but I'm not sure we can include it in the list of must pass tests. Any other ideas?

Comment 36

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

Indeed, I hadn't thought of that, but it's not easy to write a reliable test.

Maybe bufferData is easier to test since one can request a 4G buffer which should always fail on current hardware, though it might start failing to fail in the near future.

Comment 37

[Jeff Muizelaar [:jrmuizel]]

Benoit, I haven't checked if it's there now or after these changes, but it's probably worth putting a short summary of how the gl error handling interacts with the webgl bindings near the declaration of the member variables involved.

Comment 38

[Jeff Muizelaar [:jrmuizel]]

Benoit and I decided to drop WebGLScopedConditionalGLErrorCheck.

Comment 39

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

Comment on attachment 541183 [details] [diff] [review]
part 7: add WebGLScopedConditionalCheck

Part 7 is obsolete, there is no part 7 anymore.

Comment 40

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

Attachment: part 9: check error in bufferData

Comment 41

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

Attachment: part 10: check error in renderbufferStorage

Comment 42

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

Attachment: part 11: check error in texImage2D

Comment 43

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

Attachment: part 14: check error in copyTexImage2D

Up for review again.

Comment 44

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

Attachment: part 14: check error in copyTexImage2D

Comment 45

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

Attachment: part 14: check error in copyTexImage2D

Comment 46

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 543052 [details] [diff] [review]
part 9: check error in bufferData


> 
>     if (!boundBuffer)
>         return ErrorInvalidOperation("BufferData: no buffer bound!");
> 
>     MakeContextCurrent();
>+    
>+    // bug 665070. if we try to increase the buffer size and that fails, we must
>+    // catch that GL error and avoid wrongly believing that we have a larger buffer

It's probably good to drop the "and avoid wrongly believ.." to avoid exposing the security problem more than necessary.

Comment 47

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 543053 [details] [diff] [review]
part 10: check error in renderbufferStorage


>-    gl->fRenderbufferStorage(target, internalformatForGL, width, height);
>+    MakeContextCurrent();
>+
>+    // bug 665070. if we try to increase the buffer size and that fails, we must
>+    // catch that GL error and avoid wrongly believing that we have a larger buffer

Same as last patch.

Comment 48

[Asa Dotzler [:asa]]

release drivers don't need to track 8 until it's uplifted to Aurora (6 weeks from today)

Comment 49

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

http://hg.mozilla.org/mozilla-central/rev/3d83c40b3590
http://hg.mozilla.org/mozilla-central/rev/3f0dd23263f2
http://hg.mozilla.org/mozilla-central/rev/2cb699b370e3
http://hg.mozilla.org/mozilla-central/rev/01a20b627def
http://hg.mozilla.org/mozilla-central/rev/de9daaa5585b
http://hg.mozilla.org/mozilla-central/rev/d302c1e3b975
http://hg.mozilla.org/mozilla-central/rev/3afd6f9d83cf
http://hg.mozilla.org/mozilla-central/rev/4f053dfe1b7d
http://hg.mozilla.org/mozilla-central/rev/b318f43fcd21
http://hg.mozilla.org/mozilla-central/rev/4fd3ac440415
http://hg.mozilla.org/mozilla-central/rev/afdbe635bde8
http://hg.mozilla.org/mozilla-central/rev/a56cd1f418fe
http://hg.mozilla.org/mozilla-central/rev/93f7d12c49bf

Comment 50

[Daniel Veditz [:dveditz]]

(In reply to comment #48)
> release drivers don't need to track 8 until it's uplifted to Aurora (6 weeks
> from today)

The security team needs to track that this was fixed in 8 though (through the status fields, not the "tracking" ones).

Comment 51

[Daniel Veditz [:dveditz]]

This seems like a lot of code change to shoe-horn into Beta. Bumping back to a nomination. Also not sure about the sg:high rating: if you don't think you can write a reliable testcase to demonstrate the problem is it exploitable in practice, or just in special targeted situations?

Comment 52

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

Regarding reproducibility, AFAICR the conversation stalled after comment 36:

(In reply to comment #36)
> Indeed, I hadn't thought of that, but it's not easy to write a reliable test.
> 
> Maybe bufferData is easier to test since one can request a 4G buffer which
> should always fail on current hardware, though it might start failing to
> fail in the near future.

While there's agreement that this is not practical to put in a testcase (high risk of intermittent failure/unexpected-success), the idea proposed in comment 36, i.e. calling bufferData with size=4G, would be a quite sure way of triggering this bug on most common hardware.

So I would recommend this at least for Aurora. I agree that this is bigger than what we like to take in Beta, so I won't push it for Beta. I could make a smaller, more self-contained hotfix for Beta, but that would only be trading size for untestedness and incompleteness.

Comment 53

[LegNeato]

We are no longer tracking this for Firefox 7. It should come up naturally in Firefox 8

Comment 55

[u279076]

As per testcase-wanted keyword, is there something QA can do to verify this fix?