1203135 - crash in link_shaders

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Andrei Vaida [:avaida]]

Attachment: attachment.txt

This bug was filed from the Socorro interface and is 
report bp-66c41d6d-d10f-459f-b03b-b7ff22150909.
=============================================================

Note: this is a follow up issue filed for Bug 1189860. Marked as security-sensitive as well, since it derived from it.

Reproducible on:
* 41.0b8 (20150907144446)
* 42.0a2 (2015-09-07) e10s & non-e10s
* 43.0a1 (2015-09-07) e10s & non-e10s

Affected platforms:
Linux-only, tested with Ubuntu 14.04 (x86) and Ubuntu 14.04 (x64)

Steps to reproduce:
1. See attached HTML and JS; extract and name the JS "poc.js" and put it in the same folder as the extracted HTML.
2. Launch Firefox and load the HTML.

Expected result:
As in the case of Windows and Mac OS X platforms, a "Could not initialise shaders" error is displayed.

Actual result:
Firefox crashes.

Additional notes:
* See Bug 1189860, Comment 15 for context.

Comment 1

[Kamil Jozwiak [:kjozwiak]]

Used the following asan m-c build:
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1441792943/

Hopefully this helps in assigning severity to this crash.

=================================================================
==3554==ERROR: AddressSanitizer: SEGV on unknown address 0x003363657675 (pc 0x7f5399b8292c sp 0x7fffe9e7bd50 bp 0x60400011fbf8 T0)
    #0 0x7f5399b8292b in ?? ??:0
    #1 0x7f5399b95b61 in ?? ??:0
    #2 0x7f5399b181ea in ?? ??:0
    #3 0x7f5399a65f49 in ?? ??:0
    #4 0x7f53c15d233b in fLinkProgram GLContext.h:1424
    #5 0x7f53c15d0f7e in LinkProgram WebGLProgram.cpp:847
    #6 0x7f53c152138a in LinkProgram WebGLContextGL.cpp:1202
    #7 0x7f53c0b82a13 in linkProgram WebGLRenderingContextBinding.cpp:13664
    #8 0x7f53c13c70d7 in GenericBindingMethod BindingUtils.cpp:2602
    #9 0x7f53c627e89b in CallJSNative jscntxtinlines.h:235
    #10 0x7f53c62bed34 in Interpret Interpreter.cpp:3067
    #11 0x7f53c629e357 in RunScript Interpreter.cpp:704
    #12 0x7f53c627efff in Invoke Interpreter.cpp:781
    #13 0x7f53c62274cd in Invoke Interpreter.cpp:818
    #14 0x7f53c6e07615 in Call jsapi.cpp:4610
    #15 0x7f53c0f41cff in Call EventHandlerBinding.cpp:259
    #16 0x7f53c17b71d9 in Call<nsISupports *> EventHandlerBinding.h:351
    #17 0x7f53c176e89b in HandleEventSubType EventListenerManager.cpp:1009
    #18 0x7f53c1770277 in HandleEventInternal EventListenerManager.cpp:1136
    #19 0x7f53c175fa11 in HandleEventTargetChain EventDispatcher.cpp:314
    #20 0x7f53c1763eea in Dispatch EventDispatcher.cpp:651
    #21 0x7f53c34b7958 in LoadComplete nsDocumentViewer.cpp:992
    #22 0x7f53c4115ddf in EndPageLoad nsDocShell.cpp:7349
    #23 0x7f53c4112453 in OnStateChange nsDocShell.cpp:7168
    #24 0x7f53c4118eff in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) Unified_cpp_docshell_base0.cpp:7175
    #25 0x7f53bebe191b in DoFireOnStateChange nsDocLoader.cpp:1250
    #26 0x7f53bebe0c03 in doStopDocumentLoad nsDocLoader.cpp:831
    #27 0x7f53bebdde0f in DocLoaderIsEmpty nsDocLoader.cpp:721
    #28 0x7f53bebdfb93 in OnStopRequest nsDocLoader.cpp:605
    #29 0x7f53bebe05cc in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) Unified_cpp_uriloader_base0.cpp:609
    #30 0x7f53bd5df669 in RemoveRequest nsLoadGroup.cpp:643
    #31 0x7f53bf89c57c in DoUnblockOnload nsDocument.cpp:9131
    #32 0x7f53bf93d9cf in Run nsDocument.cpp:9084
    #33 0x7f53bd435303 in ProcessNextEvent nsThread.cpp:874
    #34 0x7f53bd4ac4da in NS_ProcessNextEvent nsThreadUtils.cpp:277
    #35 0x7f53bdd2d8b9 in Run MessagePump.cpp:95
    #36 0x7f53bdcba0ec in RunInternal message_loop.cc:234
    #37 0x7f53c2c7e1f7 in Run nsBaseAppShell.cpp:156
    #38 0x7f53c4ad2162 in XRE_RunAppShell nsEmbedFunctions.cpp:785
    #39 0x7f53bdcba0ec in RunInternal message_loop.cc:234
    #40 0x7f53c4ad1859 in XRE_InitChildProcess nsEmbedFunctions.cpp:621
    #41 0x48d670 in content_process_main plugin-container.cpp:237
    #42 0x7f53bae31ec4 in __libc_start_main libc-start.c:287
    #43 0x48c9cc in _start ??:?

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ??:0 ??
==3554==ABORTING

Comment 2

[Daniel Veditz [:dveditz]]

From the crash:
OpenGL: X.Org -- Gallium 0.4 on AMD RS780 -- 3.0 Mesa 10.1.3 -- texture_from_pixmap
WebGL- libGL.so.1- libGL.so.1+ GL Context- GL Context+ WebGL+

Comment 3

[Andrew Comminos [:acomminos]]

This appears to manifest itself as an attempted stack smashing in mesa's DRI implementations (I could reproduce on nouveau and swrast, but not NVIDIA's proprietary GL driver).

> #0  0x00007ffff6db5107 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> #1  0x00007ffff6db64e8 in __GI_abort () at abort.c:89
> #2  0x00007ffff6df3214 in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x7ffff6ee3a4b "*** %s ***: %s terminated\n") at ../sysdeps/posix/libc_fatal.c:175
> #3  0x00007ffff6e764e7 in __GI___fortify_fail (msg=msg@entry=0x7ffff6ee3a33 "stack smashing detected") at fortify_fail.c:31
> #4  0x00007ffff6e764b0 in __stack_chk_fail () at stack_chk_fail.c:28
> #5  0x00007fffc4b984c0 in assign_attribute_or_color_locations (prog=prog@entry=0x7fffcc979f08, target_index=target_index@entry=0, max_index=max_index@entry=16) at ../../../../src/glsl/linker.cpp:2271
> #6  0x00007fffc4b9989f in link_shaders (ctx=ctx@entry=0x7fffc840d000, prog=prog@entry=0x7fffd2792ee8) at ../../../../src/glsl/linker.cpp:3065
> #7  0x00007fffc4b0cf53 in _mesa_glsl_link_shader (ctx=ctx@entry=0x7fffc840d000, prog=prog@entry=0x7fffd2792ee8) at ../../../../src/mesa/program/ir_to_mesa.cpp:2960
> #8  0x00007fffc4a5551a in link_program (ctx=0x7fffc840d000, program=<optimized out>) at ../../../../src/mesa/main/shaderapi.c:946
> #9  0x00007fffe4ca1c36 in mozilla::gl::GLContext::fLinkProgram (this=0x7fffcebd4800, program=3) at /home/andrew/mozilla-central/gfx/gl/GLContext.h:1425
> #10 0x00007fffe610a41d in mozilla::WebGLProgram::LinkAndUpdate (this=0x7fffcbaac100) at /home/andrew/mozilla-central/dom/canvas/WebGLProgram.cpp:912
> #11 0x00007fffe610a26d in mozilla::WebGLProgram::LinkProgram (this=0x7fffcbaac100) at /home/andrew/mozilla-central/dom/canvas/WebGLProgram.cpp:847
> #12 0x00007fffe60d3741 in mozilla::WebGLContext::LinkProgram (this=0x7fffcbbfc100, prog=0x7fffcbaac100) at /home/andrew/mozilla-central/dom/canvas/WebGLContextGL.cpp:1202

It appears that we overwrite the canary (at least, according to my GDB-fu) when attempting to set a field of `to_assign[16]` (one element past the end of the array, which is 16 elements large) in assign_attribute_or_color_locations on Mesa 10.6.8. This access only happens due to what appears to be a logic error in mesa's attempts to prevent linking over 16 shader attributes.

This may be fixable within the WebGL code by limiting the number of vertex attributes permitted by our shader validator. I thought this was already done, but perhaps it may need refining.

Comment 4

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

You're probably thinking of bug 1190526.

Comment 5

[Andrew Comminos [:acomminos]]

Attachment: Terminate linking if maximum vertex attribute count is exceeded on Mesa.

Looks like ANGLE's shader validator doesn't actually check the maximum vertex attribute count. This patch ensures we don't exceed the maximum vertex attribute count on Mesa similarly to our solution for avoiding over 16 samplers per program.

Thanks!

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8678430 [details] [diff] [review]
Terminate linking if maximum vertex attribute count is exceeded on Mesa.

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

::: dom/canvas/WebGLShaderValidator.h
@@ +42,5 @@
>      void GetInfoLog(nsACString* out) const;
>      void GetOutput(nsACString* out) const;
>      bool CanLinkTo(const ShaderValidator* prev, nsCString* const out_log) const;
>      size_t CalcNumSamplerUniforms() const;
> +    size_t CalcNumAttributes() const;

Just `NumAttributes()`.
Generally it's Foo() if it's an infallible accessor, GetFoo() if fallible, and CalcFoo() if it takes much longer than just an accessor.

Comment 7

[Andrew Comminos [:acomminos]]

Attachment: Terminate linking if maximum vertex attribute count is exceeded on Mesa. Carry r=jgilbert

Thanks, updated.

Comment 8

[Andrew Comminos [:acomminos]]

Comment on attachment 8682356 [details] [diff] [review]
Terminate linking if maximum vertex attribute count is exceeded on Mesa. Carry r=jgilbert

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Fairly easily, simply by defining lots of vertex attributes.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I mention that it causes an "internal crash" in Mesa, justifying the check so it doesn't get removed by accident. The security implications were not detailed.

> Which older supported branches are affected by this flaw?

While I haven't looked into it extensively, all supported branches would initially appear to be affected on Linux (at least since ESR38).

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I don't think the WebGL shader validation code has changed significantly since ESR38, so it shouldn't be difficult to backport.

> How likely is this patch to cause regressions; how much testing does it need?

Unlikely to cause regressions. We should not provide the driver with a shader having more than its reported number of vertex attributes, even if it might support it anyway.

Comment 9

[Carsten Book [:Tomcat]]

clearing checkin-needed till this has sec-approval=+ since we need this before checkin

Comment 10

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

Comment on attachment 8682356 [details] [diff] [review]
Terminate linking if maximum vertex attribute count is exceeded on Mesa. Carry r=jgilbert

Sec-approval+ for checkin on Nov 17, two weeks into the cycle. This is to reduce the time window of vulnerability after the checkin.

Comment 11

[Andrew McCreight [:mccr8]]

This is okay to check in now.

Comment 12

[Carsten Book [:Tomcat]]

Hi, this failed to apply to m-i:

1 out of 1 hunks FAILED -- saving rejects to file dom/canvas/WebGLProgram.cpp.rej
patching file dom/canvas/WebGLShader.h
Hunk #1 FAILED at 44
1 out of 1 hunks FAILED -- saving rejects to file dom/canvas/WebGLShader.h.rej
patching file dom/canvas/WebGLShaderValidator.h
Hunk #1 succeeded at 39 with fuzz 1 (offset 0 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 1203135---Terminate-linking-if-maximum-vertex-.patch

can you take a look, thanks!

Comment 13

[Andrew Comminos [:acomminos]]

Attachment: Terminate linking if maximum vertex attribute count is exceeded on Mesa. Carry r=jgilbert

Thanks, updated.

Comment 14

[Carsten Book [:Tomcat]]

Attachment: rej file

seems this still has problems to apply , rej file attached

1 out of 1 hunks FAILED -- saving rejects to file dom/canvas/WebGLProgram.cpp.rej

Comment 15

[Andrew Comminos [:acomminos]]

I just gave it a push myself, it seems there's an issue with git-bz uploads and whitespace-related changes. Sorry about that.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8021382da972

Comment 17

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

Andrew, should we uplift this to Aurora44?

Comment 18

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

Jeff, I am wondering if you can help with comment 17. Thanks!

Comment 19

[Andrew Comminos [:acomminos]]

Comment on attachment 8690347 [details] [diff] [review]
Terminate linking if maximum vertex attribute count is exceeded on Mesa. Carry r=jgilbert

Sorry for the delay! Yes, this should likely be uplifted.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: A potential crash for Mesa users on WebGL shader programs with a high number of attributes.
[Describe test coverage new/current, TreeHerder]: N/A, test infrastructure currently uses NVIDIA's proprietary libGL.
[Risks and why]: No known risks, other drivers reject shader programs with an unsupported number of attributes anyway.
[String/UUID change made/needed]: N/A

Comment 20

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

Andrew, thanks for the aurora uplift request. Given that this bug also impacts Beta43, ESR38 and it's a sec-high, we usually land the fix on all affected branches at the same time. Do you want to nominate this for uplift to beta and esr38 as well?

Comment 21

[Andrew Comminos [:acomminos]]

Comment on attachment 8690347 [details] [diff] [review]
Terminate linking if maximum vertex attribute count is exceeded on Mesa. Carry r=jgilbert

Sure; flagging for the same approval reasons as mentioned in comment #19.

Comment 22

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

Comment on attachment 8690347 [details] [diff] [review]
Terminate linking if maximum vertex attribute count is exceeded on Mesa. Carry r=jgilbert

Carrying forward sec-approval+ from comment 10

Comment 23

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

Comment on attachment 8690347 [details] [diff] [review]
Terminate linking if maximum vertex attribute count is exceeded on Mesa. Carry r=jgilbert

This fix has been on Nightly for over a week and also a sec-high fix, taking it for esr38, aurora, beta.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b5d2cb6958d5
https://hg.mozilla.org/releases/mozilla-beta/rev/90eff805d281
https://hg.mozilla.org/releases/mozilla-esr38/rev/bdbe8785dbf5

Comment 25

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced this using Firefox 41 beta 8 on Ubuntu 14.04 x64.

Confirming the fix on the same OS for 2016-13-01 Aurora, 44.0b9, 43.0.4 and latest 38.5.2esr tinderbox build (20160114062730).