Closed Bug 1301381 (CVE-2016-9897) Opened 8 years ago Closed 8 years ago

Memory corruption @ libGLESv2!rx::VaryingPacking::isFree


(Core :: Graphics: CanvasWebGL, defect)

51 Branch
Not set



Tracking Status
firefox-esr45 50+ verified
firefox50 + verified
firefox51 + verified
firefox52 + verified
firefox53 + verified


(Reporter: aral.yaman, Assigned: eflores)


(Keywords: csectype-bounds, reporter-external, sec-high, Whiteboard: [adv-main50.1+][adv-esr45.6+])


(3 files)

Attached file crash.html
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160907030427

Steps to reproduce:

Open crash.html

Actual results:

Firefox crashes after 1-2 sec.

The reason is when using vector constructor with varying array:

precision mediump float;  
varying  vec4 niqqki[100]; 
void main() {  
vec4 (niqqki[0]);

This could be a security issue ?

Expected results:

No crash or compiler error.
Attached file windbg.txt
:jgilbert, am I right in thinking this is something you would know more about?
Group: firefox-core-security → core-security
Component: Untriaged → Canvas: WebGL
Flags: needinfo?(jgilbert)
Product: Firefox → Core
There's also a ton of intermittent jemalloc heap corruption crashes on treeherder right now (see the blocks blocking bug 1301453).
IIRC we get this library from Google. Is Chrome affected? Is our version outdated?
Group: core-security → gfx-core-security
libGLES is a part from the angle project from Google -> 

I was not able to crash chrome.

I also compared the code from the angle projet and the mozilla code:

Maybe there is an difference in a other part of the code ... 

I also commited a crash report :
Flags: needinfo?(milan)
Jeff, it looks like you were the last one to update Angle. Can you figure out what the next step is for this bug? Thanks.
Flags: needinfo?(jmuizelaar)
(In reply to Andrew McCreight [:mccr8] from comment #6)
> Jeff, it looks like you were the last one to update Angle. Can you figure
> out what the next step is for this bug? Thanks.

It crashes (Windows) with the latest ANGLE as well (we had an updated, and this is still a problem.)  On OS X, no crash and an error/warning:

Error: WebGL: Drawing without vertex attrib 0 array enabled forces the browser to do expensive emulation work when running on desktop OpenGL platforms, for example on Mac. It is preferable to always draw with vertex attrib 0 array enabled, by using bindAttribLocation to bind some always-used attribute to location 0.
Assignee: nobody → edwin
Flags: needinfo?(milan)
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(jgilbert)
We trigger this assertion in VaryingPacking::isFree - ASSERT(registerRow + row < mRegisterMap.size());
OS: Unspecified → Windows
(In reply to Milan Sreckovic [:milan] from comment #8)
> We trigger this assertion in VaryingPacking::isFree - ASSERT(registerRow +
> row < mRegisterMap.size());

With varyingRows set to 100 and mRegisterMap sized 30.
Ever confirmed: true
Interesting. Chrome just refuses to use the shader, printing "useProgram: program not valid" to the console. We may be failing to catch an error from ANGLE somewhere.
That's with a release build, though, which probably has this assert compiled out. We reject the shader in the same way.

We probably just have to add a check for this at [1].

(In reply to Edwin Flores [:eflores] [:edwin] from comment #11)
> We probably just have to add a check for this at [1].

This works, but just trying to find whether it belongs somewhere else. Parser looks like a 'no', since it doesn't know about ContextState. Maybe in the compiler. Trying to understand the compiler code now.
Attachment #8803840 - Flags: review?(jmuizelaar) → review?(jgilbert)
Comment on attachment 8803840 [details] [diff] [review]

Review of attachment 8803840 [details] [diff] [review]:

This is fine, and we should upstream this.
Attachment #8803840 - Flags: review?(jgilbert) → review+
Is this patch going to be in the next release ?
(In reply to Aral from comment #15)
> Is this patch going to be in the next release ?

No. If it is was, the status flag for 50 would be marked "Fixed" and also this bug should be resolved. This patch isn't even checked in and the release goes out on Tuesday. It would have to have been checked in weeks ago.
Comment on attachment 8803840 [details] [diff] [review]

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not very. Looks fairly benign.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

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?
Should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?
Very low risk of regression.
Attachment #8803840 - Flags: sec-approval?
Comment on attachment 8803840 [details] [diff] [review]

sec-approval+ for trunk. We should backport to other affected versions as well.
Attachment #8803840 - Flags: sec-approval? → sec-approval+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Hi :edwin,
could you please nominate this uplift to Beta51 and Aurora52?
Flags: needinfo?(edwin)
Comment on attachment 8803840 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: ANGLE.
[User impact if declined]: Crashes from malicious or poor use of WebGL.
[Describe test coverage new/current, TreeHerder]: On m-c.
[Risks and why]: Very low. Just adds a bounds check.
[String/UUID change made/needed]: None.
Flags: needinfo?(edwin)
Attachment #8803840 - Flags: approval-mozilla-beta?
Attachment #8803840 - Flags: approval-mozilla-aurora?
Comment on attachment 8803840 [details] [diff] [review]

Fix a sec-high. Beta51+ and Aurora52+. Should be in 51 beta 2.
Attachment #8803840 - Flags: approval-mozilla-beta?
Attachment #8803840 - Flags: approval-mozilla-beta+
Attachment #8803840 - Flags: approval-mozilla-aurora?
Attachment #8803840 - Flags: approval-mozilla-aurora+
Comment on attachment 8803840 [details] [diff] [review]

See comment 22.
Attachment #8803840 - Flags: approval-mozilla-esr45?
Attachment #8803840 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Group: gfx-core-security → core-security-release
Temporarily reverted from esr45 for reasons. No action needed on your part, this'll be relanded at the appropriate time.
[Tracking Requested - why for this release]:
Comment on attachment 8803840 [details] [diff] [review]

Low-risk sec-high with a simple patch. Also, this was already landed on ESR45 for the 45.6 release, so I think that makes this more important to ship with 50.1 as well.
Attachment #8803840 - Flags: approval-mozilla-release?
Comment on attachment 8803840 [details] [diff] [review]

Sec-high, meets the triage bar for inclusion in 50.1.0
Attachment #8803840 - Flags: approval-mozilla-release? → approval-mozilla-release+
Flags: qe-verify+
Can't write a 50.1 advisory for this because it will 0day ESR45 users. ESR45 doesn't ship again until late January. This should not have gone into 50.1 for this reason.
Flags: needinfo?(abillings)
Ok. There is a 45.6 ESR release at the same time as 50.1. The advisory for this should be in the release.
Flags: needinfo?(abillings)
Whiteboard: [adv-main50.1+][adv-esr45.6+]
Alias: CVE-2016-9897
Flags: sec-bounty?
I've reproduced the bug on an affected build - 50.0 (20161104212021) - using the test case from Comment 0 on Windows 10 x64.

The issue is now confirmed fixed on Windows 10 x64, across the following versions:

        - 45.6.0esr-build1 (20161209150850)
        - 50.1.0-build2 (20161208153507)
        - 51.0b7-build1 (20161212025550)

The following version is *still affected* though:

        - 52.0a2 (2016-12-13)
            * crash report: bp-b391865f-e695-403c-8224-a2dc62161213
        - 53.0a1 (2016-12-13)
            * crash report: bp-4a1afa00-e71a-4357-931c-d67bc2161213

Edwin, Jeff -- any thoughts on what might be going on for 52 and 53?
Flags: needinfo?(jgilbert)
Flags: needinfo?(edwin.bugs)
Flags: sec-bounty? → sec-bounty+
Aral: in the future please email to request security bug bounty consideration rather than setting the field yourself. We use that field to represent status that's kept elsewhere and setting the flag doesn't necessarily catch our attention.
Ok I didn't know that. 
Sorry about that.
We took an ANGLE update which reverted this fix, since it wasn't upstreamed.

We'll need to reland.
Flags: needinfo?(jgilbert)
Resolution: FIXED → ---
I'll get this upstreamed.
Flags: needinfo?(jgilbert)
Keywords: checkin-needed
Whiteboard: [adv-main50.1+][adv-esr45.6+] → [adv-main50.1+][adv-esr45.6+][checkin-needed-aurora][checkin-needed-beta]
Closed: 8 years ago8 years ago
Flags: needinfo?(edwin.bugs)
Resolution: --- → FIXED
Whiteboard: [adv-main50.1+][adv-esr45.6+][checkin-needed-aurora][checkin-needed-beta] → [adv-main50.1+][adv-esr45.6+]
Following up on Comment 35, this is now verified fixed on Windows 10 x64, using:

        - 51.0-build2 (20170118123726)
        - 52.0a2 (2017-01-19)
        - 53.0a1 (2017-01-19)
        - 45.7esr-build1 (20170118123525)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.