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

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Arthur Gerkis]

Attachment: 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]

Attachment: ASan log

Comment 2

[Christian Holler (:decoder)]

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/[e-mail]-bfc7e887ada0/try-linux64-debug/firefox-15.0a1.en-US.linux-x86_64.tar.bz2

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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)]

Attachment: 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 - ex-MoCo]]

Attachment: 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 [:bzbarsky]]

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)]

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)]

Attachment: fix ANGLE non-null-terminated strings

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

CC'ing ANGLE and Chrome devs.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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)]

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

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

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)]

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)]

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)]

[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

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/241b44d55176

Comment 17

[Alex Keybl [:akeybl]]

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)]

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]]

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]]

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)]

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)]

...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)]

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)]

(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]]

(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)]

(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 28

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 29

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

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]

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)]

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

Comment 32

[Christian Holler (:decoder)]

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

Comment 33

[Scoobidiver (away)]

(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)]

(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 - ex-MoCo]]

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.