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

RESOLVED FIXED in Firefox 23

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

({csectype-disclosure, csectype-uninitialized, sec-high})

unspecified
mozilla25
Points:
---

Firefox Tracking Flags

(firefox22- wontfix, firefox23+ fixed, firefox24+ fixed, firefox25+ fixed, firefox-esr1723+ fixed, b2g1823+ fixed)

Details

(Whiteboard: [adv-main23+][adv-esr1708+][qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
Attachment #768675 - Flags: review?(bjacob)
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 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!
Attachment #768675 - Flags: review?(bjacob) → review-
This can potentially leak uninitialized video memory to scripts, and is therefore sec-high.
Keywords: sec-high
(Assignee)

Comment 4

6 years ago
Posted patch patchSplinter Review
Attachment #768675 - Attachment is obsolete: true
Attachment #769233 - Flags: review?(bjacob)
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
Attachment #769233 - Flags: review?(bjacob) → review+
(Assignee)

Comment 6

6 years ago
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.
Attachment #769233 - Flags: sec-approval?
Attachment #769233 - Flags: approval-mozilla-beta?
Attachment #769233 - Flags: approval-mozilla-aurora?
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.
Attachment #769233 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/406437684622
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Attachment #769233 - Flags: approval-mozilla-beta?
Attachment #769233 - Flags: approval-mozilla-beta+
Attachment #769233 - Flags: approval-mozilla-aurora?
Attachment #769233 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 12

6 years ago
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.
Attachment #769233 - Flags: approval-mozilla-release?
Attachment #769233 - Flags: approval-mozilla-esr17?
Attachment #769233 - Flags: approval-mozilla-release?
Attachment #769233 - Flags: approval-mozilla-release-
Attachment #769233 - Flags: approval-mozilla-esr17?
Attachment #769233 - Flags: approval-mozilla-esr17+
(Assignee)

Comment 14

6 years ago
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.
Attachment #769233 - Flags: approval-mozilla-b2g18?
Whiteboard: [adv-main23+][adv-esr1708+]
Marking qa- due to lack of test case or steps. If that changes, feel free to advise how it could be verified. Thanks.
Whiteboard: [adv-main23+][adv-esr1708+] → [adv-main23+][adv-esr1708+][qa-]
Attachment #769233 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Group: core-security
You need to log in before you can comment on or make changes to this bug.