Closed Bug 1342049 Opened 7 years ago Closed 7 years ago

Crash [@ js::jit::SnapshotIterator::fromInstructionResult]

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed
firefox55 + verified

People

(Reporter: decoder, Unassigned)

References

Details

(4 keywords, Whiteboard: [jsbugmon:][post-critsmash-triage])

Crash Data

Attachments

(4 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision 32dcdde1fc64 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --ion-eager --no-threads):

function assert(mustBeTrue, message) {};
assert.throws = function (expectedErrorConstructor, func, message) {
    try {
        func();
    } catch (thrown) {}
}
gczeal(9);
var nextItem;
var iterable = {};
iterable[Symbol.iterator] = function() {
  return {
    next: function() {
      return { value: nextItem, done: false };
    }
  };
};
assert.throws(TypeError, function() {
  new Map(iterable);
});
nextItem = true;
assert.throws(TypeError, function() {
  new Map(iterable);
});
new Map(iterable);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x00000000006462aa in js::jit::SnapshotIterator::fromInstructionResult (index=<optimized out>, this=0x7fffffffc340) at js/src/jit/JitFrames.cpp:2209
#0  0x00000000006462aa in js::jit::SnapshotIterator::fromInstructionResult (index=<optimized out>, this=0x7fffffffc340) at js/src/jit/JitFrames.cpp:2209
#1  js::jit::SnapshotIterator::allocationValue (this=this@entry=0x7fffffffc340, alloc=..., rm=rm@entry=js::jit::SnapshotIterator::RM_Normal) at js/src/jit/JitFrames.cpp:1929
#2  0x0000000000648dd1 in js::jit::SnapshotIterator::read (this=0x7fffffffc340) at js/src/jit/JitFrameIterator.h:540
#3  js::jit::CloseLiveIteratorIon (tn=<optimized out>, frame=..., cx=0x7ffff6926800) at js/src/jit/JitFrames.cpp:352
#4  js::jit::HandleExceptionIon (overrecursed=0x7fffffffc1af, rfe=0x7fffffffc7a0, frame=..., cx=0x7ffff6926800) at js/src/jit/JitFrames.cpp:449
#5  js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:859
#6  0x000029391ff4215b in ?? ()
[...]
#15 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffffffff	140737488355327
rcx	0x6462a0	6578848
rdx	0x2	2
rsi	0x7fffffffc280	140737488339584
rdi	0x7fffffffc340	140737488339776
rbp	0x5	5
rsp	0x7fffffffc128	140737488339240
r8	0x7fffffffc0d0	140737488339152
r9	0x2	2
r10	0x83	131
r11	0x0	0
r12	0x7fffffffc340	140737488339776
r13	0x8	8
r14	0x7ffff5bc6bc8	140737316154312
r15	0x7ffff6926800	140737330178048
rip	0x6462aa <js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod)+186>
=> 0x6462aa <js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod)+186>:	mov    (%rax),%rax
   0x6462ad <js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod)+189>:	mov    (%rax),%rax


This looks like a crash at NULL but the test involves gczeal and it only reproduces with --no-threads, so I'm marking this s-s until investigated.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/757b50c0ee48
user:        Shu-yu Guo
date:        Sat Jan 14 14:51:38 2017 -0800
summary:     Bug 1147371 - Implement IteratorClose for for-of. (r=arai)

changeset:   https://hg.mozilla.org/mozilla-central/rev/8ad6c93e5162
user:        Shu-yu Guo
date:        Sat Jan 14 14:51:38 2017 -0800
summary:     Bug 1147371 - Rename allowContentSpread to allowContentIter. (r=arai)

changeset:   https://hg.mozilla.org/mozilla-central/rev/d7d332a5b2e8
user:        Shu-yu Guo
date:        Sat Jan 14 14:51:38 2017 -0800
summary:     Bug 1147371 - Convert self-hosted code that need to call IteratorClose to use for-of. (r=arai)

changeset:   https://hg.mozilla.org/mozilla-central/rev/ce293b3c0a8b
user:        Shu-yu Guo
date:        Sat Jan 14 14:51:38 2017 -0800
summary:     Bug 1147371 - Implement IteratorClose for array destructuring. (r=arai)

changeset:   https://hg.mozilla.org/mozilla-central/rev/e0dc4150f8ac
user:        Shu-yu Guo
date:        Sat Jan 14 14:51:39 2017 -0800
summary:     Bug 1147371 - Implement calling IteratorClose and "return" on iterators in yield*. (r=jandem)

This iteration took 216.728 seconds to run.
NI from shu based on comment 1.
Flags: needinfo?(shu)
JSTRY_ITERCLOSE is going away after bug 1331092 which replaces it with explicit try-catches. Going to wait for that to land.
Flags: needinfo?(shu)
Depends on: 1331092
(In reply to Shu-yu Guo [:shu] from comment #3)
> JSTRY_ITERCLOSE is going away after bug 1331092 which replaces it with
> explicit try-catches. Going to wait for that to land.

But will that be something we can uplift to 53?
Tracking 53/54+ for this security regression issue.
(In reply to Jan de Mooij [:jandem] from comment #4)
> (In reply to Shu-yu Guo [:shu] from comment #3)
> > JSTRY_ITERCLOSE is going away after bug 1331092 which replaces it with
> > explicit try-catches. Going to wait for that to land.
> 
> But will that be something we can uplift to 53?

Not the whole bug, but the part that redoes the ITERCLOSE stuff should be.
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 7ef1e9abd296).
Does comment 7 mean we can close this, or do you think it's still hanging around and just intermittent?
Flags: needinfo?(choller)
Waiting for fix bisection before closing.
Flags: needinfo?(choller)
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
How long until the bisectfix is run?
Flags: needinfo?(choller)
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:]
JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ba3d6be51e34
user:        Tooru Fujisawa
date:        Sun Feb 26 14:02:36 2017 +0900
summary:     Bug 1342553 - Part 0.1: Use try-catch for IteratorClose in for-of. r=shu

This iteration took 224.974 seconds to run.
Arai-san, is bug 1342553 a likely fix?
Flags: needinfo?(choller) → needinfo?(arai.unmht)
yes
but I'm not sure if bug 1342553 patch is stable enough to uplift...
anyway, I'll prepare patch (with folding other 2 patches that blocked it).
Flags: needinfo?(arai.unmht)
that patch can be applied almost cleanly by also uplifting bug 1322069 and bug 1338796
the main patches are bug 1342553, bug 1343072, and bug 1344753.
any thoughts on uplifting bug 1322069 and bug 1338796 too?
Flags: needinfo?(shu)
(In reply to Tooru Fujisawa [:arai] from comment #15)
> any thoughts on uplifting bug 1322069 and bug 1338796 too?

Can we create a rollup bug for a batch uplift, instead of bug-by-bug?
Flags: needinfo?(shu)
yeah, I can fold them into 1 (or 2 for pre-req and bug 1342553+fix ?)
do you mean yet another bug than this bug?
so far, here's raw not-folded patches:

73193fbc03ec: Bug 1322069 - Add TryEmitter. r=shu
697c73202fb6: Bug 1338796 - Do not call iterator.return if iterator.throw is present in yield*. (r=arai)
ba3d6be51e34: Bug 1342553 - Part 0.1: Use try-catch for IteratorClose in for-of. r=shu
712e84866cf5: Bug 1342553 - Part 0.2: Support JSOP_CHECKISCALLABLE in JIT. r=shu
2e920e33d89a: Bug 1343072 - Update HasLiveStackValueAtDepth to follow the change in JSTRY_FOR_OF r=shu
047b8fb1c6a3: Bug 1344753 - Update for-of stack depth in ControlFlowGenerator::processWhileOrForInLoop. r=jandem

rabsed on mozilla-beta e2752c5516c1
Just a reminder we want to get this into 53 (which you're working on already). 
Also - what security level?  It hasn't been clear from the initial report

 Thanks
Flags: needinfo?(arai.unmht)
I haven't investigated about the security details, but the patch above replaces most part of the implementation around the crash.

forwarding the ni? about the security-level (and also how to proceed from the above patch series) to shu.
if it's okay with the above patches (folded or not), I would be able to ask approval this weekend.
if it's urgent, feel free to take this over.
Flags: needinfo?(shu)
(In reply to Tooru Fujisawa [:arai] (almost away until April) from comment #21)
> I haven't investigated about the security details, but the patch above
> replaces most part of the implementation around the crash.
> 
> forwarding the ni? about the security-level (and also how to proceed from
> the above patch series) to shu.

The actual crash is indexing with an uint32 a nullptr as base (that is, 0x0+index). I guess that in theory, index could be made very large to compute a legitimate address? I don't know how that would be done. In practice I don't think this would ever result in reading a legitimate address, I think these indices are usually like <10?

To trigger the crash requires throwing from inside a for-of loop while executing in Ion, optimized in a certain way (I didn't debug exactly what Ion optimization caused this).

Given those things, I'd think low or medium?
Flags: needinfo?(shu)
Attachment #8846239 - Attachment is obsolete: true
Flags: needinfo?(arai.unmht)
Attachment #8848824 - Flags: review+
Attachment #8848825 - Attachment description: Do not call iterator.return if iterator.throw is present in yield*. (r=arai) → (mozilla-beta) Do not call iterator.return if iterator.throw is present in yield*. (r=arai)
Attachment #8848826 - Attachment description: Part 0.1: Use try-catch for IteratorClose in for-of. r=shu → (mozilla-beta) Part 0.1: Use try-catch for IteratorClose in for-of. r=shu
Comment on attachment 8848824 [details] [diff] [review]
(mozilla-beta) Add TryEmitter. r=shu

Approval Request Comment
> [Feature/Bug causing the regression]:
bug 1147371

> [User impact if declined]:
crash by executing JavaScript

> [Is this code covered by automated tests?]:
not by these patch series (all patches are backport).
if required, I'll prepare.

> [Has the fix been verified in Nightly?]:
yes (comment #11)

> [Needs manual test from QE? If yes, steps to reproduce]:
no

> [List of other uplifts needed for the feature/fix]:
none

all required patches are attached here.
these patch series are consist of the following bug patches:
  * bug 1322069
    required for bug 1342553 patch
  * bug 1338796
    required for bug 1342553 patch
  * bug 1342553
    the main patch that replaces the implemetation
  * bug 1343072
    fix for bug 1342553 patch
  * bug 1344753
    fix for bug 1342553 patch

> [Is the change risky?]:
less risky.

> [Why is the change risky/not risky?]:
just a backport from nightly/aurora

> [String changes made/needed]:
none
Attachment #8848824 - Flags: approval-mozilla-beta?
btw, these patches don't need sec-approval, right?
they're simply backports of other open bugs.
(In reply to Tooru Fujisawa [:arai] (almost away until April) from comment #28)
> btw, these patches don't need sec-approval, right?
> they're simply backports of other open bugs.

Correct.  If the original patch has already landed, then uplifts don't need separate approval.
52 was marked as affected but ESR52 was marked unaffected. That's inconsistent and probably wrong. 52 is either affected or not across the board.

I'm making this a sec-moderate based on comment 22.
Oh, my bad. I misread and 52 is unaffected.
Do you feel strongly this needs to be in 53? I would prefer to keep it in 55 or 54. 

Did this land on mozilla-central yet?
Flags: needinfo?(shu)
Flags: needinfo?(arai.unmht)
Jesup, you still want this for 53?
Flags: needinfo?(rjesup)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #32)
> Do you feel strongly this needs to be in 53? I would prefer to keep it in 55
> or 54. 
> 
> Did this land on mozilla-central yet?

all those patches already landed to 54, and attached patches are uplift for 53.
I'd forward the former question to shu.
Flags: needinfo?(arai.unmht)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #32)
> Do you feel strongly this needs to be in 53? I would prefer to keep it in 55
> or 54. 

I feel that given this can be triggered purely with reasonable code from content JS (albeit under some specific circumstances -- throwing while inside the highest tier JIT), we should take this for 53.
Flags: needinfo?(shu)
If shu is correct in comment 22, and index can't be an arbitrary (and content-influenced) number, then this is sec-moderate (I agree with al), and as a sec-moderate with non-trivial patches I don't see a need to uplift to 53 unless this is causing instability in the field (or on automation).  content JS forcing a DoS-crash isn't worth an uplift.  shu - please verify the index bounds you mentioned in comment 22, since you seemed unsure there
Flags: needinfo?(rjesup) → needinfo?(shu)
Redirecting NI to nbp, who knows that code better than I do.

Nicolas, please see comment 22 and comment 36.
Flags: needinfo?(shu) → needinfo?(nicolas.b.pierron)
This crash is located inside "RInstructionResults::operator []", the vector which is indexed has 64 bits elements.
The index is technically not limited but practically inefficient.

The methods to increase the index are:
 - Rely on the number of formal arguments (cap to 127) or functions inlined by IonBuilder.
 - Add local variable to the body of the function (no cap).

To make them part of the RInstructionResults vector, in order to increase the index, each value should have operations which are considered dead for some branches, but used by others which are removed.

So even if this index is not cap, using this technique to address random memory is unpractical, unless one has the control over what data is going to be allocated in the first pages.

This bug happens because we attempt to read the wrong slot number, which happen to be a RecoverInstruction when a lambda is inlined.  Any other type of data would only read the wrong information, which would be a JSFunction pointer, on which the CloseIterator functions have no effect.

One thing I do not understand, which might be worth a follow-up bug, is why do not we evaluate RInstructions when we are under HandleException.

 --------------

A similar bug is possible and can be used to forge a pointer to access random memory!

If there is any frame slot, such as local variables, a value can be stored as a double in a register, right before the expected slot of the iterator.

Thus, one might have control over the pointer which is being manipulated by the cast done at:
  http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/js/src/jit/JitFrames.cpp#352
Flags: needinfo?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #38)

> A similar bug is possible and can be used to forge a pointer to access
> random memory!
> 
> If there is any frame slot, such as local variables, a value can be stored
> as a double in a register, right before the expected slot of the iterator.
> 
> Thus, one might have control over the pointer which is being manipulated by
> the cast done at:
>  
> http://searchfox.org/mozilla-central/rev/
> 7419b368156a6efa24777b21b0e5706be89a9c2f/js/src/jit/JitFrames.cpp#352

There's a loop right before that line that skips all the locals and arguments. How can a local be reached via the |si.read()| after the skips?
Priority: -- → P1
Per comment 38 by nbp, I think we need to bump this to sec-high.  nbp, it wasn't 100% clear to me how real the arbitrary pointer part of this was; if it's really possible, this is sec-critical IMHO.

If this should be sec-high or sec-critical, we'll want that 53 uplift soon.

Also: arai is away at the moment; can someone else shepherd this until arai is back?
Flags: needinfo?(shu)
Flags: needinfo?(nicolas.b.pierron)
Keywords: sec-moderatesec-high
I guess we should err on the side of caution and uplift. 53 is beta, right?
Flags: needinfo?(shu)
Not sure what else needs shepherding--looks like arai has attached all the required patches and has requested approval-beta for them as well.
(In reply to Shu-yu Guo [:shu] from comment #39)
> (In reply to Nicolas B. Pierron [:nbp] from comment #38)
> > If there is any frame slot, such as local variables, a value can be stored
> > as a double in a register, right before the expected slot of the iterator.
> > 
> > Thus, one might have control over the pointer which is being manipulated by
> > the cast done at:
> >  
> > http://searchfox.org/mozilla-central/rev/
> > 7419b368156a6efa24777b21b0e5706be89a9c2f/js/src/jit/JitFrames.cpp#352
> 
> There's a loop right before that line that skips all the locals and
> arguments. How can a local be reached via the |si.read()| after the skips?

Iterators are not supposed to be RecoverInstruction, as far as I know, as the CloseIterator is technically escaping it. (Otherwise we should fix the fact the we do not compute recover instruction results).

Thus, from what I understand the only possible way to get a RecoverInstruction from this si.read() call, is that we have an off-by-one issue.  Knowing that all iterators are at the end of the frame, this off-by-one is certainly towards the arguments / local slots.  Meaning that we can add additional locals with arbitrary values.

If this is indeed an off-by-one, then the fact that this is a recover instruction read is just one possibility and the snapshot could have contained anything such as a double value.  Reading a double value out of the stack or a register should work and not fail with the SEGV that we saw, but go through the JSObject cast and flow in any of the CloseIterator functions.
Flags: needinfo?(nicolas.b.pierron)
OK, can you ask for sec-approval since we re-rated the bug? Sorry for the delay. This can still potentially make the beta 8 build tomorrow if we can get sec approval and land it on m-c in time. If not then we can take it for beta 9 next monday.
Flags: needinfo?(shu)
Flags: needinfo?(shu)
Attachment #8848824 - Flags: approval-mozilla-beta?
Comment on attachment 8848824 [details] [diff] [review]
(mozilla-beta) Add TryEmitter. r=shu

Approval Request Comment (modified from arai's original)
> [Feature/Bug causing the regression]:
bug 1147371

> [User impact if declined]:
Crash by executing JavaScript that throws during iteration that uses the ES6 Iterator protocol while inside Ion. Possibly exploitable.

> [Is this code covered by automated tests?]:
not by these patch series (all patches are backport).

> [Has the fix been verified in Nightly?]:
yes (comment #11)

> [Needs manual test from QE? If yes, steps to reproduce]:
no

> [List of other uplifts needed for the feature/fix]:
none

all required patches are attached here.
these patch series are consist of the following bug patches:
  * bug 1322069
    required for bug 1342553 patch
  * bug 1338796
    required for bug 1342553 patch
  * bug 1342553
    the main patch that replaces the implemetation
  * bug 1343072
    fix for bug 1342553 patch
  * bug 1344753
    fix for bug 1342553 patch

> [Is the change risky?]
No.

> [Why is the change risky/not risky?]:
Bug fix backport from nightly/aurora, and while we believe it is maybe exploitable, the patch series does not make it obvious that it is a security issue or how to exploit.


> [String changes made/needed]:
none
Attachment #8848824 - Flags: approval-mozilla-beta?
Comment on attachment 8848825 [details] [diff] [review]
(mozilla-beta) Do not call iterator.return if iterator.throw is present in yield*. (r=arai)

See comment 45.
Attachment #8848825 - Flags: approval-mozilla-beta?
Comment on attachment 8848826 [details] [diff] [review]
(mozilla-beta) Part 0.1: Use try-catch for IteratorClose in for-of. r=shu

See comment 45.
Attachment #8848826 - Flags: approval-mozilla-beta?
Comment on attachment 8848827 [details] [diff] [review]
(mozilla-beta) Part 0.2: Support JSOP_CHECKISCALLABLE in JIT. r=shu

See comment 45.
Attachment #8848827 - Flags: approval-mozilla-beta?
Comment on attachment 8848824 [details] [diff] [review]
(mozilla-beta) Add TryEmitter. r=shu

Fix for a sec-high or critical issue. These patches already landed on m-c (in other bugs) with sec approval. This should land for beta 9 on Monday.
Attachment #8848824 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8848825 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8848826 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8848827 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [jsbugmon:] → [jsbugmon:][post-critsmash-triage]
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: core-security-release
Assignee: arai.unmht → nobody
Keywords: bugmon
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: