Heap-buffer-overflow can be triggered while trying to compile WebGL shaders. Test-case is flaky - it will reload itself until crash. Was unable to reproduce on Windows 7 x64, 15.0a1 (2012-05-01). Crash was found on 14.0a1 version (ASan log respectively)

Reproduced using this try build (which will also give an ASan stack with less inlining if thats desired): http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/decoder@own-hero.net-bfc7e887ada0/try-linux64-debug/firefox-15.0a1.en-US.linux-x86_64.tar.bz2

(In reply to Christian Holler (:decoder) from comment #2) > Reproduced using this try build (which will also give an ASan stack with > less inlining if thats desired): > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/decoder@own-hero. > net-bfc7e887ada0/try-linux64-debug/firefox-15.0a1.en-US.linux-x86_64.tar.bz2 Care to share that stack, so we can put this in an appropriate component (either XPConnect or WebGL)?

No! It's mine! My precious stack trace! See attachment for the full log :) Regarding your question of component, I'd guess it's WebGL because this function is #2 in the trace: mozilla::WebGLContext::CompileShader(nsIWebGLShader*)

This is the log from my opt ASAN nightly build on OS X.

At first glance, if the uniform name or mapped name is longer than the allowed max length, ShGetActiveUniform will hand back a non-null-terminated buffer (because it uses strncpy with maxlength+1 as the count and strncpy does NOT terminate if it runs out of space). So either ShGetActiveUniform needs to be fixed or the caller in WebGLContext::CompileShader should stamp a null at the ends of the strings it gets back.

From conversation with Christian, the m-c changeset he's working off is 89e9b9213670. So the line numbers are like this: nsAutoArrayPtr<char> attribute_name(new char[attrib_max_length+1]); 4674 nsAutoArrayPtr<char> uniform_name(new char[uniform_max_length+1]); // alloc here nsAutoArrayPtr<char> mapped_name(new char[mapped_max_length+1]); for (int i = 0; i < num_uniforms; i++) { int length, size; ShDataType type; ShGetActiveUniform(compiler, i, &length, &size, &type, uniform_name, mapped_name); if (useShaderSourceTranslation) { shader->mUniforms.AppendElement(WebGLMappedIdentifier( nsDependentCString(uniform_name), 4688 nsDependentCString(mapped_name))); } So indeed, the ideas in comment 6 should work.

CC'ing ANGLE and Chrome devs.

Comment on attachment 621079 [details] [diff] [review] fix ANGLE non-null-terminated strings r=me, I guess. I'm not exactly a module peer here. ;)

Filed http://code.google.com/p/angleproject/issues/detail?id=326

The peerdom thing doesn't apply well to ANGLE. The 'right people' to r+ ANGLE patches are ANGLE maintainers, and they aren't Mozilla module owners either.

http://hg.mozilla.org/integration/mozilla-inbound/rev/241b44d55176 Also checked in upstream as ANGLE r1070: http://code.google.com/p/angleproject/source/detail?r=1070

Comment on attachment 621079 [details] [diff] [review] fix ANGLE non-null-terminated strings [Approval Request Comment] Regression caused by (bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String changes made by this patch:

[Approval Request Comment] Regression caused by (bug #): bug 676071 User impact if declined: heap buffer overflow = potential security flaw Testing completed (on m-c, etc.): m-i Risk to taking this patch (and alternatives if risky): 2-line simple patch, not risky String changes made by this patch: none

Comment on attachment 621079 [details] [diff] [review] fix ANGLE non-null-terminated strings [Triage Comment] Security regression in FF13 and trivial to fix. Approved for both Aurora 14 and Beta 13.

Assuming sg:high since it might be possible to read beyond the missing null-terminator and get access to random memory behind the string (information leakage). If it's not possible to either get that information back into content, or if it's possible to somehow execute arbitrary code through this, please adjust the security rating accordingly.

It will read beyond the terminator, and if it doesn't crash (not exploitable) then random memory will be incorporated into the name and/or mapped_name of the WebGLMappedIdentifier put into the mAttributes or mUniforms arrays. The shader program may not work, but how would a malicious shader 1) recover those names or 2) report them back in any useable way. OK, maybe the latter could be accomplished by drawing to the canvas. Is there any evidence this is actually exploitable? In a non-ASan build (so it doesn't abort at the over-read) can a shader find the internal representation of the identifiers? Reading off the end of an array is not an "overflow".

Christian points out the mapping feature was added to protect against broken drivers, and the code for mUniforms arrays was added in security bug 732233 for much the same reason. Does this allow a bypass of that? It's the mapped_names we pass into the driver and they're all the same length--it's the script-supplied names that are going to trigger this bug.

These strings get copied, in WebGLContext::CompileShader. The bug caused arbitrarily long strings (due to lack of null-termination) to be copied into the fixed-size buffer allocated for mapped_name and uniform_name. So the term 'buffer overflow' seems appropriate i.e. it's not just illegal reads, it's also illegal writes past the end of the destination buffers. The crash was with mapped_name but the ANGLE patch affects uniform_name as well, and also attrib_name; I don't know whether it was possible to trigger the buffer overflow with uniform_name or attrib_name.

...though I should amend 'arbitrarily long' a bit. Any string longer than 256 characters would have been rejected by the ANGLE parser much earlier. That's probably why we didn't see any crash with uniform_name and attrib_name which are big enough buffers to receive 256 characters. mapped_name is only 32 bytes, so it seems possible to write 256-32 = 224 bytes past the end of mapped_name.

But I don't see how an attacker could have any control over the values of the bytes that get scribbled. AFAICS, all an attacker can do is cause up to 224 heap bytes, that he can't control, to be copied past the end of another heap buffer.

(In reply to Benoit Jacob [:bjacob] from comment #23) > ...though I should amend 'arbitrarily long' a bit. Any string longer than > 256 characters would have been rejected by the ANGLE parser much earlier. > That's probably why we didn't see any crash with uniform_name and > attrib_name which are big enough buffers to receive 256 characters. > mapped_name is only 32 bytes, so it seems possible to write 256-32 = 224 > bytes past the end of mapped_name. Sorry, friday night, tired, i should stop. In absence of null termination, the source string could really be arbitrarily long.

(In reply to Benoit Jacob [:bjacob] from comment #22) > These strings get copied, in WebGLContext::CompileShader. The bug caused > arbitrarily long strings (due to lack of null-termination) to be copied into > the fixed-size buffer allocated for mapped_name and uniform_name. So the > term 'buffer overflow' seems appropriate If that's what's happening then I agree that would be an "overflow", but I must be missing something. 1. ShGetInfo returns 1+MAX_SYMBOL_NAME_LEN for all three types 2. we allocate 1+MAX_SYMBOL_NAME_LEN + 1 space for the name 3. ShGetActiveUniform() calls getVariableInfo() which does a strncpy(..., 1+MAX_SYMBOL_NAME_LEN), possibly leaving it not null-terminated but not overwriting anything. There's at least one uninitialized character at the end of the buffer. 4. nsDependentCString() creates a nsACString out of the buffer, scanning until it finds a null to figure out the length. Might crash harmlessly on an access violation here, but if not the string is now longer than expected and contains extra memory. 5. WebGLMappedIdentifier contains two nsCStrings, initialized from the nsDependentCStrings. This will copy their contents, but into buffers newly allocated based on the size reported by the nsDependentCString. 6. the WebGLMappedIdentifier is added to a nsTArray. After this point I don't know what happens to the uniform and attribute names and it's possible that being too long is bad, but I'm not seeing an overwrite in CompileShader. Then again you said the crash was with mapped_name and I'm not seeing how that's possible either (we create those and they're shorter than MAX_SYMBOL_NAME_LEN), so maybe there's something important I'm missing.

(In reply to Daniel Veditz [:dveditz] from comment #26) > 5. WebGLMappedIdentifier contains two nsCStrings, initialized from > the nsDependentCStrings. This will copy their contents, but > into buffers newly allocated based on the size reported by > the nsDependentCString. Hah, yes, that latter point ("allocated based on the size reported by the nsDependentCString") is what I missed. It seems that you're right.

http://hg.mozilla.org/releases/mozilla-aurora/rev/104b56122486 http://hg.mozilla.org/releases/mozilla-beta/rev/0540130b5f2f

Used two of Christian's ASAN 64-bit Linux builds, the one that reproduced the problem (linked to above) and a build from the day after the fix was checked into trunk. Bug repro's cleanly with PoC pre-fix and does not reproduce post-fix. Marking this as verified.

So it sounds like its ok to open this bug up now, and we don't think its a security issue. Does this sound correct?

Up to Daniel who has given this more thought than I have.

@scoobidiver, I assume you did not intend to remove the VERIFIED status here, resetting.

(In reply to Christian Holler (:decoder) from comment #32) > @scoobidiver, I assume you did not intend to remove the VERIFIED status > here, resetting. VERIFIED means it's verified in every versions where the patch landed. Based on comment 29, it's only verified in Fx 15, that is why it should be marked as RESOLVED.

(In reply to Scoobidiver from comment #33) > VERIFIED means it's verified in every versions where the patch landed. No. VERIFIED means it was verified on trunk, just as RESOLVED means it was resolved on trunk. For branches, the verified flags for each branch are used instead.

What Christian says is correct. "Verified" and "Resolved" states refer to Trunk unless the bug is specifically branch only. That's why we have flags for branches.