Bug 1189860 (CVE-2015-7178)

Missing bounds check causes memory-safety bug in ProgramBinary::linkAttributes

VERIFIED FIXED in Firefox 41

Status

()

Core
Graphics
VERIFIED FIXED
3 years ago
11 months ago

People

(Reporter: q1, Assigned: Kyle Fung)

Tracking

({csectype-bounds, sec-high})

39 Branch
mozilla43
Unspecified
Windows
csectype-bounds, sec-high
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox40 wontfix, firefox41+ verified, firefox42+ verified, firefox43 verified, firefox-esr3841+ verified, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.2r unaffected, b2g-master unaffected)

Details

(Whiteboard: [adv-main41+][adv-esr38.3+])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8641769 [details]
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.
Flags: sec-bounty?
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.
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.
Flags: needinfo?(kfung)
Flags: needinfo?(jmuizelaar)
OS: Unspecified → Windows
Keywords: csectype-bounds, sec-high
Created attachment 8643274 [details] [diff] [review]
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
Flags: needinfo?(jmuizelaar)
(Assignee)

Updated

3 years ago
Assignee: nobody → kfung
Flags: needinfo?(kfung)
(Assignee)

Comment 4

3 years ago
Created attachment 8643820 [details] [diff] [review]
max-vert-att.patch
Attachment #8643820 - Flags: review?(jmuizelaar)
Attachment #8643820 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 5

3 years ago
Created attachment 8643835 [details] [diff] [review]
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.
Attachment #8643820 - Attachment is obsolete: true
Attachment #8643835 - Flags: sec-approval?
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.
status-firefox40: --- → wontfix
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox41: --- → +
tracking-firefox42: --- → +
tracking-firefox-esr38: --- → ?
Whiteboard: [checkin on 8/25]
Attachment #8643835 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/51819c4396b4
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-v2.2r: --- → unaffected
status-b2g-master: --- → unaffected
status-firefox43: --- → affected
Flags: in-testsuite?
Whiteboard: [checkin on 8/25]
https://hg.mozilla.org/mozilla-central/rev/51819c4396b4

Please nominate this for Aurora/Beta/esr38 approval when you get a chance.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Flags: needinfo?(kyle_fung)
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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]:
Flags: needinfo?(kyle_fung)
Attachment #8643835 - Flags: approval-mozilla-esr38?
Attachment #8643835 - Flags: approval-mozilla-beta?
Attachment #8643835 - Flags: approval-mozilla-aurora?
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.
Attachment #8643835 - Flags: approval-mozilla-esr38?
Attachment #8643835 - Flags: approval-mozilla-esr38+
Attachment #8643835 - Flags: approval-mozilla-beta?
Attachment #8643835 - Flags: approval-mozilla-beta+
Attachment #8643835 - Flags: approval-mozilla-aurora?
Attachment #8643835 - Flags: approval-mozilla-aurora+
Flags: sec-bounty? → sec-bounty+

Updated

3 years ago
Group: core-security → core-security-release
Flags: qe-verify+
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?
Flags: qe-verify+ → needinfo?(kyle_fung)
(Assignee)

Comment 15

3 years ago
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.
Flags: needinfo?(kyle_fung)
(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.
Status: RESOLVED → VERIFIED
status-firefox41: fixed → verified
status-firefox42: fixed → verified
status-firefox43: fixed → verified
Flags: qe-verify+
tracking-firefox-esr38: ? → 41+
Andrei, can you CC me on bug 1203135?
Flags: needinfo?(andrei.vaida)
(In reply to Milan Sreckovic [:milan] from comment #17)
> Andrei, can you CC me on bug 1203135?

Done.
Flags: needinfo?(andrei.vaida)
(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.
status-firefox-esr38: fixed → verified
Flags: qe-verify+
Whiteboard: [adv-main41+][adv-esr38.3+]
Alias: CVE-2015-7178
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.