Last Comment Bug 1301381 - (CVE-2016-9897) Memory corruption @ libGLESv2!rx::VaryingPacking::isFree
(CVE-2016-9897)
: Memory corruption @ libGLESv2!rx::VaryingPacking::isFree
Status: VERIFIED FIXED
[adv-main50.1+][adv-esr45.6+]
: csectype-bounds, sec-high
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: 51 Branch
: Unspecified Windows
-- normal (vote)
: mozilla53
Assigned To: Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-08 05:58 PDT by Aral
Modified: 2017-02-09 08:03 PST (History)
16 users (show)
jgilbert: needinfo? (jgilbert)
dveditz: sec‑bounty+
andrei.vaida: qe‑verify+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified
+
verified
50+
verified


Attachments
crash.html (1.19 KB, text/html)
2016-09-08 05:58 PDT, Aral
no flags Details
windbg.txt (5.88 KB, text/plain)
2016-09-08 05:59 PDT, Aral
no flags Details
1301381.patch (1.31 KB, patch)
2016-10-24 05:26 PDT, Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]
jgilbert: review+
gchang: approval‑mozilla‑aurora+
gchang: approval‑mozilla‑beta+
rkothari: approval‑mozilla‑release+
gchang: approval‑mozilla‑esr45+
abillings: sec‑approval+
Details | Diff | Splinter Review

Description User image Aral 2016-09-08 05:58:50 PDT
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.
Comment 1 User image Aral 2016-09-08 05:59:17 PDT
Created attachment 8789368 [details]
windbg.txt
Comment 2 User image :Gijs 2016-09-08 06:06:00 PDT
:jgilbert, am I right in thinking this is something you would know more about?
Comment 3 User image Andrew McCreight [:mccr8] 2016-09-08 10:07:32 PDT
There's also a ton of intermittent jemalloc heap corruption crashes on treeherder right now (see the blocks blocking bug 1301453).
Comment 4 User image Daniel Veditz [:dveditz] 2016-09-08 10:25:18 PDT
IIRC we get this library from Google. Is Chrome affected? Is our version outdated?
Comment 5 User image Aral 2016-09-09 01:17:56 PDT
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
Comment 6 User image Andrew McCreight [:mccr8] 2016-09-22 10:59:09 PDT
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.
Comment 7 User image Milan Sreckovic [:milan] 2016-10-04 07:30:26 PDT
(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.
Comment 8 User image Milan Sreckovic [:milan] 2016-10-04 08:27:30 PDT
We trigger this assertion in VaryingPacking::isFree - ASSERT(registerRow + row < mRegisterMap.size());
Comment 9 User image Milan Sreckovic [:milan] 2016-10-04 10:26:26 PDT
(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.
Comment 10 User image Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin] 2016-10-13 05:38:28 PDT
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.
Comment 11 User image Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin] 2016-10-13 05:57:38 PDT
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
Comment 12 User image Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin] 2016-10-13 10:32:04 PDT
(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.
Comment 13 User image Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin] 2016-10-24 05:26:32 PDT
Created attachment 8803840 [details] [diff] [review]
1301381.patch
Comment 14 User image Jeff Gilbert [:jgilbert] 2016-11-07 19:47:48 PST
Comment on attachment 8803840 [details] [diff] [review]
1301381.patch

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

This is fine, and we should upstream this.
Comment 15 User image Aral 2016-11-09 21:25:17 PST
Is this patch going to be in the next release ?
Comment 16 User image Al Billings [:abillings] 2016-11-11 12:11:44 PST
(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 17 User image Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin] 2016-11-14 07:37:39 PST
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.
Comment 18 User image Al Billings [:abillings] 2016-11-15 14:55:42 PST
Comment on attachment 8803840 [details] [diff] [review]
1301381.patch

sec-approval+ for trunk. We should backport to other affected versions as well.
Comment 19 User image Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin] 2016-11-17 03:36:57 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a72de75573c8
Comment 20 User image Phil Ringnalda (:philor) 2016-11-17 19:34:48 PST
https://hg.mozilla.org/mozilla-central/rev/a72de75573c8
Comment 21 User image Gerry Chang [:gchang] 2016-11-18 00:46:34 PST
Hi :edwin,
could you please nominate this uplift to Beta51 and Aurora52?
Comment 22 User image Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin] 2016-11-18 02:59:52 PST
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.
Comment 23 User image Gerry Chang [:gchang] 2016-11-18 19:29:26 PST
Comment on attachment 8803840 [details] [diff] [review]
1301381.patch

Fix a sec-high. Beta51+ and Aurora52+. Should be in 51 beta 2.
Comment 25 User image Ryan VanderMeulen [:RyanVM] 2016-11-20 10:43:17 PST
Comment on attachment 8803840 [details] [diff] [review]
1301381.patch

See comment 22.
Comment 26 User image Ryan VanderMeulen [:RyanVM] 2016-11-20 11:14:53 PST
https://hg.mozilla.org/releases/mozilla-esr45/rev/762fa3127658
Comment 27 User image Ryan VanderMeulen [:RyanVM] 2016-11-29 18:05:45 PST
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
Comment 28 User image Ryan VanderMeulen [:RyanVM] 2016-11-30 06:02:41 PST
[Tracking Requested - why for this release]:
Comment 29 User image Ryan VanderMeulen [:RyanVM] 2016-11-30 06:03:19 PST
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.
Comment 30 User image Ryan VanderMeulen [:RyanVM] 2016-11-30 15:51:53 PST
https://hg.mozilla.org/releases/mozilla-esr45/rev/2e5438a92617
Comment 31 User image Ritu Kothari (:ritu) 2016-12-05 10:36:07 PST
Comment on attachment 8803840 [details] [diff] [review]
1301381.patch

Sec-high, meets the triage bar for inclusion in 50.1.0
Comment 32 User image Ryan VanderMeulen [:RyanVM] 2016-12-05 15:15:30 PST
https://hg.mozilla.org/releases/mozilla-release/rev/a52f1b7f4627
Comment 33 User image Al Billings [:abillings] 2016-12-07 12:18:57 PST
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.
Comment 34 User image Al Billings [:abillings] 2016-12-07 12:34:07 PST
Ok. There is a 45.6 ESR release at the same time as 50.1. The advisory for this should be in the release.
Comment 35 User image Andrei Vaida, QA [:avaida] – please ni? me 2016-12-13 09:00:25 PST
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?
Comment 36 User image Daniel Veditz [:dveditz] 2016-12-19 10:43:15 PST
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.
Comment 37 User image Aral 2016-12-19 11:23:49 PST
Ok I didn't know that. 
Sorry about that.
Comment 38 User image Jeff Gilbert [:jgilbert] 2017-01-05 03:20:18 PST
We took an ANGLE update which reverted this fix, since it wasn't upstreamed.

We'll need to reland.
Comment 39 User image Jeff Gilbert [:jgilbert] 2017-01-05 03:21:11 PST
I'll get this upstreamed.
Comment 40 User image Ryan VanderMeulen [:RyanVM] 2017-01-05 20:47:37 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/53b1b7c121a7
Comment 42 User image Andrei Vaida, QA [:avaida] – please ni? me 2017-01-20 01:34:00 PST
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)

Note You need to log in before you can comment on or make changes to this bug.