Assertion failure: ptr->isTenured(), at jit/shared/Assembler-shared.h:189

VERIFIED FIXED in Firefox 34

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: decoder, Assigned: terrence)

Tracking

(Blocks 1 bug, {assertion, sec-high, testcase})

Trunk
mozilla36
ARM
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox32 wontfix, firefox33 wontfix, firefox34+ verified, firefox35+ verified, firefox36+ verified, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.2 fixed)

Details

(Whiteboard: [jsbugmon:update,testComment=6][adv-main34+][b2g-adv-main2.2-])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
The following testcase asserts on mozilla-central revision 41a54c8add09 (run with --fuzzing-safe --ion-eager):


function second() {
  var arr = {};
  var s = 'a|b|c';
  return s.replace(/[a-z]/g, function(a) { return arr[a]; }, 'g');
}
Object.defineProperty(
  Object.prototype, "b", {
    get: (function() {
            "use asm"
            return {}
        })
  }
);
second();
Reporter

Comment 1

5 years ago
Another issue found with the simulator build, marked as s-s because it involves the Assembler and the assertion sounds GC-related.
Flags: needinfo?(mrosenberg)
Keywords: sec-high
Reporter

Updated

5 years ago
Whiteboard: [jsbugmon:update,bisect]
Reporter

Updated

5 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Is there somebody who can look at this Jandem?
Flags: needinfo?(jdemooij)
(In reply to Andrew McCreight [:mccr8] from comment #2)
> Is there somebody who can look at this Jandem?

Hm this could be a duplicate of bug 1023158, let's try this again once that bug is fixed...
Flags: needinfo?(jdemooij)
See Also: → 1023158
Flags: needinfo?(mrosenberg)
Whiteboard: [jsbugmon:update] → [jsbugmon:update][dupe of bug 1023158?]
Group: javascript-core-security
Reporter

Updated

5 years ago
Whiteboard: [jsbugmon:update][dupe of bug 1023158?] → [dupe of bug 1023158?] [jsbugmon:update,ignore]
Reporter

Comment 4

5 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 76b55c0850ca).
Reporter

Updated

5 years ago
Whiteboard: [dupe of bug 1023158?] [jsbugmon:update,ignore] → [dupe of bug 1023158?] [jsbugmon:bisectfix]
Reporter

Updated

5 years ago
Whiteboard: [dupe of bug 1023158?] [jsbugmon:bisectfix] → [dupe of bug 1023158?] [jsbugmon:]
Reporter

Comment 5

5 years ago
JSBugMon: Fix Bisection requested, result:
Due to skipped revisions, the first good revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a0dd5a83ba36
user:        Jan de Mooij
date:        Thu Jul 24 11:56:43 2014 +0200
summary:     Bug 1031529 part 2 - Remove JS_THREADSAFE #ifdefs everywhere. r=bhackett

changeset:   https://hg.mozilla.org/mozilla-central/rev/6426fef52f51
user:        Jan de Mooij
date:        Thu Jul 24 11:56:45 2014 +0200
summary:     Bug 1031529 part 3 - Step defining JS_THREADSAFE, remove --disable-threadsafe. r=glandium

This iteration took 479.049 seconds to run.
Still happens with rev 7fc96293ada8 with these flags: --fuzzing-safe --ion-eager --no-threads

function second() {
  var arr = {};
  var s = 'a|b|c';
  return s.replace(/[a-z]/g, function(a) { return arr[a]; }, 'g');
}
Object.defineProperty(
  Object.prototype, "b", {
    get: (function() {
            "use asm"
            return {}
        })
  }
);
second();
Whiteboard: [dupe of bug 1023158?] [jsbugmon:] → [dupe of bug 1023158?] [jsbugmon:update,testComment=6]
Assignee

Comment 7

5 years ago
Posted patch bug_1013001-v0.diff (obsolete) — Splinter Review
Sorry I didn't see this bug sooner, as the fix is straightforward. Please do NI me on any like this in the future so that I don't miss them in the tide of bugmail.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8505876 - Flags: review?(jdemooij)
Comment on attachment 8505876 [details] [diff] [review]
bug_1013001-v0.diff

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

Looks good and thanks for fixing this, but see the question below.

::: js/src/jit-test/tests/gc/bug-1013001.js
@@ +1,1 @@
> +function second() {

If it's really a sec-high we should probably land without the testcase.

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +1178,5 @@
>      void pushValue(const Value &val) {
>          jsval_layout jv = JSVAL_TO_IMPL(val);
>          push(Imm32(jv.s.tag));
>          if (val.isMarkable())
> +            push(ImmMaybeNurseryPtr(reinterpret_cast<gc::Cell *>(val.toGCThing())));

Don't we need this on other platforms too?
Attachment #8505876 - Flags: review?(jdemooij)
Assignee

Comment 9

5 years ago
Comment on attachment 8505876 [details] [diff] [review]
bug_1013001-v0.diff

On further investigation, this is simply incomplete -- the same path is buggy in x86.
Assignee

Comment 10

5 years ago
Posted patch bug_1013001-v0.diff (obsolete) — Splinter Review
This re-implementation of ImmMaybeNurseryPtr strictly enforces the membrane, catching the misuse on the other two platforms as well as simplifying the rest of the code significantly.

This patch gives visibility of the raw pointer value in an ImmMaybeNurseryPtr to only AssemblerShared. This should make it impossible to do anything at all with a nursery pointer without first calling noteMaybeNurseryPtr. Between this, the isTenured assertion in ImmGCPtr, and the onMainThread assertion, it should now be basically impossible to use a nursery pointer incorrectly in the assembler in debug builds.

I've built and tested this with x86, x64, and arm-simulator builds. It will probably break mips and arm64 builds when we land it, but fixing these will be trivial.
Attachment #8506478 - Flags: review?(jdemooij)
Assignee

Updated

5 years ago
Attachment #8505876 - Attachment is obsolete: true
Assignee

Comment 11

5 years ago
Comment on attachment 8506478 [details] [diff] [review]
bug_1013001-v0.diff

Nope, it looks like this doesn't actually close the hole discussed bug 1023158. Next time's the charm.
Attachment #8506478 - Attachment is obsolete: true
Attachment #8506478 - Flags: review?(jdemooij)
Assignee

Comment 12

5 years ago
To address the case of writeValueRelocation(Value), I (1) moved the implementation from x86-shared.h to x64.h, as it isn't used on x86, and (2) handled the IsInsideNursery case manually. This is less bulletproof than the other changes here, but I think is as good as we're going to get without an enormous amount of work wrapping Value in an ImmGCValue/ImmMaybeNurseryValue equivalent.
Attachment #8507200 - Flags: review?(jdemooij)
Comment on attachment 8507200 [details] [diff] [review]
bug_1013001-v2.diff

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

Great refactoring. Thanks for fixing this!

::: js/src/jit/shared/Assembler-shared.h
@@ +223,2 @@
>  // Used for immediates which require relocation.
> +class ImmGCPtr

I wonder if we should rename this to ImmTenured(GC)Ptr or something as a followup?
Attachment #8507200 - Flags: review?(jdemooij) → review+
Assignee

Comment 14

5 years ago
Comment on attachment 8507200 [details] [diff] [review]
bug_1013001-v2.diff

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Probably not easily, although the fuzzing testcase is pretty small.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not really.

Which older supported branches are affected by this flaw?
Back to 33.

If not all supported branches, which bug introduced the flaw?
GGC.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be fairly easy to backport and low risk.

How likely is this patch to cause regressions; how much testing does it need?
It's mostly a type-system change, so it's fairly unlikely to cause regressions or need significant testing.
Attachment #8507200 - Flags: sec-approval?
Comment on attachment 8507200 [details] [diff] [review]
bug_1013001-v2.diff

Sec-approval+. Please nominate for Aurora and Beta as well so we can fix it there (or make new patches for these if the current one doesn't apply cleanly and nominate them).
Attachment #8507200 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/5df7f0bf77fc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Reporter

Updated

5 years ago
Status: RESOLVED → VERIFIED
Reporter

Comment 18

5 years ago
JSBugMon: This bug has been automatically verified fixed.
Assignee

Comment 19

5 years ago
Approval Request Comment
[Feature/regressing bug #]: GGC.
[User impact if declined]: A potentially exploitable sec-high in release browsers.
[Describe test coverage new/current, TBPL]: On m-i/m-c for several weeks.
[Risks and why]: Low: there isn't much that can go wrong other than build failures.
[String/UUID change made/needed]: None.
Attachment #8512135 - Flags: review+
Attachment #8512135 - Flags: approval-mozilla-beta?
Assignee

Comment 20

5 years ago
Comment on attachment 8507200 [details] [diff] [review]
bug_1013001-v2.diff

Approval Request Comment
Same as for beta.
Attachment #8507200 - Flags: approval-mozilla-aurora?
Attachment #8507200 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8512135 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter

Comment 22

5 years ago
JSBugMon: This bug has been automatically verified fixed on Fx34
JSBugMon: This bug has been automatically verified fixed on Fx35
Group: javascript-core-security
Whiteboard: [dupe of bug 1023158?] [jsbugmon:update,testComment=6] → [jsbugmon:update,testComment=6][adv-main34+]
Whiteboard: [jsbugmon:update,testComment=6][adv-main34+] → [jsbugmon:update,testComment=6][adv-main34+][b2g-adv-main2.2-]

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.