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

RESOLVED FIXED in Firefox 14

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks: 1 bug, {crash, sec-critical, testcase})

Other Branch
mozilla15
x86_64
Linux
crash, sec-critical, testcase
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox13 wontfix, firefox14+ fixed, firefox15+ fixed, firefox-esr1014+ fixed)

Details

(Whiteboard: [qa-][advisory-tracking+])

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 1

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

Updated

5 years ago
Duplicate of this bug: 756616
(Reporter)

Comment 3

5 years ago
Still valid with ballast patch, same parameters.
(Assignee)

Updated

5 years ago
Group: core-security
(Assignee)

Comment 4

5 years ago
Created attachment 625994 [details] [diff] [review]
Patch

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?
(Assignee)

Updated

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

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb27363cfba1
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
Whiteboard: [sg:critical]
(Assignee)

Updated

5 years ago
Keywords: sec-critical
Whiteboard: [sg:critical]
(Assignee)

Comment 8

5 years ago
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?

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/cb27363cfba1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox15: --- → fixed
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.
status-firefox13: --- → wontfix

Updated

5 years ago
Attachment #625994 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox-esr10: --- → affected
status-firefox14: --- → affected
tracking-firefox-esr10: --- → 14+
tracking-firefox14: --- → +
tracking-firefox15: --- → +
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+
(Assignee)

Comment 12

5 years ago
Pushed to aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/f38511c875c8
status-firefox14: affected → fixed
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+
(Assignee)

Comment 15

5 years ago
(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
status-firefox-esr10: affected → fixed
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?
(Reporter)

Comment 19

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

Updated

4 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.