Heap-buffer overread in WebGLContext::CompileShader (nsIDOMWebGLRenderingContext_CompileShader when inlining happens)

VERIFIED FIXED in Firefox 13

Status

()

Core
Canvas: WebGL
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Arthur Gerkis, Assigned: bjacob)

Tracking

({regression})

14 Branch
mozilla15
x86_64
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed, firefox15 verified, firefox-esr10 unaffected)

Details

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
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)
(Reporter)

Comment 1

5 years ago
Created attachment 620786 [details]
ASan log
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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)?
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*)
Attachment #620785 - Attachment mime type: text/plain → text/html
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.
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
QA Contact: untriaged → canvas.webgl
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.
Summary: Heap-buffer-overflow in nsIDOMWebGLRenderingContext_CompileShader → Heap-buffer-overflow in WebGLContext::CompileShader (nsIDOMWebGLRenderingContext_CompileShader when inlining happens)
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
Created attachment 621079 [details] [diff] [review]
fix ANGLE non-null-terminated strings
Attachment #621079 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

5 years ago
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.  ;)
Attachment #621079 - Flags: review?(bzbarsky) → review+
Filed http://code.google.com/p/angleproject/issues/detail?id=326
(Assignee)

Updated

5 years ago
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
Assignee: nobody → bjacob
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla13
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:
Attachment #621079 - Flags: approval-mozilla-beta?
Attachment #621079 - Flags: approval-mozilla-aurora?
[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 16

5 years ago
https://hg.mozilla.org/mozilla-central/rev/241b44d55176
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla13 → mozilla15
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.
Attachment #621079 - Flags: approval-mozilla-beta?
Attachment #621079 - Flags: approval-mozilla-beta+
Attachment #621079 - Flags: approval-mozilla-aurora?
Attachment #621079 - Flags: approval-mozilla-aurora+

Updated

5 years ago
status-firefox-esr10: --- → unaffected
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.
Keywords: sec-high
Whiteboard: [sg:high]
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".
Blocks: 676071
Keywords: sec-high → regression
Summary: Heap-buffer-overflow in WebGLContext::CompileShader (nsIDOMWebGLRenderingContext_CompileShader when inlining happens) → Heap-buffer overread in WebGLContext::CompileShader (nsIDOMWebGLRenderingContext_CompileShader when inlining happens)
Whiteboard: [sg:high]
Whiteboard: not exploitable? need evidence
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.
Blocks: 732233
Keywords: sec-vector
Whiteboard: not exploitable? need evidence
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
status-firefox13: --- → fixed
status-firefox14: --- → fixed
status-firefox15: --- → fixed
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.
Status: RESOLVED → VERIFIED
status-firefox13: fixed → ---
status-firefox14: fixed → ---
status-firefox15: fixed → ---

Comment 30

5 years ago
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.
Group: core-security
Keywords: sec-vector

Updated

5 years ago
Status: VERIFIED → RESOLVED
Last Resolved: 5 years ago5 years ago
status-firefox13: --- → fixed
status-firefox14: --- → fixed
status-firefox15: --- → verified
@scoobidiver, I assume you did not intend to remove the VERIFIED status here, resetting.
No longer blocks: 676071, 732233
Status: RESOLVED → VERIFIED
Blocks: 676071, 732233

Comment 33

5 years ago
(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.
You need to log in before you can comment on or make changes to this bug.