Last Comment Bug 751643 - Heap-buffer overread in WebGLContext::CompileShader (nsIDOMWebGLRenderingContext_CompileShader when inlining happens)
: Heap-buffer overread in WebGLContext::CompileShader (nsIDOMWebGLRenderingCont...
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: 14 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 676071 732233
  Show dependency treegraph
 
Reported: 2012-05-03 11:09 PDT by Arthur Gerkis
Modified: 2012-05-21 12:14 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
verified
unaffected


Attachments
PoC triggering the crash (813 bytes, text/html)
2012-05-03 11:09 PDT, Arthur Gerkis
no flags Details
ASan log (3.70 KB, text/plain)
2012-05-03 11:09 PDT, Arthur Gerkis
no flags Details
AddressSanitizer Debug Log (8.62 KB, text/plain)
2012-05-03 14:04 PDT, Christian Holler (:decoder)
no flags Details
OS X ASAN log from May 3 2012 trunk (3.44 KB, text/plain)
2012-05-03 14:22 PDT, Al Billings [:abillings]
no flags Details
fix ANGLE non-null-terminated strings (1.25 KB, patch)
2012-05-04 09:51 PDT, Benoit Jacob [:bjacob] (mostly away)
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Arthur Gerkis 2012-05-03 11:09:20 PDT
Created attachment 620785 [details]
PoC triggering the crash

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)
Comment 1 Arthur Gerkis 2012-05-03 11:09:50 PDT
Created attachment 620786 [details]
ASan log
Comment 2 Christian Holler (:decoder) 2012-05-03 11:17:49 PDT
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
Comment 3 David Baron :dbaron: ⌚️UTC-10 2012-05-03 13:45:52 PDT
(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)?
Comment 4 Christian Holler (:decoder) 2012-05-03 14:04:48 PDT
Created attachment 620845 [details]
AddressSanitizer Debug Log

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*)
Comment 5 Al Billings [:abillings] 2012-05-03 14:22:34 PDT
Created attachment 620850 [details]
OS X ASAN log from May 3 2012 trunk

This is the log from my opt ASAN nightly build on OS X.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-05-03 20:41:48 PDT
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.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-05-04 09:45:19 PDT
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.
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-05-04 09:51:27 PDT
Created attachment 621079 [details] [diff] [review]
fix ANGLE non-null-terminated strings
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-05-04 09:52:32 PDT
CC'ing ANGLE and Chrome devs.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-05-04 09:55:06 PDT
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.  ;)
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-05-04 10:08:25 PDT
Filed http://code.google.com/p/angleproject/issues/detail?id=326
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-05-04 10:10:25 PDT
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.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-05-06 19:21:56 PDT
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 14 Benoit Jacob [:bjacob] (mostly away) 2012-05-06 19:22:11 PDT
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:
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2012-05-06 19:24:26 PDT
[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 17 Alex Keybl [:akeybl] 2012-05-09 15:44:26 PDT
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.
Comment 18 Christian Holler (:decoder) 2012-05-11 09:42:20 PDT
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.
Comment 20 Daniel Veditz [:dveditz] 2012-05-11 12:31:04 PDT
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".
Comment 21 Daniel Veditz [:dveditz] 2012-05-11 13:57:26 PDT
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.
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2012-05-11 14:25:31 PDT
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.
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2012-05-11 14:27:21 PDT
...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.
Comment 24 Benoit Jacob [:bjacob] (mostly away) 2012-05-11 14:31:17 PDT
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.
Comment 25 Benoit Jacob [:bjacob] (mostly away) 2012-05-11 14:33:21 PDT
(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.
Comment 26 Daniel Veditz [:dveditz] 2012-05-11 17:10:16 PDT
(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.
Comment 27 Benoit Jacob [:bjacob] (mostly away) 2012-05-12 14:24:04 PDT
(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.
Comment 29 Al Billings [:abillings] 2012-05-14 13:14:08 PDT
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.
Comment 30 chris hofmann 2012-05-14 15:20:04 PDT
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?
Comment 31 Benoit Jacob [:bjacob] (mostly away) 2012-05-14 15:35:34 PDT
Up to Daniel who has given this more thought than I have.
Comment 32 Christian Holler (:decoder) 2012-05-19 07:59:09 PDT
@scoobidiver, I assume you did not intend to remove the VERIFIED status here, resetting.
Comment 33 Scoobidiver (away) 2012-05-19 08:07:30 PDT
(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.
Comment 34 Christian Holler (:decoder) 2012-05-19 08:10:44 PDT
(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.
Comment 35 Al Billings [:abillings] 2012-05-21 12:14:56 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.