Closed Bug 756600 Opened 12 years ago Closed 12 years ago

OOM Testing: Glibc Abort due to invalid free in js::TokenStream::~TokenStream

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox13 --- wontfix
firefox14 + fixed
firefox15 + fixed
firefox-esr10 14+ fixed

People

(Reporter: decoder, Assigned: jandem)

References

Details

(Keywords: crash, sec-critical, testcase, Whiteboard: [qa-][advisory-tracking+])

Attachments

(1 file)

The following command crashes on ionmonkey revision 8c54899dae82 (dbg build):

js  -e 'const libdir = "js/src/jit-test/lib/";' -A 7046 -f js/src/jit-test/tests/debug/breakpoint-resume-01.js
The reason for the missing signature is a glibc abort here:

(gdb) bt
#0  0x00007ffff6eb8d05 in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x00007ffff6ebcab6 in abort () at abort.c:92
#2  0x00007ffff6ef1d7b in __libc_message (do_abort=2, fmt=0x7ffff6fda400 "*** glibc detected *** %s: %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:189
#3  0x00007ffff6efda8f in malloc_printerr (av=<value optimized out>, p=0x7fffffffc0e0) at malloc.c:6283
#4  _int_free (av=<value optimized out>, p=0x7fffffffc0e0) at malloc.c:4795
#5  0x00007ffff6f018e3 in __libc_free (mem=<value optimized out>) at malloc.c:3738
#6  0x0000000000403b6f in js_free (p=0x7fffffffc0f0) at ../dist/include/js/Utility.h:201
#7  0x0000000000403b89 in js::Foreground::free_ (p=0x7fffffffc0f0) at ../dist/include/js/Utility.h:620
#8  0x0000000000419688 in JSRuntime::free_ (this=0x7ffff7f9a010, p=0x7fffffffc0f0) at /tmp/abc-8c54899dae82-tNQ40M/compilePath/js/src/jscntxt.h:895
#9  0x00000000004196dc in JSContext::free_ (this=0xda05e0, p=0x7fffffffc0f0) at /tmp/abc-8c54899dae82-tNQ40M/compilePath/js/src/jscntxt.h:1320
#10 0x00000000006c4bd6 in js::TokenStream::~TokenStream (this=0x7fffffffbf90, __in_chrg=<value optimized out>) at /tmp/abc-8c54899dae82-tNQ40M/compilePath/js/src/frontend/TokenStream.cpp:252
#11 0x00000000006a5350 in js::Parser::~Parser (this=0x7fffffffbf40, __in_chrg=<value optimized out>) at /tmp/abc-8c54899dae82-tNQ40M/compilePath/js/src/frontend/Parser.cpp:152
#12 0x0000000000673779 in js::frontend::CompileScript (cx=0xda05e0, scopeChain=0x7ffff6516060, callerFrame=0x0, principals=0x0, originPrincipals=0x0, compileAndGo=true, noScriptRval=false, needScriptGlobal=false, 
    chars=0xdaf660, length=137, filename=0xda9791 "/home/decoder/Mozilla/JSBugMon/repos/ionmonkey/js/src/jit-test/tests/debug/breakpoint-resume-01.js", lineno=17, version=JSVERSION_ECMA_5, source=0x7ffff6629820, 
    staticLevel=0) at /tmp/abc-8c54899dae82-tNQ40M/compilePath/js/src/frontend/BytecodeCompiler.cpp:122


I'm not getting this abort on mozilla-central. I assume this is not security-related since the test used is jsdbg2-only.
Summary: IonMonkey: OOM Testing: Crash/Signal 6 (no signature available) → IonMonkey: OOM Testing: Glibc Abort due to invalid free in js::TokenStream::~TokenStream
Still valid with ballast patch, same parameters.
Group: core-security
Attached patch PatchSplinter Review
Not an IonMonkey bug but the fix is straight-forward and this seems security sensitive.

Nicholas, flagging you for review since you've done a lot of frontend work lately. The problem is that in Parser::init:

bool
Parser::init(...)
{
    JSContext *cx = context;
    if (!cx->ensureParseMapPool())
        return false;
    tempPoolMark = cx->tempLifoAlloc().mark();
    if (!tokenStream.init(base, length, filename, lineno, version)) {
        cx->tempLifoAlloc().release(tempPoolMark);
        return false;
    }
...

If cx->ensureParseMapPool() fails, we don't call tokenStream.init. This means TokenStream::sourceMap is now uninitialized and the TokenStream destructor crashes when it tries to free sourceMap.

The patch initializes sourceMap to NULL in the constructor instead of init, so that it's no longer uninitialized if we don't call init().
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #625994 - Flags: review?
Attachment #625994 - Flags: review? → review?(n.nethercote)
Comment on attachment 625994 [details] [diff] [review]
Patch

Review of attachment 625994 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine as a spot fix.  I'm going to do a follow-up that will make this kind of thing impossible in the future.
Attachment #625994 - Flags: review?(n.nethercote) → review+
> I'm going to do a follow-up that will make this kind of thing impossible in
> the future.

Bug 757690.  It removes the unnecessary split between TokenStream::TokenStream and TokenStream::init, ensuring that all TokenStream fields are always initialized.
Whiteboard: [sg:critical]
Keywords: sec-critical
Whiteboard: [sg:critical]
Comment on attachment 625994 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 674283

User impact if declined: Freeing arbitrary memory if an attacker can trigger an OOM in Parser::init.

Testing completed (on m-c, etc.): None, patch just landed.

Risk to taking this patch (and alternatives if risky): Very small, patch changes two lines: initialize a class member in the constructor instead of init function.

String or UUID changes made by this patch: None.
Attachment #625994 - Flags: approval-mozilla-esr10?
Attachment #625994 - Flags: approval-mozilla-beta?
Attachment #625994 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/cb27363cfba1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to Jan de Mooij (:jandem) from comment #8)
> User impact if declined: Freeing arbitrary memory if an attacker can trigger
> an OOM in Parser::init.

Our final beta is being built Monday. We weighed the risk of exploit vs the risk of regression and decided to let this ride till FF14.
Attachment #625994 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 625994 [details] [diff] [review]
Patch

[Triage Comment]
Approving for Aurora 14, however. We'll approve for ESR once we've bumped the version (after 6/5).
Attachment #625994 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Christian, can you verify that this is fixed on trunk?
Comment on attachment 625994 [details] [diff] [review]
Patch

[Triage Comment]
Time to get this landed for release alongside FF14.
Attachment #625994 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(In reply to Lukas Blakk [:lsblakk] from comment #14)
> 
> [Triage Comment]
> Time to get this landed for release alongside FF14.

https://hg.mozilla.org/releases/mozilla-esr10/rev/643bd5945098
What's the difference between an IonMonkey debug JS Shell and a Firefox debug JS Shell? and how can I set one up to verify for Firefox 10.0.6esr if there is a difference?
Whoops, this was not an IonMonkey bug, so you shouldn't have to worry about dealing with that branch.
Summary: IonMonkey: OOM Testing: Glibc Abort due to invalid free in js::TokenStream::~TokenStream → OOM Testing: Glibc Abort due to invalid free in js::TokenStream::~TokenStream
So I should be able to verify this fixed for ESR by making a debug shell from latest-mozilla-esr10?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #18)
> So I should be able to verify this fixed for ESR by making a debug shell
> from latest-mozilla-esr10?

No, unfortunately it's not that easy. The problem is that the -A parameter varies across different versions, even on the same branch. There is no way to verify these bugs except by rerunning the whole testing and hoping it doesn't pop up anymore.
Marking this qa- given that there does not appear to be any way to reliably verify this fix. Please mark as qa+ if this changes.

Thanks Christian.
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][advisory-tracking+]
Group: core-security
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.