888107 - We don't clear newly allocated WebGL array buffers if their contents are not initially specified

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: calloc data for pure-alloc BufferData calls

I'm not sure if this is actually a sec issue. bjacob might have a better idea.

Do we do this for a reason? Does the spec specify that they're cleared to zero?

I ran into this while updating ANGLE. ANGLE currently has a nice bug where it tries to copy the `data` during BufferData, even if `data` is null. While this should work anyways, I was surprised that we do this at all, since it's possible to read data back out of array buffers obliquely.

Comment 1

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

You are right, we are actually calling this with data==null in the WebGL BufferData signature taking a size, and the GL ES 2.0.25 spec section 2.9 Buffer Object says:

> If data is non-null, then the source data is copied to the buffer object’s data store. If data is null, then the contents of the buffer object’s data store are undefined.

However, the job of this CheckedBufferData function is just to be a thin wrapper around the GL function with the same semantics, with the only difference being that it checks for errors (specifically, for OUT_OF_MEMORY).

I don't want it to become a general "let's add all sorts of checks that WebGL needs" ground. Since the need to initialize the buffer is specific to WebGL, and specifically to the BufferData signature taking a size, I believe that the right place for this calloc is there. In this way, you'll have simpler code as you will know that you need to calloc, no need for an if().

Comment 2

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

Comment on attachment 768675 [details] [diff] [review]
patch: calloc data for pure-alloc BufferData calls

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

r- because better place to do this check, but great catch!

Comment 3

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

This can potentially leak uninitialized video memory to scripts, and is therefore sec-high.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Comment 5

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

Comment on attachment 769233 [details] [diff] [review]
patch

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +386,5 @@
>      InvalidateCachedMinInUseAttribArrayLength();
>  
> +    GLenum error = CheckedBufferData(target, size, zeroBuffer, usage);
> +    free(zeroBuffer);
> +    

trailing \w

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 769233 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): webgl
User impact if declined: Potential for a website to read uninitialized GPU memory, which can include pixels from a user's desktop.
Testing completed (on m-c, etc.): None yet.
Risk to taking this patch (and alternatives if risky): Very low.
String or IDL/UUID changes made by this patch: None.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Medium-easy.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It's not called out, but it's quite clear what's being fixed.

Which older supported branches are affected by this flaw?
All of them.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This is the backport, and it should be very low risk.

How likely is this patch to cause regressions; how much testing does it need?
Not likely to regress.

Comment 7

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

Comment on attachment 769233 [details] [diff] [review]
patch

Discussed this one with Dveditz too. This is good to go. Sec-approval+ for trunk. We'll want this on branches after it goes in.

Comment 8

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/406437684622

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/406437684622

Comment 10

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-beta/rev/960a41eb5c01

Comment 11

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6723694edbb

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 769233 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: Potential for reading uninitialized graphics memory, which has previously included images of the desktop.
Fix Landed on Version: 23, 24, 25
Risk to taking this patch (and alternatives if risky): Very low.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Regression caused by (bug #): WebGL implementation.
User impact if declined: Potential for reading uninitialized graphics memory, which has previously included images of the desktop.
Testing completed (on m-c, etc.): On m-c, Aurora, and Beta.
Risk to taking this patch (and alternatives if risky): Very low.
String or IDL/UUID changes made by this patch: None.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/adb2cd320578

Is this something we want for b2g18 still?

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 769233 [details] [diff] [review]
patch

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
See previous requests.

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking qa- due to lack of test case or steps. If that changes, feel free to advise how it could be verified. Thanks.

Comment 16

[Kelsey Gilbert [:jgilbert]]

remote:   https://hg.mozilla.org/releases/mozilla-b2g18/rev/bc2c6813842e