Closed Bug 665936 (CVE-2011-2988) Opened 13 years ago Closed 13 years ago

string crash found while fuzzing WebGL shaders

Categories

(Core :: Graphics: CanvasWebGL, defect)

2.0 Branch
x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8
Tracking Status
firefox5 - wontfix
firefox6 + fixed
firefox7 + fixed
firefox8 --- fixed
status2.0 --- wontfix
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: dveditz, Assigned: bjacob)

References

Details

(Whiteboard: [sg:critical?] [waiting on approved fixes to land on 6 and 7][qa?])

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file 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.
On WinXP 4.0.1 I either get a stack overflow 
[@ nsAString_internal::Assign(unsigned short const*, unsigned int) ]
bp-32c09dc1-3ff0-4bcf-990d-d50f02110621
bp-729660d2-449e-4f7c-b487-fc66b2110621

or a read violation crash
[@ memcpy | nsAString_internal::Assign(unsigned short const*, unsigned int) ]
bp-4db6c78d-9ce1-4a50-925d-dc2f92110621
bp-d6837215-c204-411b-96d0-1487a2110621
Status: UNCONFIRMED → NEW
Crash Signature: [@ memcpy | nsAString_internal::Assign(unsigned short const*, unsigned int) ] [@ nsAString_internal::Assign(unsigned short const*, unsigned int) ]
Ever confirmed: true
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.
In Firefox 5 on WinXP I crash [@ JS_GetStringCharsZAndLength ]
bp-387ee110-c4bc-4989-99ab-f8e822110621
bp-5c506167-b679-47ff-8215-264cd2110621

Didn't crash in nightly (7.0a1) and have not tried aurora (6.0a1) yet
Crash Signature: [@ memcpy | nsAString_internal::Assign(unsigned short const*, unsigned int) ] [@ nsAString_internal::Assign(unsigned short const*, unsigned int) ] → [@ memcpy | nsAString_internal::Assign(unsigned short const*, unsigned int) ] [@ nsAString_internal::Assign(unsigned short const*, unsigned int) ] [@ JS_GetStringCharsZAndLength ]
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.
Assignee: general → nobody
Component: JavaScript Engine → Canvas: WebGL
QA Contact: general → canvas.webgl
Summary: Javascript string crash found while fuzzing → string crash found while fuzzing WebGL shaders
Version: unspecified → 2.0 Branch
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

  alert(gl.getShaderInfoLog(shader));
Is this an ANGLE bug (bugs?) that has gotten fixed along the way? exploitable?
Assignee: nobody → bjacob
symptom of the problems reported in bug 665070?
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?
In Angle r592/593 a potential memory visit violation is fixed.  Not sure it's related to this bug though.
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.
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.
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.
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 

   src/compiler/preprocessor/atom.c:186

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);
        free(stable->strings);
        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 ?? ()
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
Mo, Daniel: have you tried debug builds of Chrome? Do you get this assertion failure there?
No, I can't get it in chromium debug build.
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.
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.
Attachment #541730 - Flags: review?
Attachment #541730 - Flags: review? → review?(jmuizelaar)
Chrome devs: what's the shader source length limit in Chrome? Let's use the same as you.
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.
Attachment #541730 - Flags: review?(jmuizelaar) → review-
        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?
Whiteboard: [sg:critical?]
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?
Filed the integer overflow problem as a ANGLE security bug:
http://code.google.com/p/angleproject/issues/detail?id=176
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+.
Attachment #541730 - Attachment is obsolete: true
Attachment #543014 - Flags: review?(jmuizelaar)
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?
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 ;-)
(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?
(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.
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.
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 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.
(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 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
> 
> NS_IMETHODIMP
> 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.
Attachment #543014 - Flags: review?(jmuizelaar) → review+
http://hg.mozilla.org/mozilla-central/rev/aa69bb7c6bfe

Sorry, forgot to apply your latest review comments.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
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.
Attachment #543014 - Flags: approval-mozilla-beta?
Attachment #543014 - Flags: approval-mozilla-aurora?
Attachment #543014 - Flags: approval-mozilla-beta?
Attachment #543014 - Flags: approval-mozilla-beta+
Attachment #543014 - Flags: approval-mozilla-aurora?
Attachment #543014 - Flags: approval-mozilla-aurora+
Whiteboard: [sg:critical?] → [sg:critical?] [waiting on approved fixes to land on 6 and 7]
Alias: CVE-2011-2988
qa- as no QA fix verification needed
Whiteboard: [sg:critical?] [waiting on approved fixes to land on 6 and 7] → [sg:critical?] [waiting on approved fixes to land on 6 and 7][qa-]
Never cc'd the reporter on this bug
Reconsidering qa- on this bug. Dan, is this easily testable given the attached PoC for Firefox 7 and 8?
Whiteboard: [sg:critical?] [waiting on approved fixes to land on 6 and 7][qa-] → [sg:critical?] [waiting on approved fixes to land on 6 and 7][qa?]
Group: core-security
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: