Last Comment Bug 665936 - (CVE-2011-2988) string crash found while fuzzing WebGL shaders
: string crash found while fuzzing WebGL shaders
[sg:critical?] [waiting on approved f...
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: 2.0 Branch
: x86 Windows XP
: -- normal (vote)
: mozilla8
Assigned To: Benoit Jacob [:bjacob] (mostly away)
: Milan Sreckovic [:milan]
Depends on:
  Show dependency treegraph
Reported: 2011-06-21 09:56 PDT by Daniel Veditz [:dveditz]
Modified: 2014-10-01 16:06 PDT (History)
14 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

PoC zipped (110.88 KB, application/java-archive)
2011-06-21 09:56 PDT, Daniel Veditz [:dveditz]
no flags Details
limit shader source length to 64k (1.37 KB, patch)
2011-06-24 11:30 PDT, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review-
Details | Diff | Splinter Review
limit shader source length to 256k (1.38 KB, patch)
2011-06-29 17:05 PDT, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review+
asa: approval‑mozilla‑aurora+
asa: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2011-06-21 09:56:08 PDT
Created attachment 540783 [details]
PoC zipped

Michael Jordon of Context IS submitted this Javascript string crash he found while testing WebGL in Firefox 4.0.1 on WinXP. He cannot reproduce the bug in a nightly build, but doesn't know if that means we've fixed it or the crash is just moved a bit. From Michael:

This, might be related to WebGL but I don't think it actually is. It was just exposed when doing fuzzing against the shader compiler. It isn't clear if this has a security implication, although it looks possibly like some memory is getting overwritten causing the issues to occur.  

The bug seems to manifest in two different ways, both of them when calling into nsPromiseFlatString::nsPromiseFlatString. The first manifestation ends up with a call to memcpy where the source pointer is too small for the number of bytes it is supposed to be copying so crashes on a read AV. This is string crash 1. The second is a stack overflow, where the code calls into the function xul!nsAString_internal::Assign which seems to think it can optimise the string operation by calling back into itself, which causes an infinite loop and eventually the stack runs out. This is string crash 2. Which one occurs seems to depend on what has already been loaded at the time.

The PoC minimal_crash.html exhibits the bug on 4.0.1 on XP SP3. It doesn't seem to crash the latest nightly build, however slight changes to the Javascript (which doesn't change function, just form) seems to also stop the crash so it might just be the nightly build is rebuilt in such a way that memory layout is sufficiently different to stop the underlying bug, or it could be fixed.
Comment 2 Daniel Veditz [:dveditz] 2011-06-21 14:36:49 PDT
On WinXP 4.0.1 I either get a stack overflow 
[@ nsAString_internal::Assign(unsigned short const*, unsigned int) ]

or a read violation crash
[@ memcpy | nsAString_internal::Assign(unsigned short const*, unsigned int) ]
Comment 3 David Mandelin [:dmandelin] 2011-06-21 18:31:46 PDT
I tried twice on Win 7 5.0, and I got an NPE under the shader compiler. Mem usage goes way up, so that combined with an NPE makes it look like an OOM crash. It looks like it's probably the shader compiler, not JS.
Comment 4 Daniel Veditz [:dveditz] 2011-06-21 18:40:48 PDT
In Firefox 5 on WinXP I crash [@ JS_GetStringCharsZAndLength ]

Didn't crash in nightly (7.0a1) and have not tried aurora (6.0a1) yet
Comment 5 Daniel Veditz [:dveditz] 2011-06-21 19:15:04 PDT
I agree w/David, looks more like the shader compiler than JS. So far in Fx5 looks like a null deref so maybe not a big problem anymore.
Comment 6 Daniel Veditz [:dveditz] 2011-06-21 19:23:28 PDT
I get different results in 7 and 6. In 7 it ran until I hit the slow-script dialog. in  Aurora (6.0) I get an alert 

  ERROR: 0:1: 'unexpected EOF' : syntax error

which comes from the shader compiler

Comment 7 Daniel Veditz [:dveditz] 2011-06-22 17:27:43 PDT
Is this an ANGLE bug (bugs?) that has gotten fixed along the way? exploitable?
Comment 8 Daniel Veditz [:dveditz] 2011-06-22 17:28:35 PDT
symptom of the problems reported in bug 665070?
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2011-06-23 08:22:46 PDT
ANGLE devs: this crashes Firefox 5 which uses ANGLE r611 (chrome_m10 branch, branched off r551) but doesn't crash Firefox 6 which uses ANGLE r653.

Do you see something between r551 and r653 that could be relevant to that?
Comment 10 Zhenyao Mo 2011-06-23 08:58:02 PDT
In Angle r592/593 a potential memory visit violation is fixed.  Not sure it's related to this bug though.
Comment 11 daniel-bzmz 2011-06-23 09:24:14 PDT
I don't know of anything specifically, but there certainly have been other changes to the shader compiler/translator.

That said, I'm still getting a crash in the Nightly 7.0a1 (2011-06-23) build.
Comment 12 Zhenyao Mo 2011-06-23 09:42:35 PDT
Just FYI:  I tried with chromium Top-of-tree build, and couldn't reproduce this bug.  However, as Daniel pointed out, it could be the violation just isn't triggered in chromium.
Comment 13 daniel-bzmz 2011-06-23 11:09:15 PDT
In Firefox 5 (release) I get the same message as in Comment 6.

In Firefox 7 (nightly) I get a crash, doesn't seem to be directly from ANGLE though (I build debug EGL/GLESv2 dlls, attached to Firefox and set breakpoints in all the shader compiler related entry points).  None of them got hit before it crashed. 

However if I set webgl.shader_validator to false and set webgl.verbose to true, I often get either the alert mentioned in Comment 6, or just an empty string alert.    Is it possible the shader validator is choking on it somehow? 

Similar to Mo, I'm also not replicating the crash in Chrome.

Hope this helps.
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2011-06-23 12:11:45 PDT
OK, I can reproduce in a debug build of mozilla-central (Firefox 7), here's a stack. It shows that we're hitting an assertion at 


The code there is:

static int AddString(StringTable *stable, const char *s)
    int len, loc;
    char *str;

    len = (int) strlen(s);
    while (stable->nextFree + len + 1 >= stable->size) {
        assert(stable->size < 1000000);     ////// <---- SIGABRT here!!!
        str = (char *) malloc(stable->size*2);
        memcpy(str, stable->strings, stable->size);
        stable->strings = str;
        stable->size = stable->size*2;
    loc = stable->nextFree;
    strcpy(&stable->strings[loc], s);
    stable->nextFree += len + 1;
    return loc;
} // AddString

here's the stack:

(gdb) bt
#0  0x00007f98aa4aa19d in nanosleep () at ../sysdeps/unix/syscall-template.S:82
#1  0x00007f98aa4aa010 in __sleep (seconds=<value optimized out>)
    at ../sysdeps/unix/sysv/linux/sleep.c:138
#2  0x00007f98a5a2093c in ah_crap_handler (signum=6)
    at /home/bjacob/mozilla-central/toolkit/xre/nsSigHandlers.cpp:121
#3  0x00007f98a5a25f2f in nsProfileLock::FatalSignalHandler (signo=6, info=0x7fff1f2a4cb0, 
    context=0x7fff1f2a4b80) at /home/bjacob/build/firefox/toolkit/profile/nsProfileLock.cpp:226
#4  <signal handler called>
#5  0x00007f98aa4363d5 in raise (sig=<value optimized out>)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#6  0x00007f98aa439650 in abort () at abort.c:92
#7  0x00007f98aa42f581 in __assert_fail (assertion=0x7f98a7e4cef8 "stable->size < 1000000", 
    file=<value optimized out>, line=186, function=0x7f98a7e4d0ec "AddString") at assert.c:81
#8  0x00007f98a72e0fd6 in AddString (stable=0x7f98a8c01b20, s=0x7fff1f2a566c "A144908")
    at /home/bjacob/mozilla-central/gfx/angle/src/compiler/preprocessor/atom.c:186
#9  0x00007f98a72e19b3 in LookUpAddStringHash (atable=0x7f98a8c01b20, s=0x7fff1f2a566c "A144908")
    at /home/bjacob/mozilla-central/gfx/angle/src/compiler/preprocessor/atom.c:507
#10 0x00007f98a72e1a15 in LookUpAddString (atable=0x7f98a8c01b20, s=0x7fff1f2a566c "A144908")
    at /home/bjacob/mozilla-central/gfx/angle/src/compiler/preprocessor/atom.c:524
#11 0x00007f98a72e5f8b in byte_scan (in=0x2224490, yylvalpp=0x7fff1f2a5660)
    at /home/bjacob/mozilla-central/gfx/angle/src/compiler/preprocessor/scanner.c:304
#12 0x00007f98a72e2438 in CPPdefine (yylvalpp=0x7fff1f2a5660)
    at /home/bjacob/mozilla-central/gfx/angle/src/compiler/preprocessor/cpp.c:158
#13 0x00007f98a72e4117 in readCPPline (yylvalpp=0x7fff1f2a5660)
    at /home/bjacob/mozilla-central/gfx/angle/src/compiler/preprocessor/cpp.c:785
#14 0x00007f98a72e7099 in yylex_CPP (buf=0x215bf10 "", maxSize=8192)
    at /home/bjacob/mozilla-central/gfx/angle/src/compiler/preprocessor/scanner.c:627
#15 0x00007f98a731b913 in string_input (buf=0x215bf10 "", max_size=8192, yyscanner=0x2b57c90)
    at compiler/glslang_lex.cpp:3110
#16 0x00007f98a73191a4 in yy_get_next_buffer (yyscanner=0x2b57c90) at compiler/glslang_lex.cpp:1924
---Type <return> to continue, or q <return> to quit---
#17 0x00007f98a7318d2e in yylex (yylval_param=0x7fff1f2a8d00, yyscanner=0x2b57c90)
    at compiler/glslang_lex.cpp:1765
#18 0x00007f98a731c1ea in yyparse (context=0x7fff1f2a9f30)
    at /home/bjacob/mozilla-central/gfx/angle/src/compiler/glslang_tab.cpp:2030
#19 0x00007f98a732670a in glslang_parse (context=0x7fff1f2a9f30)
    at /home/bjacob/mozilla-central/gfx/angle/src/compiler/glslang_tab.cpp:4713
#20 0x00007f98a73082f2 in PaParseStrings (count=1, string=0x7fff1f2aa1c8, length=0x0, 
    context=0x7fff1f2a9f30) at /home/bjacob/mozilla-central/gfx/angle/src/compiler/ParseHelper.cpp:1439
#21 0x00007f98a72e8dae in TCompiler::compile (this=0x32d62c0, shaderStrings=0x7fff1f2aa1c8, 
    numStrings=1, compileOptions=5)
    at /home/bjacob/mozilla-central/gfx/angle/src/compiler/Compiler.cpp:142
#22 0x00007f98a730b56b in ShCompile (handle=0x32d62c0, shaderStrings=0x7fff1f2aa1c8, numStrings=1, 
    compileOptions=4) at /home/bjacob/mozilla-central/gfx/angle/src/compiler/ShaderLang.cpp:167
#23 0x00007f98a61b8e35 in mozilla::WebGLContext::CompileShader (this=0x3156b30, sobj=0x29e6ca0)
    at /home/bjacob/mozilla-central/content/canvas/src/WebGLContextGL.cpp:3926
#24 0x00007f98a6a43a5c in nsIDOMWebGLRenderingContext_CompileShader (cx=0x2c79cd0, argc=1, 
    vp=0x7f9896107278) at /home/bjacob/build/firefox/js/src/xpconnect/src/dom_quickstubs.cpp:28967
#25 0x00007f98a7659507 in js::CallJSNative (cx=0x2c79cd0, 
    native=0x7f98a6a43903 <nsIDOMWebGLRenderingContext_CompileShader(JSContext*, uintN, jsval*)>, 
    args=...) at /home/bjacob/mozilla-central/js/src/jscntxtinlines.h:282
#26 0x00007f98a783225c in CallCompiler::generateNativeStub (this=0x7fff1f2aab90)
    at /home/bjacob/mozilla-central/js/src/methodjit/MonoIC.cpp:808
#27 0x00007f98a782e620 in js::mjit::ic::NativeCall (f=..., ic=0x35fbc68)
    at /home/bjacob/mozilla-central/js/src/methodjit/MonoIC.cpp:1026
#28 0x00007f989567f5fe in ?? ()
#29 0x00007f989567d2a5 in ?? ()
#30 0x0000000000000027 in ?? ()
#31 0x00007fff1f2aac30 in ?? ()
#32 0x0000000000000000 in ?? ()
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2011-06-23 12:14:06 PDT
Here are some local variables:

(gdb) up
#8  0x00007f98a72e0fd6 in AddString (stable=0x7f98a8c01b20, s=0x7fff1f2a566c "A144908")
    at /home/bjacob/mozilla-central/gfx/angle/src/compiler/preprocessor/atom.c:186
warning: Source file is more recent than executable.
186             assert(stable->size < 1000000);
(gdb) p stable
$1 = (StringTable *) 0x7f98a8c01b20
(gdb) p stable->size
$2 = 1048576
(gdb) p s
$3 = 0x7fff1f2a566c "A144908"
(gdb) p len
$4 = 7
(gdb) p stable->nextFree
$5 = 1048569
(gdb) p stable->nextFree+len+1
$6 = 1048577
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2011-06-23 12:15:52 PDT
Mo, Daniel: have you tried debug builds of Chrome? Do you get this assertion failure there?
Comment 17 Zhenyao Mo 2011-06-24 09:07:57 PDT
No, I can't get it in chromium debug build.
Comment 18 daniel-bzmz 2011-06-24 09:28:51 PDT
A couple thoughts here: 

For the assert -- you won't hit that in the release build, so that doesn't really tell us specifically where the problem is (other than due to a very large string/number of atoms).  You might want to try disabling it in your debug build and see what sort of "fun" you can encounter then.

I believe chrome has a maximum limit on the size of string that it'll even attempt to pass into the validator or ANGLE as a shader source (I'm not sure what the limit is though).  It seems that if firefox implemented a maximum shader string length as well, it might knock out a whole bunch of attempted OOM exploits like this.

I can't think of a good use-case where shader source should ever need to be more than a couple MB in size.  Maybe if someone wants to write a novel in their comments, but even then you can fit a *lot* of ASCII text into 1 or 2 MB.

We could probably add this sort maximum string check into ANGLE (and/or the validator?) to make this more consistent across browsers, and it would likely be better to actually propagate an error out instead of just assert… please file an ANGLE issue if you want us to investigate, but a maximum string length check in firefox is likely to be the fastest turn-around.
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2011-06-24 11:30:56 PDT
Created attachment 541730 [details] [diff] [review]
limit shader source length to 64k

Thanks. Actually I don't even see a use case for shaders longer than 64k, and even all the ray/path tracing demos I've seen seem to be well under that limit, so this patch implements this limit. Removes the crash here on the present testcase.
Comment 20 Benoit Jacob [:bjacob] (mostly away) 2011-06-24 11:49:38 PDT
Chrome devs: what's the shader source length limit in Chrome? Let's use the same as you.
Comment 21 Jeff Muizelaar [:jrmuizel] 2011-06-24 12:12:51 PDT
Comment on attachment 541730 [details] [diff] [review]
limit shader source length to 64k

>+    const PRUint32 maxSourceLength = 65535;
>+    if (strlen(sourceCString.get()) > maxSourceLength)
>+        return ErrorInvalidValue("shaderSource: source has more than %d characters", maxSourceLength);

Use .Length() instead of strlen()

Also it would be good to pick a length that matches Chrome along with test cases for just over and just under.
Comment 22 Daniel Veditz [:dveditz] 2011-06-29 16:48:05 PDT
        assert(stable->size < 1000000);     ////// <---- SIGABRT here!!!
        str = (char *) malloc(stable->size*2);
        memcpy(str, stable->strings, stable->size);

Without the assert that code is a buffer overrun waiting to happen once stable->size*2 overflows. Unless something outside this routine is enforcing limits that will keep it from getting there?
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2011-06-29 16:51:08 PDT
Good catch, we need to guard against integer overflow there.

Incidentally, my patch limiting shader source length to 64k would have prevented that.

Mo, let me ask again: is Chrome implementing a limit on shader source string length?
Comment 24 Benoit Jacob [:bjacob] (mostly away) 2011-06-29 17:02:41 PDT
Filed the integer overflow problem as a ANGLE security bug:
Comment 25 Benoit Jacob [:bjacob] (mostly away) 2011-06-29 17:05:52 PDT
Created attachment 543014 [details] [diff] [review]
limit shader source length to 256k

Here's a new version using .Length() and the limit is upped to 256k which is an insanely big size for a shader source hence should be easier to r+.
Comment 26 Gregg Tavares 2011-06-29 17:06:36 PDT
chrome does not have a shader source limit as far as I know.

If we do put a limit I think it should be in the spec. I argued for this before but was shot down. Maybe opinions have changed.

If we do put it in the spec we have to define is the limit before stripping comments or after? Before remapping variables or after? Before re-writing or after?
Comment 27 Benoit Jacob [:bjacob] (mostly away) 2011-06-29 17:14:03 PDT
I am of the opinion that a limit is needed, and I agree it should be in the spec.

The limit should be as implementation-independent as possible. The spec should be self-contained and not depend on anything particular that ANGLE does. So it could well be that using the size after stripping comments is a good idea, but I'm wary of using the size after remapping variables and/or after rewriting as that seems too dependent on ANGLE details.

This conversation should probably be had on a WebGL mailing list ;-)
Comment 28 Zhenyao Mo 2011-06-29 17:16:48 PDT
(In reply to comment #23)
> Good catch, we need to guard against integer overflow there.
> Incidentally, my patch limiting shader source length to 64k would have
> prevented that.
> Mo, let me ask again: is Chrome implementing a limit on shader source string
> length?

No, I don't think so.

(In reply to comment #23)
> Good catch, we need to guard against integer overflow there.
> Incidentally, my patch limiting shader source length to 64k would have
> prevented that.
> Mo, let me ask again: is Chrome implementing a limit on shader source string
> length?
Comment 29 daniel-bzmz 2011-06-29 17:35:34 PDT
(In reply to comment #28)
> (In reply to comment #23)
> > Good catch, we need to guard against integer overflow there.
> > 
> > Incidentally, my patch limiting shader source length to 64k would have
> > prevented that.
> > 
> > Mo, let me ask again: is Chrome implementing a limit on shader source string
> > length?
> No, I don't think so.
I discussed this with Vangelis and John from Google today. They said they don't have a explicit limit on the source string length, but it might end up effectively being limited by their command buffer implementation which has a maximum packet size of 1 MB.  For buffers and textures they explicitly break them up into pieces < 1 MB but they don't have anything like that for shader source strings so it might be getting dropped somewhere due to that limit.
Comment 30 Gregg Tavares 2011-06-29 17:40:15 PDT
There is no such limit. At the time I implemented it I wanted to make limited to the size of our command bufffer but I was specifically told OpenGL has no such limit so I wasn't allowed to have one either.

Lots of code was added to handle this case as well as the gets of passing strings larger than 1meg back from GL (GetShaderSource) etc.
Comment 31 daniel-bzmz 2011-06-29 17:45:32 PDT
Gregg: can you figure out what happens to the shader in the above PoC attachment under Chrome then?  I've run this with a debug build of ANGLE and I never see that shader show up in a breakpoint on glShaderSource.
Comment 32 Jeff Muizelaar [:jrmuizel] 2011-06-29 19:09:22 PDT
Comment on attachment 543014 [details] [diff] [review]
limit shader source length to 256k

I'm going to wait until there is consensus on what the limit should be.
Comment 33 Gregg Tavares 2011-06-29 19:32:19 PDT
(In reply to comment #31)
> Gregg: can you figure out what happens to the shader in the above PoC
> attachment under Chrome then?  I've run this with a debug build of ANGLE and
> I never see that shader show up in a breakpoint on glShaderSource.

In ran it in DumpRenderTree. The 8000000 version just runs out of memory in JavaScript, at least in a debug build. shortened it to 200000 and it asserts the same place as comment 14
Comment 34 Jeff Muizelaar [:jrmuizel] 2011-07-05 13:25:53 PDT
Comment on attachment 543014 [details] [diff] [review]
limit shader source length to 256k

># HG changeset patch
># Parent 868de5e9e21765a7c5fcbe6aa8d997120a06a7e7
>diff --git a/content/canvas/src/WebGLContextGL.cpp b/content/canvas/src/WebGLContextGL.cpp
>--- a/content/canvas/src/WebGLContextGL.cpp
>+++ b/content/canvas/src/WebGLContextGL.cpp
>@@ -4091,21 +4091,29 @@ WebGLContext::GetShaderSource(nsIWebGLSh
> WebGLContext::ShaderSource(nsIWebGLShader *sobj, const nsAString& source)
> {
>     WebGLShader *shader;
>     WebGLuint shadername;
>     if (!GetConcreteObjectAndGLName("shaderSource: shader", sobj, &shader, &shadername))
>         return NS_OK;
>-    if (!NS_IsAscii(nsPromiseFlatString(source).get()))
>+    const nsPromiseFlatString& flatSource = PromiseFlatString(source);
>+    if (!NS_IsAscii(flatSource.get()))
>         return ErrorInvalidValue("shaderSource: non-ascii characters found in source");
>-    shader->SetSource(NS_LossyConvertUTF16toASCII(source));
>+    const nsCString& sourceCString = NS_LossyConvertUTF16toASCII(flatSource);
>+    const PRUint32 maxSourceLength = (PRUint32(1)<<18) - 1;

Please use 256*1024-1 instead of the shift version. It's easier to read. And maybe add a comment about what we discussed in person.
Comment 35 Benoit Jacob [:bjacob] (mostly away) 2011-07-07 17:09:33 PDT

Sorry, forgot to apply your latest review comments.
Comment 36 Johnny Stenback (:jst, 2011-07-14 13:56:55 PDT
Comment on attachment 543014 [details] [diff] [review]
limit shader source length to 256k

This is a pretty simple fix for a security issue, we should get this in.
Comment 38 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 15:26:13 PDT
qa- as no QA fix verification needed
Comment 39 Daniel Veditz [:dveditz] 2011-09-23 11:42:43 PDT
Never cc'd the reporter on this bug
Comment 40 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-27 14:47:55 PDT
Reconsidering qa- on this bug. Dan, is this easily testable given the attached PoC for Firefox 7 and 8?
Comment 41 Raymond Forbes[:rforbes] 2013-07-19 18:20:40 PDT

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