1189860 - (CVE-2015-7178) Missing bounds check causes memory-safety bug in ProgramBinary::linkAttributes

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[q1]

Attachment: HTML and JS files. Extract and use as in note [1].

ProgramBinary::linkAttributes (gfx\angle\src\libGLESv2\ProgramBinary.cpp) does not bounds-check accesses to its mShaderAttributes array. Because this is a fixed-length array (gfx\angle\src\libGLESv2\renderer\ProgramImpl.h):

122:    sh::Attribute mShaderAttributes[gl::MAX_VERTEX_ATTRIBS];

where

17:     MAX_VERTEX_ATTRIBS = 16,

(gfx\angle\src\libGLESv2\Constants.h),

and because of the absence of bounds checks elsewhere, it is possible to create more shader attributes than mShaderAttributes can accommodate. This causes ProgramBinary::linkAttributes to write data beyond the end of mShaderAttributes (and indeed beyond the end of *mProgram) into whatever other heap object lives there.

Attached is a proof-of-concept WebGL program [1] that causes this overwriting.

The best way to observe it is to attach a debugger to FF debug 39.0, set a breakpoint on ProgramBinary::linkAttributes:

683:        mProgram->getShaderAttributes()[attributeIndex] = attribute;

and observe how attributesIndex gets incremented beyond the end of mShaderAttributes. You can then open a memory window to ((char *) &mProgram->mShaderAttributes) + sizeof (mProgram->mShaderAttributes) and step into the assignment. In my limited testing, the memory beyond the end of mShaderAttributes contains data that makes line 50 of ShaderVariable::operator= (gfx\angle\src\compiler\translator\ShaderVars.cpp) fail with a write-to-nullptr:

45: ShaderVariable &ShaderVariable::operator=(const ShaderVariable &other)
46: {
47:     type = other.type;
48:     precision = other.precision;
49:     name = other.name;
50:     mappedName = other.mappedName;
51:     arraySize = other.arraySize;
52:     staticUse = other.staticUse;
53:     fields = other.fields;
54:     structName = other.structName;
55:     return *this;
56: }

after lines 47-49 have written 5 or so DWORDs into the unowned memory. However, the unowned memory probably can contain anything at all, and if the memory occupied by, e.g., mappedName were to contain a valid pointer to heap memory, the assignment on line 50 would free it, causing havoc down the road.

The proof-of-concept also crashes 40.0b8 Win64 release, in which it writes about 10 DWORDs of data into the unowned memory before crashing. I have not yet examined whether it frees any unowned memory before crashing.

[1] See attached HTML and JS; extract and name the JS "poc.js" and put it in the same folder as the extracted HTML, then load the HTML.

Comment 1

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

So, ProgramBinary::linkAttributes is certainly assuming that number of active vertex attributes is not larger than the maximum for the vertex attributes.  Elsewhere in that function we watch for that type of overflow, but too late, and only when the location is specified.  The obvious thing is to just watch for the overflow when assigning to mProgram->getShaderAttributes()[attributeIndex], the other is to make mShaderAttributes a std::vector instead and keep growing if necessary.  There is probably a third.

Comment 2

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

Jeff, any suggestions which of the possible approaches to take, Kyle, can you then implement it?  The code around this area may have changed, but I don't think the underlying problem is gone.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Check for max

I'd do something like this patch which is similar to what upstream does:
https://github.com/google/angle/blob/master/src/libANGLE/Program.cpp#L1295

Comment 4

[Kyle Fung]

Attachment: max-vert-att.patch

Comment 5

[Kyle Fung]

Attachment: max-vert-att.patch

Changed commit message.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Someone could write WebGL code that purposely goes over the maximum attribute count and start writing to unowned memory.

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

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

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?
Not difficult.

How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause regressions, and will not require testing.

Comment 6

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

I'll give sec-approval+ for checkin (without any tests) on August 25 or later. 

Once it is in trunk, we'll want Aurora, Beta, and ESR38 patches for this checked in as well so if this doesn't apply to those branches cleanly, you may need to prepare and nominate patches before then.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51819c4396b4

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51819c4396b4

Please nominate this for Aurora/Beta/esr38 approval when you get a chance.

Comment 9

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

Comment on attachment 8643835 [details] [diff] [review]
max-vert-att.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible attack vector.
Fix Landed on Version: 43.
Risk to taking this patch (and alternatives if risky): Low; we are just refusing out of bounds scenario.
String or UUID changes made by this patch: None.



Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 10

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

Comment on attachment 8643835 [details] [diff] [review]
max-vert-att.patch

Since this is a sec bug fix, let's uplift to all nom'd branches.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34830dacfbdc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/45ad2e46fb41

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/3e3578cc78e1

Comment 14

[Andrei Vaida [:avaida]]

Reproduced the bug with an affected build: 41.0a1 (2015-05-06), using the instructions from Comment 0. Both e10s and non-e10s were crashing.

This is verified fixed on:
 • 41.0b8 (20150907144446)
 • 42.0a2 (2015-09-07), e10s and non-e10s
 • 43.0a1 (2015-09-07), e10s and non-e10s
using Windows 7 x64 and Mac OS X 10.10.4. A "Could not initialise shaders" error is now displayed and Firefox is no longer crashing.

Unfortunately, the same test case is still crashing 41.0b8, 42.0a2 (2015-09-07, e10s&non-e10s) and 43.0a1 (2015-09-07, e10s&non-e10s) on Ubuntu 14.04 x86. 

Kyle, what's your take on this?

Comment 15

[Kyle Fung]

Well the initial problem was a problem with ANGLE, so the fix and the problem itself only applies to Windows. I'm guessing that this points toward another problem that already existed with our use of the OpenGL implementations on Linux.

I think a possible solution to this would be to add a similar bounds check inside the WebGL implementation so that we don't need to rely on the underlying OpenGL implementation to check for us; that is, assuming that the problem is the same as was in Windows.

Comment 16

[Andrei Vaida [:avaida]]

(In reply to Kyle Fung from comment #15)
> Well the initial problem was a problem with ANGLE, so the fix and the
> problem itself only applies to Windows. I'm guessing that this points toward
> another problem that already existed with our use of the OpenGL
> implementations on Linux.
> 
> I think a possible solution to this would be to add a similar bounds check
> inside the WebGL implementation so that we don't need to rely on the
> underlying OpenGL implementation to check for us; that is, assuming that the
> problem is the same as was in Windows.

I've filed a separate issue for Linux - Bug 1203135. Marking this one as verified, based on Comment 14 and Comment 15. Setting back qe-verify+, as a reminder to follow up with my results from esr38.

Comment 17

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

Andrei, can you CC me on bug 1203135?

Comment 18

[Andrei Vaida [:avaida]]

(In reply to Milan Sreckovic [:milan] from comment #17)
> Andrei, can you CC me on bug 1203135?

Done.

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Andrei Vaida, QA [:avaida] from comment #16)
> Setting back qe-verify+, as a reminder to follow up with my results from esr38.

I've confirmed the fix on 38.2.1esrpre (latest tinderbox, 20150910030034) as well, using Windows 10 x64 and Mac OS X 10.10.4.