Bug 1301381 (CVE-2016-9897)

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

VERIFIED FIXED in Firefox -esr45

Status

()

Core
Canvas: WebGL
VERIFIED FIXED
a year ago
5 months ago

People

(Reporter: Aral Yaman, Assigned: eflores)

Tracking

({csectype-bounds, sec-high})

51 Branch
mozilla53
Unspecified
Windows
csectype-bounds, sec-high
Points:
---
Bug Flags:
sec-bounty +
qe-verify +

Firefox Tracking Flags

(firefox-esr4550+ verified, firefox50+ verified, firefox51+ verified, firefox52+ verified, firefox53+ verified)

Details

(Whiteboard: [adv-main50.1+][adv-esr45.6+])

Attachments

(3 attachments)

(Reporter)

Description

a year ago
strtestcase
Created attachment 8789367 [details]
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.
(Reporter)

Comment 1

a year ago
Created attachment 8789368 [details]
windbg.txt

Comment 2

a year ago
: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
(Reporter)

Comment 5

a year ago
libGLES is a part from the angle project from Google -> https://github.com/google/angle 

I was not able to crash chrome.

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

https://github.com/google/angle/blob/bc4c4bc5f6659f3dc815b6b1d0c68ad8ab80eea2/src/libANGLE/renderer/d3d/VaryingPacking.cpp

https://hg.mozilla.org/mozilla-unified/annotate/91c2b9d5c135/gfx/angle/src/libANGLE/renderer/d3d/VaryingPacking.cpp#l185

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

I also commited a crash report :
https://crash-stats.mozilla.com/report/index/531464b5-7dae-42d2-b7b9-2e7392160908
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.

Updated

11 months ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: csectype-bounds, sec-high
Status: NEW → ASSIGNED
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].

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/libANGLE/renderer/d3d/VaryingPacking.cpp#74
(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.

Updated

10 months ago
status-firefox51: --- → affected
status-firefox52: --- → affected
tracking-firefox51: --- → +
tracking-firefox52: --- → +
Created attachment 8803840 [details] [diff] [review]
1301381.patch
Attachment #8803840 - Flags: review?(jmuizelaar)
Attachment #8803840 - Flags: review?(jmuizelaar) → review?(jgilbert)
Comment on attachment 8803840 [details] [diff] [review]
1301381.patch

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

This is fine, and we should upstream this.
Attachment #8803840 - Flags: review?(jgilbert) → review+
(Reporter)

Comment 15

9 months ago
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.
status-firefox50: --- → wontfix
status-firefox-esr45: --- → ?
Comment on attachment 8803840 [details] [diff] [review]
1301381.patch

[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?
No.

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

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]
1301381.patch

sec-approval+ for trunk. We should backport to other affected versions as well.
Attachment #8803840 - Flags: sec-approval? → sec-approval+
status-firefox53: --- → affected
status-firefox-esr45: ? → affected
tracking-firefox53: --- → +
tracking-firefox-esr45: --- → 51+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a72de75573c8
https://hg.mozilla.org/mozilla-central/rev/a72de75573c8
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox53: affected → fixed
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]
1301381.patch

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]
1301381.patch

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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/80688aa32ba5
https://hg.mozilla.org/releases/mozilla-beta/rev/231288ac4490
status-firefox51: affected → fixed
status-firefox52: affected → fixed
Comment on attachment 8803840 [details] [diff] [review]
1301381.patch

See comment 22.
Attachment #8803840 - Flags: approval-mozilla-esr45?
https://hg.mozilla.org/releases/mozilla-esr45/rev/762fa3127658
status-firefox-esr45: affected → fixed

Updated

9 months ago
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.

https://hg.mozilla.org/releases/mozilla-esr45/rev/f2792b829193
status-firefox-esr45: fixed → affected
[Tracking Requested - why for this release]:
status-firefox50: wontfix → ?
tracking-firefox50: --- → ?
tracking-firefox-esr45: 51+ → ?
Comment on attachment 8803840 [details] [diff] [review]
1301381.patch

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?
status-firefox50: ? → affected
https://hg.mozilla.org/releases/mozilla-esr45/rev/2e5438a92617
status-firefox-esr45: affected → fixed

Updated

9 months ago
tracking-firefox50: ? → +
tracking-firefox-esr45: ? → 51+

Comment 31

9 months ago
Comment on attachment 8803840 [details] [diff] [review]
1301381.patch

Sec-high, meets the triage bar for inclusion in 50.1.0
Attachment #8803840 - Flags: approval-mozilla-release? → approval-mozilla-release+
https://hg.mozilla.org/releases/mozilla-release/rev/a52f1b7f4627
status-firefox50: affected → fixed
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
(Reporter)

Updated

8 months ago
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?
status-firefox50: fixed → verified
status-firefox51: fixed → verified
status-firefox-esr45: fixed → verified
Flags: needinfo?(jgilbert)
Flags: needinfo?(edwin.bugs)
Flags: sec-bounty? → sec-bounty+
Aral: in the future please email security@mozilla.org 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.
(Reporter)

Comment 37

8 months ago
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.
Status: RESOLVED → REOPENED
status-firefox51: verified → affected
status-firefox52: fixed → affected
status-firefox53: fixed → affected
Flags: needinfo?(jgilbert)
Resolution: FIXED → ---
Keywords: checkin-needed
I'll get this upstreamed.
Flags: needinfo?(jgilbert)
https://hg.mozilla.org/integration/mozilla-inbound/rev/53b1b7c121a7
Keywords: checkin-needed
Whiteboard: [adv-main50.1+][adv-esr45.6+] → [adv-main50.1+][adv-esr45.6+][checkin-needed-aurora][checkin-needed-beta]
https://hg.mozilla.org/mozilla-central/rev/53b1b7c121a7
https://hg.mozilla.org/releases/mozilla-aurora/rev/9a9493b68208
https://hg.mozilla.org/releases/mozilla-beta/rev/ebdc66fc1d6f
Status: REOPENED → RESOLVED
Last Resolved: 9 months ago8 months ago
status-firefox51: affected → fixed
status-firefox52: affected → fixed
status-firefox53: affected → fixed
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+]
tracking-firefox-esr45: 51+ → 50+
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)
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
status-firefox52: fixed → verified
status-firefox53: fixed → verified
Group: core-security-release
Fixed upstream:
https://github.com/google/angle/commit/2ad1c4905e80502362a53844278936dd0bb25704
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.