User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0 Iceweasel/34.0 Build ID: 20141203163643 Steps to reproduce: I opened the attached file with - Firefox 34 on ubuntu with NVIDIA closed source drivers - Iceweasel 31 on debian (jessie) with NVIDIA closed source drivers - Icewease 34 on debian (experimental) with NVIDIA closed source drivers Actual results: In all three configurations a segmentation fault happened (consistently). I haven't had the chance to test it with non NVIDIA drivers or on windows. It may be related to NVIDIA drivers, but I suspect the bug to be in FF code. This is a backtrace (iceweasel 31): #0 __strcpy_ssse3 () at ../sysdeps/x86_64/multiarch/strcpy-ssse3.S:2289 #1 0x00007ffff3d0f87c in mozilla::WebGLContext::CompileShader (this=0x7fffc3a2e9c0, shader=0x7fffc3a2e940) at /tmp/buildd/iceweasel-31.3.0esr/content/canvas/src/WebGLContextGL.cpp:3135 #2 0x00007ffff3916f33 in mozilla::dom::WebGLRenderingContextBinding::compileShader (cx=0x7fffd2d08a80, obj=..., self=0x7fffc34a7c00, args=...) at /tmp/buildd/iceweasel-31.3.0esr/build-browser/dom/bindings/WebGLRenderingContextBinding.cpp:8225 #3 0x00007ffff395beea in mozilla::dom::GenericBindingMethod (cx=0x7fffd2d08a80, argc=<optimized out>, vp=<optimized out>) at /tmp/buildd/iceweasel-31.3.0esr/dom/bindings/BindingUtils.cpp:2297 #4 0x00007ffff4959a01 in CallJSNative (args=..., native=<optimized out>, cx=<optimized out>) at /tmp/buildd/iceweasel-31.3.0esr/js/src/jscntxtinlines.h:239 #5 js::Invoke (cx=0x7fffd2d08a80, args=..., construct=(js::CONSTRUCT | unknown: 4154173016)) at /tmp/buildd/iceweasel-31.3.0esr/js/src/vm/Interpreter.cpp:475 #6 0x00007ffff49501d2 in Interpret (cx=0x7fffd2d08a80, state=...) at /tmp/buildd/iceweasel-31.3.0esr/js/src/vm/Interpreter.cpp:2620 #7 0x00007ffff4959669 in js::RunScript (cx=0x7fffd2d08a80, state=...) at /tmp/buildd/iceweasel-31.3.0esr/js/src/vm/Interpreter.cpp:422 #8 0x00007ffff49598d9 in js::Invoke (cx=0x7fffd2d08a80, args=..., construct=(js::CONSTRUCT | unknown: 4154173016), construct@entry=js::NO_CONSTRUCT) at /tmp/buildd/iceweasel-31.3.0esr/js/src/vm/Interpreter.cpp:494 It appears that compiler->getInfoSink().info.size() is 0 in ShGetInfo() lenWithNull is 1 in WebGLContext::CompileShader(), therefore len is 0 and the new memory is not allocated. I'm not sure this is actually a security issue, but maybe there could be a way to control the pointers involved in the strcpy() and do nasty things.

mac crash: bp-d7700f0e-c02b-4ca9-a014-f3ed22141216

Dan could reproduce this on OSX, so maybe it is less likely it is a driver issue. Milan, is there somebody who can look at this? Thanks.

Yes, I can see this on OS X as well, nightly. Let me take a look.

Comment on attachment 8542995 [details] [diff] [review] Checking for empty log needs to cover length 1, as we count the trailing null. Review of attachment 8542995 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLContextGL.cpp @@ +3371,5 @@ > ShGetInfo(compiler, SH_INFO_LOG_LENGTH, &lenWithNull); > > + if (lenWithNull < 2) { > + // Error in ShGetInfo. Since we're counting trailing null, > + // anything shorter than 2 means no actual length. I find this somewhat unsatisfying. It seems like lenWithNull could legitimately be 1, if the InfoLog was the empty string. I might actually prefer: if (len) { // when strings are 0 length they use a common buffer that we can't write to. ShGetInfoLog(compiler, info.BeginWriting()); } That being said, maybe you can convince me that your patch is better.

Comment on attachment 8544181 [details] [diff] [review] Skip the copying of empty strings. r=jrmuizelaar Review of attachment 8544181 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLContextGL.cpp @@ +3374,5 @@ > } else { > size_t len = lenWithNull - 1; > > nsAutoCString info; > + if (len) { Might as well add a comment like: "zero length strings can not be written to"

[Security approval request comment] How easily could an exploit be constructed based on the patch? Don't think it's easy - pretty sure this will always crash. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? As this is a security patch, didn't want to make the reason for not doing write to zero length string too obvious. Which older supported branches are affected by this flaw? 34, 35, 36, ESR31 is OK. If not all supported branches, which bug introduced the flaw? 1010371 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Same patch should apply to aurora and beta. How likely is this patch to cause regressions; how much testing does it need? This will just stop a crash, as we try to write to const+static location. Very low chance of regression.

I'm rating this as a sec-moderate since the crash doesn't seem to be exploitable. If this is a correct rating, then you don't need sec-approval to checkin.

I did a quick test on firefox 31 (Linux 64-bit official build) and it seems to be affected too

Please request Aurora36 and b2g34 approval on this when you get a chance.

Comment on attachment 8544667 [details] [diff] [review] Skip the copying of empty strings. Carry r=jrmuizelaar NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): 1110488 (or perhaps earlier) User impact if declined: Crash when there are errors in WebGL being run Testing completed: Risk to taking this patch (and alternatives if risky): Low String or UUID changes made by this patch: Not asking for ESR31 approval as this is sec-moderate.

https://hg.mozilla.org/releases/mozilla-aurora/rev/94c87aafc56c https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/44874e713ddf