1301381 - (CVE-2016-9897) Memory corruption @ libGLESv2!rx::VaryingPacking::isFree

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Aral Yaman]

Attachment: 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

[Aral Yaman]

Attachment: windbg.txt

Comment 2

[:Gijs (he/him)]

:jgilbert, am I right in thinking this is something you would know more about?

Comment 3

[Andrew McCreight [:mccr8]]

There's also a ton of intermittent jemalloc heap corruption crashes on treeherder right now (see the blocks blocking bug 1301453).

Comment 4

[Daniel Veditz [:dveditz]]

IIRC we get this library from Google. Is Chrome affected? Is our version outdated?

Comment 5

[Aral Yaman]

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

[Andrew McCreight [:mccr8]]

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

[Milan Sreckovic [:milan] (needinfo for best results)]

(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

[Milan Sreckovic [:milan] (needinfo for best results)]

We trigger this assertion in VaryingPacking::isFree - ASSERT(registerRow + row < mRegisterMap.size());

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

(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

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1301381.patch

Comment 14

[Kelsey Gilbert [: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.

Comment 15

[Aral Yaman]

Is this patch going to be in the next release ?

Comment 16

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

(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

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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

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

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

sec-approval+ for trunk. We should backport to other affected versions as well.

Comment 19

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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

Comment 20

[Phil Ringnalda (:philor)]

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

Comment 21

[Gerry Chang [:gchang]]

Hi :edwin,
could you please nominate this uplift to Beta51 and Aurora52?

Comment 22

[Edwin Flores [inactive from 2016-12-01] [:eflores] [: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.

Comment 23

[Gerry Chang [:gchang]]

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

Fix a sec-high. Beta51+ and Aurora52+. Should be in 51 beta 2.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80688aa32ba5
https://hg.mozilla.org/releases/mozilla-beta/rev/231288ac4490

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

See comment 22.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/762fa3127658

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]:

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/2e5438a92617

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Sec-high, meets the triage bar for inclusion in 50.1.0

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/a52f1b7f4627

Comment 33

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

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

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

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

[Andrei Vaida [:avaida]]

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

[Daniel Veditz [:dveditz]]

Aral: in the future please email [e-mail] 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

[Aral Yaman]

Ok I didn't know that. 
Sorry about that.

Comment 38

[Kelsey Gilbert [:jgilbert]]

We took an ANGLE update which reverted this fix, since it wasn't upstreamed.

We'll need to reland.

Comment 39

[Kelsey Gilbert [:jgilbert]]

I'll get this upstreamed.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53b1b7c121a7

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[Andrei Vaida [:avaida]]

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)

Comment 43

[Kelsey Gilbert [:jgilbert]]

Fixed upstream:
https://github.com/google/angle/commit/2ad1c4905e80502362a53844278936dd0bb25704