1248580 - (CVE-2016-2824) Crash in TSymbolTableLevel::~TSymbolTableLevel

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Aral Yaman]

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

Comment 1

[u279076]

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

Comment 2

[u279076]

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?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

This was a regression from the ANGLE update in bug 1211774.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Doesn't crash Chrome 48, so I guess this is fixed in whatever version of ANGLE they're using.

Comment 5

[Aral Yaman]

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...

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Aral Yaman]

Is this bug fixed ?

Comment 8

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

Aral, I don't think so - does it still crash for you?

Comment 9

[Aral Yaman]

It still crashes with the latest nightly.

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I fond that we might have memory corruption problem with this test page. Let me check this again.

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: strip the uploading element num according to the uniform array size. v1

Comment 14

[Aral Yaman]

Is this not a security critical bug ?

Comment 15

[Kelsey Gilbert [:jgilbert]]

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

Comment 16

[Kelsey Gilbert [:jgilbert]]

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.

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: strip the uploading element num according to the uniform array size. v2

update for comment 15.
Also fix the problem that just pass the array name for gl.getUniformLocation().

Comment 20

[Kelsey Gilbert [:jgilbert]]

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);

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: strip the uploading element num according to the uniform array size. v3. r=jgilbert

update for comment 20

Comment 22

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: handle gl.UniformMatrixXfv() function uploading element size. v1

handle matrix case.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 24

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 25

[Daniel Veditz [:dveditz]]

(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.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you ask for sec approval before you land this? 
Tracking for 47+ since this is now sec-high.

Comment 27

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 28

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8754234 [details] [diff] [review]
P2: handle gl.UniformMatrixXfv() function uploading element size. v1

[Security approval request comment]

as comment 27

Comment 29

[Carsten Book [:Tomcat]]

(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

Comment 30

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

Is this too late to get into 47?

Comment 31

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

(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.

Comment 32

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

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.

Comment 33

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: strip the uploading element num according to the uniform array size. v4. r=jgilbert. sec-approval=abillings.

fix build break

Comment 34

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: handle gl.UniformMatrixXfv() function uploading element size. v2. r=jgilbert sec-approval=abillings.

fix build break

Comment 35

[Carsten Book [:Tomcat]]

re-landed

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e26c1d13046
https://hg.mozilla.org/integration/mozilla-inbound/rev/b052acfa6f28

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31879b3fdce3
 https://hg.mozilla.org/integration/mozilla-inbound/rev/56c2db882b7d

Comment 37

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=28663480&repo=mozilla-inbound

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7e26c1d13046
https://hg.mozilla.org/mozilla-central/rev/b052acfa6f28

Comment 39

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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?

Comment 40

[Ryan VanderMeulen [:RyanVM]]

sec-approval only applies to landing on trunk. Follow the usual uplift request procedures for Aurora/Beta/ESR45.

Comment 41

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

(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.

Comment 42

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 43

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 44

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 45

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 46

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 47

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 48

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

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+

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ccd86521d845
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a77ab4d41c7

https://hg.mozilla.org/releases/mozilla-beta/rev/1c47a4f604c6
https://hg.mozilla.org/releases/mozilla-beta/rev/e40d0d6bd40d

https://hg.mozilla.org/releases/mozilla-esr45/rev/4b54feddf36c
https://hg.mozilla.org/releases/mozilla-esr45/rev/4c2977c3fda9

Comment 50

[Bogdan Maris, Desktop QA [Holiday+PTO 1-6 May]]

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.

Comment 51

[Bogdan Maris, Desktop QA [Holiday+PTO 1-6 May]]

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

Comment 52

[Aral Yaman]

Is this bug not supposed to get a bug bounty ? 
Because I didn't get any information about that...