Crash [@ Decompile] with let expressions

VERIFIED FIXED in Firefox 15

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: decoder, Assigned: dmandelin)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla17
x86
Linux
crash, sec-high, testcase, valgrind
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15+ fixed, firefox16+ fixed, firefox17 verified, firefox-esr1015+ fixed)

Details

(Whiteboard: [js:p1][sg:high][advisory-tracking+], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
The following test crashes on mozilla-central revision 3fa30b0edd15 (options -m -n -a), 32 bit opt build:


let ([] = 1) 3; 
let (i) new [i][print[i]];


Valgrind Errors:

==59269== Conditional jump or move depends on uninitialised value(s)
==59269==    at 0x80F970B: Decompile(SprintStack*, unsigned char*, int) (jsopcode.cpp:5077)
==59269==    by 0x81004DD: DecompileCode(JSPrinter*, JSScript*, unsigned char*, unsigned int, unsigned int) (jsopcode.cpp:5404)
==59269==    by 0x81006F0: DecompileExpression(JSContext*, JSScript*, JSFunction*, unsigned char*) (jsopcode.cpp:5810)
==59269==    by 0x81009C1: js_DecompileValueGenerator (jsopcode.cpp:5699)
==59269==    by 0x807E97C: js_ReportValueErrorFlags(JSContext*, unsigned int, unsigned int, int, JS::Value const&, JSString*, char const*, char const*) (jsopcode.h:401)
==59269==    by 0x809A7BF: js_ReportIsNotFunction(JSContext*, JS::Value const*, unsigned int) (jsfun.cpp:1383)
==59269==    by 0x80CDAC5: js::InvokeConstructorKernel(JSContext*, js::CallArgs const&) (jsinterp.cpp:605)
==59269==    by 0x80C1FF8: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2682)
==59269==    by 0x80CD563: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:472)
==59269==    by 0x80CE129: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:670)
==59269==    by 0x806160C: JS_ExecuteScript (jsapi.cpp:5247)
==59269==    by 0x805030D: Process(JSContext*, JSObject*, char const*, bool) (js.cpp:478)

(twice)

==59269== Invalid read of size 1
==59269==    at 0x80F9700: Decompile(SprintStack*, unsigned char*, int) (jsopcode.cpp:5077)
==59269==    by 0x81004DD: DecompileCode(JSPrinter*, JSScript*, unsigned char*, unsigned int, unsigned int) (jsopcode.cpp:5404)
==59269==    by 0x81006F0: DecompileExpression(JSContext*, JSScript*, JSFunction*, unsigned char*) (jsopcode.cpp:5810)
==59269==    by 0x81009C1: js_DecompileValueGenerator (jsopcode.cpp:5699)
==59269==    by 0x807E97C: js_ReportValueErrorFlags(JSContext*, unsigned int, unsigned int, int, JS::Value const&, JSString*, char const*, char const*) (jsopcode.h:401)
==59269==    by 0x809A7BF: js_ReportIsNotFunction(JSContext*, JS::Value const*, unsigned int) (jsfun.cpp:1383)
==59269==    by 0x80CDAC5: js::InvokeConstructorKernel(JSContext*, js::CallArgs const&) (jsinterp.cpp:605)
==59269==    by 0x80C1FF8: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2682)
==59269==    by 0x80CD563: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:472)
==59269==    by 0x80CE129: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:670)
==59269==    by 0x806160C: JS_ExecuteScript (jsapi.cpp:5247)
==59269==    by 0x805030D: Process(JSContext*, JSObject*, char const*, bool) (js.cpp:478)
==59269==  Address 0x6ba1a88 is 0 bytes after a block of size 64 alloc'd
==59269==    at 0x48D1876: malloc (vg_replace_malloc.c:236)
==59269==    by 0x80F3D5F: InitSprintStack(JSContext*, SprintStack*, JSPrinter*, unsigned int) (Utility.h:173)
==59269==    by 0x8100480: DecompileCode(JSPrinter*, JSScript*, unsigned char*, unsigned int, unsigned int) (jsopcode.cpp:5379)
==59269==    by 0x81006F0: DecompileExpression(JSContext*, JSScript*, JSFunction*, unsigned char*) (jsopcode.cpp:5810)
==59269==    by 0x81009C1: js_DecompileValueGenerator (jsopcode.cpp:5699)
==59269==    by 0x807E97C: js_ReportValueErrorFlags(JSContext*, unsigned int, unsigned int, int, JS::Value const&, JSString*, char const*, char const*) (jsopcode.h:401)
==59269==    by 0x809A7BF: js_ReportIsNotFunction(JSContext*, JS::Value const*, unsigned int) (jsfun.cpp:1383)
==59269==    by 0x80CDAC5: js::InvokeConstructorKernel(JSContext*, js::CallArgs const&) (jsinterp.cpp:605)
==59269==    by 0x80C1FF8: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2682)
==59269==    by 0x80CD563: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:472)
==59269==    by 0x80CE129: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:670)
==59269==    by 0x806160C: JS_ExecuteScript (jsapi.cpp:5247)


The test does not always crash, but it has an 80-90% chance to do so. Marking s-s because this seems to be an out-of-bounds read. Assigned to luke for now, as he has been looking on this issue already (and provided the smaller/more reliable testcase :D).
(Assignee)

Comment 1

5 years ago
This crashes here:


              case JSOP_ENDINIT:
              {
                JSBool inArray;

                op = JSOP_NOP;           /* turn off parens */
                rval = PopStr(ss, op, &rvalpc);
                sn = js_GetSrcNote(jp->script, pc);

                /* Skip any #n= prefix to find the opening bracket. */
                for (xval = rval; *xval != '[' && *xval != '{'; xval++)
                    continue;

with an invalid read of *xval. rval ends up pointing to an empty string, which this loop reads past, so we can crash or read data we're not supposed to. I verified that we can step on the security bug by adding a bounds check to the loop. That leaves a bug, which is that we print out the wrong thing, but I'm not sure that we care that much about invalid decompiler messages for obscure code.
Whiteboard: js-triage-needed → [sg:high]
Keywords: sec-high
(Assignee)

Updated

5 years ago
Whiteboard: [sg:high] → [js:p1][sg:high]
(Assignee)

Comment 2

5 years ago
I can't seem to reproduce this any more. Gary, if you can show me how, I want to put in the bounds check and knock it out, otherwise we're working on removing this code so it should be taken care of that way.
(Assignee)

Comment 3

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

Decompiler's hopefully going away soon, so let's just close the hole. Christian, do you think we need a cover bug? It only reads data, and not necessarily anything interesting, but the bug is really obvious.
Assignee: luke → dmandelin
Attachment #637999 - Flags: review?(luke)

Updated

5 years ago
Attachment #637999 - Flags: review?(luke) → review+
status-firefox16: --- → affected
tracking-firefox16: --- → ?
(Assignee)

Comment 4

5 years ago
Terrence, can you sneak this into one of your landings in the next couple of days as cover?

Updated

5 years ago
tracking-firefox16: ? → +
This slipped for 16.  I had 5 patches out with 3 different people in the week leading up to the split and all of them (and some new ones) are still waiting for review.
(Assignee)

Comment 6

5 years ago
(In reply to Terrence Cole [:terrence] from comment #5)
> This slipped for 16.  I had 5 patches out with 3 different people in the
> week leading up to the split and all of them (and some new ones) are still
> waiting for review.

No biggie--it can still land for 16 later, and it was too late for 14 before I talked to you. Please do land it when it's convenient--it looks like decompiler removal has yet more problems to work through.
Pushed to 17 in in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/189816733310
Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/166cae54928d
Re-landed in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/139a8f2a8538
(Assignee)

Comment 9

5 years ago
Comment on attachment 637999 [details] [diff] [review]
Patch

I'm not sure if it's worth landing this on branches or not. It's only an sg:high (reads past the end of a buffer) and I don't know how likely it is to allow sensitive data to be read. Landing it there also exposes the bug more (we used a cover bug for inbound/central), so it may increase (hypothetical) risk on release branches. But it's a pretty simple, low-risk patch at least.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): decompiler
User impact if declined: leaves a read-past-the-buffer bug in the decompiler -- possible risk of data read by attackers
Testing completed (on m-c, etc.): landed on m-c a few days ago
Risk to taking this patch (and alternatives if risky): minimal
String or UUID changes made by this patch:
Attachment #637999 - Flags: approval-mozilla-beta?
Attachment #637999 - Flags: approval-mozilla-aurora?

Updated

5 years ago
status-firefox-esr10: --- → ?
Comment on attachment 637999 [details] [diff] [review]
Patch

[Triage Comment]
Low risk, sg:high fix. Please also prepare a patch for the ESR10 if affected. Approved for branches.
Attachment #637999 - Flags: approval-mozilla-beta?
Attachment #637999 - Flags: approval-mozilla-beta+
Attachment #637999 - Flags: approval-mozilla-aurora?
Attachment #637999 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2f7d9cc63a6a
https://hg.mozilla.org/releases/mozilla-beta/rev/90641bda34ec
status-firefox-esr10: ? → affected
status-firefox15: --- → affected
status-firefox17: --- → fixed
tracking-firefox-esr10: --- → ?
tracking-firefox15: --- → ?
(Assignee)

Comment 13

5 years ago
Thanks for landing, Terrence!
status-firefox15: affected → fixed
status-firefox16: affected → fixed
tracking-firefox15: ? → +
(Assignee)

Comment 14

5 years ago
Comment on attachment 637999 [details] [diff] [review]
Patch

ESR10 is affected, same patch applies.
Attachment #637999 - Flags: approval-mozilla-esr10?
tracking-firefox-esr10: ? → 15+
Comment on attachment 637999 [details] [diff] [review]
Patch

approving for ESR, please land in the next week to ensure this goes out alongside the fix with 15.
Attachment #637999 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 16

5 years ago
http://hg.mozilla.org/releases/mozilla-esr10/rev/53882eeb9637
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox-esr10: affected → fixed
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 17

5 years ago
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [js:p1][sg:high] → [js:p1][sg:high][advisory-tracking+]
What was the target milestone for this bug, Firefox 17?
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #18)
> What was the target milestone for this bug, Firefox 17?

Yes, see comment 6 and comment 7.

http://hg.mozilla.org/mozilla-central/rev/139a8f2a8538
Target Milestone: --- → mozilla17
Thanks Gary.
status-firefox17: fixed → verified
Keywords: verifyme
Group: core-security
(Reporter)

Comment 21

4 years ago
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.