744888 - (CVE-2012-3105) Work around NVIDIA driver bug in glBufferData

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

Thanks to Ken/Google for letting us know about this. My understanding of

  http://code.google.com/p/chromium/issues/detail?id=118970

is that the NVIDIA drivers have a bug: glBufferData with null data doesn't actually allocate the buffer until actual data is uploaded to it.

This affects WebGL. We could work around this in 2 different places in WebGL (in WebGL bufferData and in the attrib 0 emulation code), but instead, I think that this is best handled at the level of GLContext.

Comment 1

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

Attachment: work around nvidia bufferdata bug

Comment 2

[Kelsey Gilbert [:jgilbert]]

Patch looks fine, and should fix the problem, but it probably fixes it too conservatively. This forces us to allocate memory, zero, and upload a buffer of the full size when what is being asked for is merely buffer allocation. If we're just trying to assure the buffer is allocated and sized properly, we should just be able to upload one byte at the very end of the buffer.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 614571 [details] [diff] [review]
work around nvidia bufferdata bug

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

Moving to r- pending discussion and/or clarification.

Comment 4

[Kenneth Russell]

Note that when Chrome's GPU subsystem receives a glBufferData call from an untrusted client with NULL as the input, it explicitly allocates a large zeroed buffer and uploads it to the GPU. See GLES2DecoderImpl::DoBufferData, http://src.chromium.org/viewvc/chrome/trunk/src/gpu/command_buffer/service/gles2_cmd_decoder.cc?view=markup .

Comment 5

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

Ken: thanks, the notion a 'trusted client' at the level of the GL wrapper is quite interesting. Maybe we should do the same.

Comment 6

[Kelsey Gilbert [:jgilbert]]

webgl.bufferData(null) is disallowed, but I don't think we care otherwise. I think the important issue here is just to be sure buffers are allocated when we ask them to be. Ideally, we would just defer this until we (maybe) need them, and (maybe) assure allocation then. (ideally warning if we are using uninitialized buffers, though it should only help us internally, and we shouldn't be having any problems there)

I need to read through the chromium bug again when I'm more coherent.

After re-reading it anyways, I think I understand better. Part of the root of the problem is that the driver 'consumes'(reads) all enabled vertex arrays, even if the in-use program does not read from them. (Which I was certainly not aware of) If a too-short (or in this driver's case, lazily-thus-not-yet-allocated) buffer is bound to an enabled vertex pointer, the driver can be accessing uninitialized/invalid memory.

I think I'll need to read through our implementation and trace this out for myself.

Comment 7

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

Attachment: like this instead?

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 614867 [details] [diff] [review]
like this instead?

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

This is what seems like a more minimal change, to me.

::: gfx/gl/GLContext.h
@@ +2014,5 @@
> +        if (WorkAroundDriverBugs() &&
> +            !data &&
> +            Vendor() == VendorNVIDIA)
> +        {
> +            char c;

You might want to initialize this to keep everything happy about uninitialized variables being used. (Not that it should matter in practice, but still)

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/e74b044c18b8

Comment 10

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

Comment on attachment 614867 [details] [diff] [review]
like this instead?

[Approval Request Comment]
Regression caused by (bug #): We've had this problem since WebGL shipped in Firefox 4
User impact if declined: security issue: information leakage vulnerability with NVIDIA driver
Testing completed (on m-c, etc.): been on m-c for a few days.
Risk to taking this patch (and alternatives if risky): Does not seem risky. Could be a tiny performance overhead. Could fail to fix the problem. If we run into another unforeseen driver bug, it could even lead to a crash, but we don't have any definite reason to think that such a driver bug is waiting for us.
String changes made by this patch: none

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 614867 [details] [diff] [review]
like this instead?

[Triage Comment]
We weighed the risk and reward of this nomination and decided that this <sg:high issue was not worth landing between beta 6 and our final build. Approving for Aurora 13, however.

Comment 12

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

http://hg.mozilla.org/releases/mozilla-aurora/rev/9edd931bf07f

Comment 13

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

bustage fix:
http://hg.mozilla.org/releases/mozilla-aurora/rev/f53d4a882978

Comment 14

[u279076]

Is there a testcase QA can use to verify this fix?

Comment 15

[u279076]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #14)
> Is there a testcase QA can use to verify this fix?

Bump.

Comment 16

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

The original Chromium bug has a testcase:
http://code.google.com/p/chromium/issues/detail?id=118970

Comment 17

[u279076]

Thanks Benoit.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Assigning to Benoit to get this nominated, and landed to ESR.

Comment 19

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

Comment on attachment 614867 [details] [diff] [review]
like this instead?

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Well, this bug allows exploiting an information leakage vulnerability in NVIDIA drivers, that would probably rate sg:high
User impact if declined: information leakage vulnerability with NVIDIA driver
Fix Landed on Version: 14
Risk to taking this patch (and alternatives if risky): low risk (see above aurora approval request)
String changes made by this patch: none

Comment 20

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

Anthony, do you have a copy of the testcase? I have no access to http://code.google.com/p/chromium/issues/detail?id=118970.

Comment 21

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

Attachment: CanvasMatrix.js to use with the testcase

Comment 22

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

Attachment: testcase: unknown-2.html

Here's the testcase. The chromium bug says:

REPRODUCTION CASE
1. Download both files (nothing wrong with CanvasMatrix.js)
2. chrome-asan unknown-2.html
3. gpu process crashes

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 614867 [details] [diff] [review]
like this instead?

low risk, sg:high and fix already on mozilla-beta (13), approving for ESR landing.

Comment 24

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

Running this testcase against a Firefox 12 on my Win 7 x64 box, which has an nvdia crash, I don't have any issues. Does this only reproduce on certain operating systems or in ASAN builds (of which we have none on Windows)?

Comment 25

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

(In reply to Lukas Blakk [:lsblakk] from comment #23)
> low risk, sg:high and fix already on mozilla-beta (13), approving for ESR
> landing.

If this is sg:high, why isn't it marked as such?

Comment 26

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

(In reply to Al Billings [:abillings] from comment #25)
> (In reply to Lukas Blakk [:lsblakk] from comment #23)
> > low risk, sg:high and fix already on mozilla-beta (13), approving for ESR
> > landing.
> 
> If this is sg:high, why isn't it marked as such?

Maybe my views on what's sg:high aren't universally shared, or maybe it's just an oversight.

Patch ready for landing on ESR10 but tree is closed due to bug 756365.

Comment 27

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

https://hg.mozilla.org/releases/mozilla-esr10/rev/d740253129c7

Comment 28

[Kevin Brosnan [Ex-Mozilla]]

I don't crash on Firefox 12 with the following NVIDIA driver/hardware so I am unable to verify this fix. Is there specific driver versions that are affected or hardware mentioned in the Chrome bug?

  Application Basics
        Name
        Firefox

        Version
        12.0

        User Agent
        Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0

  Graphics

        Adapter Description
        NVIDIA GeForce GT 240M

        Vendor ID
        0x10de

        Device ID
        0x0a34

        Adapter RAM
        1024

        Adapter Drivers
        nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um

        Driver Version
        8.17.13.142

        Driver Date
        5-15-2012

        Direct2D Enabled
        true

        DirectWrite Enabled
        true (6.1.7601.17789)

        ClearType Parameters
        DISPLAY1 [ Gamma: 2200 Pixel Structure: RGB ClearType Level: 100 Enhanced Contrast: 400 ] DISPLAY2 [ Gamma: 2200 Pixel Structure: RGB ClearType Level: 50 Enhanced Contrast: 200 ]

        WebGL Renderer
        Google Inc. -- ANGLE (NVIDIA GeForce GT 240M) -- OpenGL ES 2.0 (ANGLE 1.0.0.963)

        GPU Accelerated Windows
        1/1 Direct3D 10

        AzureBackend
        direct2d

Comment 29

[u279076]

Benoit, this bug seems to be unreproducible on our end. Do you have any leads QA can pursue to test this fix? 

If there is someone else watching this bug who is able to test this, please help us out by verifying it is fixed. QA is seeking verification in Firefox 13, 10.0.5ESR and 14.0a2.

Thank you.

Comment 30

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

It's alright, driver bugs are often tricky to reproduce. If you tried and couldn't reproduce, it's probably OK to stop there on this bug.

Comment 31

[u279076]

Marking [qa-] since QA is unable to reproduce the original issue. Please mark [qa+] is a more reproducible case is found so we can verify this is fixed.

Thanks