Bug 1248580 (CVE-2016-2824)

Crash in TSymbolTableLevel::~TSymbolTableLevel

VERIFIED FIXED in Firefox 47

Status

()

defect
--
critical
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: aral.yaman, Assigned: jerry)

Tracking

(4 keywords)

44 Branch
mozilla49
All
Windows
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox44 wontfix, firefox45 wontfix, firefox46 wontfix, firefox47+ verified, firefox48+ verified, firefox49+ verified, firefox-esr38 unaffected, firefox-esr4547+ verified)

Details

(Whiteboard: [adv-main47+][adv-esr45.2+] gfx-noted, crash signature)

Attachments

(3 attachments, 4 obsolete attachments)

2.24 KB, text/html
Details
5.80 KB, patch
Details | Diff | Splinter Review
1.21 KB, patch
Details | Diff | Splinter Review
Posted file crash.html
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160215030213

Steps to reproduce:

Open crash.html after a few seconds the browser will crash.

The reason for the crash is this line:
var kernelLocation = gl.getUniformLocation(program, "u_kernel[1]"); 

no crash with var kernelLocation = gl.getUniformLocation(program, "u_kernel[0]")

I submitted a crash report as well:
https://crash-stats.mozilla.com/report/index/bp-b8689516-af85-4a0d-87cb-998d92160216




Actual results:

Firefox crashed after 2-6 seconds


Expected results:

No Crash
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ TSymbolTableLevel::~TSymbolTableLevel ]
Component: Untriaged → Canvas: WebGL
Ever confirmed: true
Keywords: crash, testcase
Product: Firefox → Core
0 	xul.dll 	TSymbolTableLevel::~TSymbolTableLevel() 	gfx/angle/src/compiler/translator/SymbolTable.cpp
1 	xul.dll 	TSymbolTable::pop() 	gfx/angle/src/compiler/translator/SymbolTable.h
2 	xul.dll 	TSymbolTable::~TSymbolTable() 	gfx/angle/src/compiler/translator/SymbolTable.cpp
3 	xul.dll 	TCompiler::~TCompiler() 	gfx/angle/src/compiler/translator/Compiler.cpp
4 	xul.dll 	TranslatorESSL::`scalar deleting destructor'(unsigned int) 	
5 	xul.dll 	ShDestruct(void*) 	gfx/angle/src/compiler/translator/ShaderLang.cpp
6 	xul.dll 	mozilla::UniquePtr<mozilla::webgl::ShaderValidator, mozilla::DefaultDelete<mozilla::webgl::ShaderValidator> >::reset(mozilla::webgl::ShaderValidator*) 	mfbt/UniquePtr.h
7 	xul.dll 	mozilla::WebGLShader::~WebGLShader() 	dom/canvas/WebGLShader.cpp
8 	xul.dll 	mozilla::WebGLShader::cycleCollection::DeleteCycleCollectable(void*) 	dom/canvas/WebGLShader.h
9 	xul.dll 	SnowWhiteKiller::~SnowWhiteKiller() 	xpcom/base/nsCycleCollector.cpp
10 	xul.dll 	nsCycleCollector::FreeSnowWhite(bool) 	xpcom/base/nsCycleCollector.cpp
11 	xul.dll 	nsCycleCollector_doDeferredDeletion() 	xpcom/base/nsCycleCollector.cpp
12 	xul.dll 	AsyncFreeSnowWhite::Run() 	js/xpconnect/src/XPCJSRuntime.cpp
13 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
14 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
15 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
16 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
17 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
18 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp
19 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp
20 	xul.dll 	XRE_RunAppShell 	toolkit/xre/nsEmbedFunctions.cpp
21 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
22 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
23 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
24 	xul.dll 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp
25 	plugin-container.exe 	content_process_main(int, char** const) 	ipc/contentproc/plugin-container.cpp
26 	plugin-container.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
27 	plugin-container.exe 	__tmainCRTStartup 	f:/dd/vctools/crt/crtw32/startup/crt0.c:255
28 	kernel32.dll 	BaseThreadInitThunk 	
29 	ntdll.dll 	__RtlUserThreadStart 	
30 	ntdll.dll 	_RtlUserThreadStart
Summary: Read Access Violation in WebGL causes a crash → Crash in TSymbolTableLevel::~TSymbolTableLevel
I am unable to reproduce this locally and it appears from stats that this crash does not occur in the wild. Can you please find a regression window either using mozregression or manually?
Flags: needinfo?(aral.yaman)
Whiteboard: gfx-noted
Flags: needinfo?(aral.yaman)
OS: Unspecified → Windows 10
This was a regression from the ANGLE update in bug 1211774.
Blocks: 1211774
Flags: needinfo?(jmuizelaar)
OS: Windows 10 → Windows
Hardware: Unspecified → All
Doesn't crash Chrome 48, so I guess this is fixed in whatever version of ANGLE they're using.
After researching in the internet I found a similar issue in Chrome:

https://code.google.com/p/chromium/issues/detail?id=145525

Maybe this can help to solve the problem...
Depends on: 1251375
Flags: needinfo?(jmuizelaar)
This still crashes on today's nightly, which I've confirmed includes the ANGLE update from bug 1251375. I generally need to refresh the POC a few times to reproduce.
https://crash-stats.mozilla.com/report/index/18a51626-b02a-411c-b192-9c1fb2160316
Flags: needinfo?(jmuizelaar)
Assignee: nobody → jmuizelaar
Is this bug fixed ?
Aral, I don't think so - does it still crash for you?
It still crashes with the latest nightly.
Flags: needinfo?(howareyou322)
I can reproduce at win10 with today's m-c to.
Then, I will try to remove the ANGLE update from bug 1251375 and test again.
I fond that we might have memory corruption problem with this test page. Let me check this again.
Assignee: jmuizelaar → hshih
Status: NEW → ASSIGNED
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(howareyou322)
At Program::setUniformInternal(), we have memory copy with the input size. There is no checking inside angle lib. So we should pass the correct size in our webgl implementation.
https://hg.mozilla.org/mozilla-central/annotate/d0be57e84807ce0853b2406de7ff6abb195ac898/gfx/angle/src/libANGLE/Program.cpp#l2464
Is this not a security critical bug ?
Comment on attachment 8753226 [details] [diff] [review]
strip the uploading element num according to the uniform array size. v1

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

::: dom/canvas/WebGLContextValidate.cpp
@@ +495,5 @@
>      if (!loc->ValidateArrayLength(setterElemSize, setterArraySize, this, funcName))
>          return false;
>  
> +    MOZ_ASSERT(loc->mArrayIndex < loc->mActiveInfo->mElemCount);
> +    uint32_t uniformElemCount = loc->mActiveInfo->mElemCount - loc->mArrayIndex - 1;

`size_t`.
Why is this -1?

::: dom/canvas/WebGLProgram.cpp
@@ +811,5 @@
>      if (loc == -1)
>          return nullptr;
>  
> +    RefPtr<WebGLUniformLocation> locObj =
> +        new WebGLUniformLocation(mContext, LinkInfo(), loc, arrayIndex, activeInfo);

Overflow it like before.

::: dom/canvas/WebGLUniformLocation.h
@@ +40,5 @@
>      }
>  
>      const WeakPtr<const webgl::LinkedProgramInfo> mLinkInfo;
>      const GLuint mLoc;
> +    const uint32_t mArrayIndex;

`size_t` here and elsewhere
Attachment #8753226 - Flags: review?(jgilbert) → review-
Group: core-security
This is an out-of-bounds write that seems to crash us in something unrelated, which sounds like stack corruption. At the least it's modifying code execution in an unusual way.

However, the possible size of the overwrite is bounded and small, so I don't think it provides a practical attack surface.

I propose sec-moderate.

Do you agree, :jrmuizel?


It claims to be crashing here:
https://hg.mozilla.org/mozilla-central/annotate/e355cacefc88/gfx/angle/src/compiler/translator/SymbolTable.cpp#l50

My guess is that the size of `*destPointer` from `Program::setUniformInternal` puts it in the same jemalloc arena as TSymbolTableLevel, so overwrites cause mutations of the pointers in TSymbolTableLevel's `map`.


We also need to audit related code, particularly GetUniform.
No longer depends on: 1251375
Flags: needinfo?(jmuizelaar)
Keywords: sec-moderate
Hi Jeff G,
I think we also need to update the setter for matrix array.

https://hg.mozilla.org/mozilla-central/annotate/a884b96685aa13b65601feddb24e5f85ba861561/dom/canvas/WebGLContextValidate.cpp#l533

 I will create another bug for this.
update for comment 15.
Also fix the problem that just pass the array name for gl.getUniformLocation().
Attachment #8753658 - Flags: review?(jgilbert)
Attachment #8753226 - Attachment is obsolete: true
Blocks: 1273768
Group: core-security → gfx-core-security
Duplicate of this bug: 1273768
Comment on attachment 8753658 [details] [diff] [review]
strip the uploading element num according to the uniform array size. v2

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

This patch is good, but I want to handle all instances of this same code bug in this bugzilla bug. (I duped the spun-off bug back to this bug. The patch for that bug which should be reviewed here instead)

::: dom/canvas/WebGLContextValidate.cpp
@@ +494,5 @@
>  
>      if (!loc->ValidateArrayLength(setterElemSize, setterArraySize, this, funcName))
>          return false;
>  
> +    MOZ_ASSERT(loc->mArrayIndex < loc->mActiveInfo->mElemCount);

It's easier to follow this if you order them like on the line below, as:
MOZ_ASSERT(loc->mActiveInfo->mElemCount > loc->mArrayIndex);
Attachment #8753658 - Flags: review?(jgilbert) → review+
Attachment #8753658 - Attachment is obsolete: true
handle matrix case.
Attachment #8754234 - Flags: review?(jgilbert)
I think sec-high is probably more appropriate. It may not be easy to weaponize, but it's hard to promise that a particular OOB write wont break things that will allow a further escalation down the road.
Flags: needinfo?(jmuizelaar)
Attachment #8754234 - Flags: review?(jgilbert) → review+
Please land
attachment 8754233 [details] [diff] [review]
attachment 8754234 [details] [diff] [review]
to m-c.

I don't push to try since this is a security bug. I just test locally, and I don't see the crash with the crash.html in comment 0.
Keywords: checkin-needed
Version: 47 Branch → 44 Branch
(In reply to Jeff Gilbert [:jgilbert] from comment #16)
> However, the possible size of the overwrite is bounded and small, so I don't
> think it provides a practical attack surface.
> 
> I propose sec-moderate.

Unfortunately that's all you really need: we've seen exploits based on single-byte writes.
Keywords: sec-moderatesec-high
Can you ask for sec approval before you land this? 
Tracking for 47+ since this is now sec-high.
Flags: needinfo?(hshih)
Comment on attachment 8754233 [details] [diff] [review]
P1: strip the uploading element num according to the uniform array size. v3. r=jgilbert

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
=> User can write a shader and do some oob memory writing easily. Just like the test attachment 8719759 [details]

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
=> Probably. There are some keyword like "strip num" or "uploading".

Which older supported branches are affected by this flaw?
=> I think the nightly, aurora, beta and release branch all have this problem.
http://mxr.mozilla.org/mozilla-aurora/source/dom/canvas/WebGLContextValidate.cpp#478

http://mxr.mozilla.org/mozilla-beta/source/dom/canvas/WebGLContextValidate.cpp#478

http://mxr.mozilla.org/mozilla-release/source/dom/canvas/WebGLContextValidate.cpp#478

If not all supported branches, which bug introduced the flaw?
=> all supported branches have this problem.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
=> not yet. I think it's easy to do for all branches.

How likely is this patch to cause regressions; how much testing does it need?
=> This patch only strip the uploading size. If it cause regressions, we should check the regressions is a side-effect or an undefined behavior.
Flags: needinfo?(hshih)
Attachment #8754233 - Flags: sec-approval?
Comment on attachment 8754234 [details] [diff] [review]
P2: handle gl.UniformMatrixXfv() function uploading element size. v1

[Security approval request comment]

as comment 27
Attachment #8754234 - Flags: sec-approval?
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #24)
> Please land
> attachment 8754233 [details] [diff] [review]
> attachment 8754234 [details] [diff] [review]
> to m-c.
> 
> I don't push to try since this is a security bug. I just test locally, and I
> don't see the crash with the crash.html in comment 0.

need to wait on sec-approval before landing, so removing keyword
Keywords: checkin-needed
Is this too late to get into 47?
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
(In reply to Al Billings [:abillings] from comment #30)
> Is this too late to get into 47?

I think we can take it. I hope this lands before I gtb 47.0b9 on Thursday.
Flags: needinfo?(rkothari)
Comment on attachment 8754234 [details] [diff] [review]
P2: handle gl.UniformMatrixXfv() function uploading element size. v1

sec-approval+.

We'll want Aurora, Beta, and ESR45 patches made and nominated ASAP (within 24 hours if possible) in order to get this into 47.
Attachment #8754234 - Flags: sec-approval? → sec-approval+
Attachment #8754233 - Flags: sec-approval? → sec-approval+
Attachment #8754233 - Attachment is obsolete: true
Attachment #8754234 - Attachment is obsolete: true
Flags: needinfo?(hshih)
https://hg.mozilla.org/mozilla-central/rev/7e26c1d13046
https://hg.mozilla.org/mozilla-central/rev/b052acfa6f28
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
In comment 32, are we need to fix this bug for Aurora, Beta, and ESR45?
How to do that? Request uplift and sec-approval for each branch?
Flags: needinfo?(abillings)
sec-approval only applies to landing on trunk. Follow the usual uplift request procedures for Aurora/Beta/ESR45.
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #39)
> In comment 32, are we need to fix this bug for Aurora, Beta, and ESR45?
> How to do that? Request uplift and sec-approval for each branch?

You need to request branch approval for an appropriate patch. As Ryan says, sec-approval is just for trunk.
Flags: needinfo?(abillings)
Comment on attachment 8756367 [details] [diff] [review]
P1: strip the uploading element num according to the uniform array size. v4. r=jgilbert. sec-approval=abillings.

Approval Request Comment
[Feature/regressing bug #]:
Bug 1248580
Crash with attachment 8719759 [details] test page.

[User impact if declined]:
Out of boundary memory writing problem from some webgl shader.

[Describe test coverage new/current, TreeHerder]:
Already in m-c

[Risks and why]: 
Low.
This patch only strip the uploading size with the destination array size.

[String/UUID change made/needed]:
None
Attachment #8756367 - Flags: approval-mozilla-aurora?
Comment on attachment 8756369 [details] [diff] [review]
P2: handle gl.UniformMatrixXfv() function uploading element size. v2. r=jgilbert sec-approval=abillings.

Approval Request Comment

as comment 42.
Attachment #8756369 - Flags: approval-mozilla-aurora?
Comment on attachment 8756367 [details] [diff] [review]
P1: strip the uploading element num according to the uniform array size. v4. r=jgilbert. sec-approval=abillings.

Approval Request Comment
[Feature/regressing bug #]:
Bug 1248580
Crash with attachment 8719759 [details] test page.

[User impact if declined]:
Out of boundary memory writing problem from some webgl shader.

[Describe test coverage new/current, TreeHerder]:
Already in m-c

[Risks and why]: 
Low.
This patch only strip the uploading size with the destination array size.

[String/UUID change made/needed]:
None
Attachment #8756367 - Flags: approval-mozilla-beta?
Comment on attachment 8756369 [details] [diff] [review]
P2: handle gl.UniformMatrixXfv() function uploading element size. v2. r=jgilbert sec-approval=abillings.

Approval Request Comment

as comment 44.
Attachment #8756369 - Flags: approval-mozilla-beta?
Comment on attachment 8756367 [details] [diff] [review]
P1: strip the uploading element num according to the uniform array size. v4. r=jgilbert. sec-approval=abillings.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
There is an out of boundary memory writing problem using attachment 8719759 [details] test page.

User impact if declined: 
Out of boundary memory writing problem from some webgl shader.

Fix Landed on Version:
firefox-49

Risk to taking this patch (and alternatives if risky): 
Low.
This patch only strip the uploading size with the destination array size.

String or UUID changes made by this patch: 
none.
Attachment #8756367 - Flags: approval-mozilla-esr45?
Comment on attachment 8756369 [details] [diff] [review]
P2: handle gl.UniformMatrixXfv() function uploading element size. v2. r=jgilbert sec-approval=abillings.

[Approval Request Comment]
as comment 46
Attachment #8756369 - Flags: approval-mozilla-esr45?
Comment on attachment 8756367 [details] [diff] [review]
P1: strip the uploading element num according to the uniform array size. v4. r=jgilbert. sec-approval=abillings.

Sec-high issue, Aurora48+, Beta47+, ESR45+
Attachment #8756367 - Flags: approval-mozilla-esr45?
Attachment #8756367 - Flags: approval-mozilla-esr45+
Attachment #8756367 - Flags: approval-mozilla-beta?
Attachment #8756367 - Flags: approval-mozilla-beta+
Attachment #8756367 - Flags: approval-mozilla-aurora?
Attachment #8756367 - Flags: approval-mozilla-aurora+
Attachment #8756369 - Flags: approval-mozilla-esr45?
Attachment #8756369 - Flags: approval-mozilla-esr45+
Attachment #8756369 - Flags: approval-mozilla-beta?
Attachment #8756369 - Flags: approval-mozilla-beta+
Attachment #8756369 - Flags: approval-mozilla-aurora?
Attachment #8756369 - Flags: approval-mozilla-aurora+
Flags: needinfo?(lhenry)
Group: gfx-core-security → core-security-release
Flags: qe-verify+
Reproduced a series of crashes on old Nightly from (2016-02-16):
bp-09ad3035-6637-40bd-a9c3-0f1ed2160527
bp-ffb5b0e7-9a70-49c5-9957-bc8cc2160527
bp-270b2e2a-1d45-4697-9746-d1b9f2160527
bp-de04e624-556c-46c9-b4a4-af1dd2160527
bp-a50a07d9-aedd-430b-a4e5-450632160527
bp-90088e7b-7d6a-4d50-9630-f1c7b2160527
bp-55133d1a-59d9-4539-9ebb-58ee12160527

Verified that no crashes are registered using Firefox 47 beta 9, latest Developer Edition 48.0a2 and latest Nightly 49.0a1 on multiple Windows versions 7, 8.1 and 10.
Whiteboard: gfx-noted → [adv-main47+][adv-esr45.2+] gfx-noted
Alias: CVE-2016-2824
Also verified that the crash does not reproduce using Firefox 45.2.0ESR as well on Windows 7, 8.1 and 10.
On windows 7 I did receive a crash though but only once and it's [@ OOM | small] crash:
https://crash-stats.mozilla.com/report/index/2b2cb2e4-4db8-4715-ad27-c3b402160606
Status: RESOLVED → VERIFIED
Is this bug not supposed to get a bug bounty ? 
Because I didn't get any information about that...
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.