Closed Bug 1494752 Opened 6 years ago Closed 6 years ago

Crash [@ js::CheckTracedThing<JSString>] with OOM and invalid read

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox-esr60 64+ fixed
firefox62 --- wontfix
firefox63 - wontfix
firefox64 + verified
firefox65 + verified

People

(Reporter: decoder, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update][adv-main64+][adv-esr60.4+])

Crash Data

Attachments

(7 files, 4 obsolete files)

708 bytes, patch
jonco
: review+
Details | Diff | Splinter Review
1.96 KB, patch
jonco
: review+
Details | Diff | Splinter Review
3.14 KB, patch
jonco
: review+
Details | Diff | Splinter Review
7.78 KB, patch
jonco
: review+
Details | Diff | Splinter Review
11.87 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
12.47 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
16.04 KB, patch
Waldo
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
The following testcase crashes on mozilla-central revision 2e3e89c9c68c (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

var lfLogBuffer = `
 function evalWithCache(code, ctx) {
  ctx = Object.create(ctx, {});
  code = code instanceof Object ? code : cacheEntry(code);
  var incremental = ctx.incremental || false;
    ctx.global = newGlobal({ cloneSingletons: !incremental });
    ctx_save = Object.create(ctx, {saveBytecode: { value: true } });
  var res1 = evaluate(code, ctx_save);
  var res3 = evaluate(code, Object.create(ctx, {loadBytecode: { value: true } }));
 }
 var test = "new class extends class { } { constructor() { super(); } }()";
 evalWithCache(test, { assertEqBytecode : true });
`;
loadFile(lfLogBuffer);
function loadFile(lfVarx, lfForceModule = false) {
    try {
        oomTest(new Function(lfVarx));
    } catch (lfVare) {}
}


Backtrace:

received signal SIGSEGV, Segmentation fault.
js::CheckTracedThing<JSString> (trc=trc@entry=0x7fffffffafa8, thing=0xfffe4ccccccccccc) at js/src/gc/Marking.cpp:210
#0  js::CheckTracedThing<JSString> (trc=trc@entry=0x7fffffffafa8, thing=0xfffe4ccccccccccc) at js/src/gc/Marking.cpp:210
#1  0x0000555555893958 in DoCallback<JSString*> (trc=0x7fffffffafa0, thingp=0x7fffffffaf30, name=0x555556501d80 "scope name") at js/src/gc/Tracer.cpp:49
#2  0x00005555561b78aa in js::TraceManuallyBarrieredEdge<JSAtom*> (name=0x555556501d80 "scope name", thingp=0x7fffffffaf30, trc=0x7fffffffafa8) at js/src/gc/Tracer.h:187
#3  TraceNullableBindingNames (length=<optimized out>, names=0x7ffff4ab3e30, trc=0x7fffffffafa8) at js/src/gc/Marking.cpp:1235
#4  js::FunctionScope::Data::trace (this=this@entry=0x7ffff4ab3e00, trc=trc@entry=0x7fffffffafa8) at js/src/gc/Marking.cpp:1260
#5  0x0000555555f02d62 in js::GCManagedDeletePolicy<js::FunctionScope::Data>::operator() (this=<optimized out>, constPtr=0x7ffff4ab3e00) at js/src/gc/DeletePolicy.h:80
#6  DeleteScopeData<js::FunctionScope::Data> (data=0x7ffff4ab3e00) at js/src/vm/Scope.cpp:266
#7  mozilla::Result<mozilla::Ok, JS::TranscodeResult> js::Scope::XDRSizedBindingNames<js::FunctionScope, (js::XDRMode)1>(js::XDRState<(js::XDRMode)1>*, JS::Handle<js::FunctionScope*>, JS::MutableHandle<js::FunctionScope::Data*>)::{lambda()#1}::operator()() const (__closure=0x7fffffffaf90) at js/src/vm/Scope.cpp:296
#8  mozilla::ScopeExit<mozilla::Result<mozilla::Ok, JS::TranscodeResult> js::Scope::XDRSizedBindingNames<js::FunctionScope, (js::XDRMode)1>(js::XDRState<(js::XDRMode)1>*, JS::Handle<js::FunctionScope*>, JS::MutableHandle<js::FunctionScope::Data*>)::{lambda()#1}>::~ScopeExit() (this=0x7fffffffaf90, __in_chrg=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/instrumentation/none/type/debug/dist/include/mozilla/ScopeExit.h:113
#9  js::Scope::XDRSizedBindingNames<js::FunctionScope, (js::XDRMode)1> (xdr=xdr@entry=0x7fffffffb860, scope=..., data=...) at js/src/vm/Scope.cpp:299
#10 0x0000555555f0b7db in js::FunctionScope::XDR<(js::XDRMode)1> (xdr=xdr@entry=0x7fffffffb860, fun=..., fun@entry=..., enclosing=..., enclosing@entry=..., scope=...) at js/src/vm/Scope.cpp:782
#11 0x0000555555e6fa20 in js::XDRScript<(js::XDRMode)1> (xdr=xdr@entry=0x7fffffffb860, scriptEnclosingScope=scriptEnclosingScope@entry=..., sourceObjectArg=..., fun=..., fun@entry=..., scriptp=...) at js/src/vm/JSScript.cpp:780
#12 0x0000555555e70b37 in js::XDRInterpretedFunction<(js::XDRMode)1> (xdr=xdr@entry=0x7fffffffb860, enclosingScope=..., enclosingScope@entry=..., sourceObject=..., sourceObject@entry=..., objp=..., objp@entry=...) at js/src/vm/JSFunction.cpp:705
#13 0x0000555555e6fc74 in js::XDRScript<(js::XDRMode)1> (xdr=xdr@entry=0x7fffffffb860, scriptEnclosingScope=..., scriptEnclosingScope@entry=..., sourceObjectArg=..., fun=fun@entry=..., scriptp=...) at js/src/vm/JSScript.cpp:900
#14 0x0000555555ffcaf4 in js::XDRState<(js::XDRMode)1>::codeScript (this=this@entry=0x7fffffffb860, scriptp=...) at js/src/vm/Xdr.cpp:200
#15 0x0000555555c69a0f in JS::DecodeScript (cx=<optimized out>, buffer=..., scriptp=..., cursorIndex=<optimized out>) at js/src/jsapi.cpp:6950
#16 0x0000555555680848 in Evaluate (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/shell/js.cpp:2103
#17 0x00002db8d48a453b in ?? ()
[...]
#20 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0xfffe4ccccccccccc	-478507460408116
rcx	0x0	0
rdx	0x555556501d80	93825008672128
rsi	0xfffe4ccccccccccc	-478507460408116
rdi	0x7fffffffafa8	140737488334760
rbp	0x7fffffffaed0	140737488334544
rsp	0x7fffffffaeb0	140737488334512
r8	0x7fffffffab70	140737488333680
r9	0x6cb	1739
r10	0x7fffffffac50	140737488333904
r11	0x7ffff6cb9f90	140737333927824
r12	0x7fffffffafa8	140737488334760
r13	0x555556501d80	93825008672128
r14	0x7fffffffaf30	140737488334640
r15	0x7ffff5f16000	140737319624704
rip	0x5555561e0e39 <js::CheckTracedThing<JSString>(JSTracer*, JSString*)+41>
=> 0x5555561e0e39 <js::CheckTracedThing<JSString>(JSTracer*, JSString*)+41>:	testb  $0x1,(%rsi)
   0x5555561e0e3c <js::CheckTracedThing<JSString>(JSTracer*, JSString*)+44>:	jne    0x5555561e0fd8 <js::CheckTracedThing<JSString>(JSTracer*, JSString*)+456>


Marking s-s because the register we read contains a garbage address.
JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20151013053056" and the hash "8d9c20c241be7d7b3cfa90a3368a77db42172781".
The "bad" changeset has the timestamp "20151013054956" and the hash "d80f9d6921f8209ef01aa730be9a97ab727704d1".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8d9c20c241be7d7b3cfa90a3368a77db42172781&tochange=d80f9d6921f8209ef01aa730be9a97ab727704d1
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
We're poisoning the TrailingNamesArray, but this data is required to be valid in case we have to delete the scope data objects outside GC.  This happens because some fields (e.g. FunctionScope::Data::canonicalFunction) may have store buffer entries that point to them, and we have to remove these if the object is deleted at a time the store buffer is not known to be empty.  We do this by tracing the object when it is deleted.
Blocks: 1461821
Attached patch bug1494752-scope-oom (obsolete) — Splinter Review
Patch to construct the BindingNames when the TrailingNamesArray is created rather than leave them as uninitialized memory.
Assignee: nobody → jcoppeard
Attachment #9013323 - Flags: review?(jwalden+bmo)
Priority: -- → P1
So the posted patch works, but.  It adds a double-write to stuff that might matter for perf.  It gets rid of some poisoning that might detect misuses faster.  And it doesn't fix a similar problem with WasmInstanceScope::Data.  I think we can do better.

So, um, I went and wrote several patches to fix this without double-writing.  :-)  I also wrote a nifty little template function algorithm for filling trailing names in fresh Data instances in the parser -- that only adds five lines (and of course templates), but gets rid of some messy index-tracking/cursor-handling code in the process.  I think it's well worth it for clarity at point of use, but you tell me.  :-)  It'll be the last patch I post here.

Anyway.  This is just a first patch that makes one more place use placement-new, that already should have.
Attachment #9016154 - Flags: review?(jcoppeard)
This makes the length field in the XDR'd stuff always reflect exactly the names added so far, so if a trace happens, we'll trace exactly the good names, no more, no less.  This fixes this bug as originally reported.
Attachment #9016155 - Flags: review?(jcoppeard)
It'd be nice if someone wrote a test that would trigger the same problem with this code, as comment 0 found for the XDR stuff.  I definitely don't have time to ramp up on wasm now to do so myself.
Attachment #9016156 - Flags: review?(jcoppeard)
This is IMO surprisingly elegant for C++ template recursion.  And I think the concision of

        // The ordering here is important. See comments in GlobalScope.
        InitializeBindingData(bindings, numBindings,
                              vars,
                              &GlobalScope::Data::letStart, lets,
                              &GlobalScope::Data::constStart, consts);

is far better than

        // The ordering here is important. See comments in GlobalScope.
        BindingName* start = bindings->trailingNames.start();
        BindingName* cursor = start;

        cursor = FreshlyInitializeBindings(cursor, vars);

        bindings->letStart = cursor - start;
        cursor = FreshlyInitializeBindings(cursor, lets);

        bindings->constStart = cursor - start;
        Unused << FreshlyInitializeBindings(cursor, consts);

        bindings->length = numBindings;

where we're manually slinging around pointers, we have MOZ_MUST_USE on FreshlyInitializeBindings so we don't forget to use an advanced cursor every time, we gotta Unused the final call where the at-end cursor is never needed, and there's a ton of noise for every pair of member-field/copied-vector pairing being set.

But strictly speaking, this patch is not necessary to fix this bug and could be rejected if you were a hater.  :-)

Assuming all these patches are go, I probably would fold them all together into one for landing, as obfuscatory tactic and to not give away the game too much.  It probably would be reasonably non-obvious exactly where the problem was in all that.
Attachment #9016158 - Flags: review?(jcoppeard)
Comment on attachment 9013323 [details] [diff] [review]
bug1494752-scope-oom

I'm fine with your approach.  I'm not sure how much difference the double-write would make to performance, but it is better not to lose the poisoning.
Attachment #9013323 - Attachment is obsolete: true
Attachment #9013323 - Flags: review?(jwalden+bmo)
Attachment #9016154 - Flags: review?(jcoppeard) → review+
Comment on attachment 9016155 [details] [diff] [review]
Ensure Data::length always reflects the trailing names that have actually been initialiezd during XDR decoding

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

Please do land the test case at some point too.
Attachment #9016155 - Flags: review?(jcoppeard) → review+
Attachment #9016156 - Flags: review?(jcoppeard) → review+
Comment on attachment 9016158 [details] [diff] [review]
Infallibly initialize the various |Data::trailingNames| bindings using a variadic template function to abstract logic -- and that, because it does only infallible operations, clearly need update |Data::length| only once at its end

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

This is neat.  Thanks for the explanatory comments on InitializeBindingData.
Attachment #9016158 - Flags: review?(jcoppeard) → review+
Assignee: jcoppeard → jwalden+bmo
Attached patch Rollup patch for trunk (obsolete) — Splinter Review
[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Depends how easily you can trigger a GC at just the wrong time, then get memory ostensibly containing BindingNames to contain prior data of the right sort.  Probably requires some memory allocator awareness.  But it seems doable with good chops, and maybe really-doable if Metasploit sorts of things mean you don't have to deeply understand allocator internals to craft an attack.

The patch comments don't squarely point out the issue, but it's possible a not-deeply-educated-in-this-code person could pick out the salient lines -- places where we increment |data->length| a bunch of times, versus setting it just once -- and then craft an attack from that.  It's hard to say, given I understand the issue and am trying to place myself in the shoes of someone who does not.

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

Which older supported branches are affected by this flaw?: Everything that includes the frontend rewrite...so probably everything

If not all supported branches, which bug introduced the flaw?: Bug 1263355

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: I have a beta backport.  It wasn't too bad to make, as long as I generated a rollup patch with little enough context to apply to the old tree, and I regressed a few places in the beta-tree to Gecko bracing style.

There's a fair chance the patch would also apply to esr60, but I'm still cloning an esr60 to find out just how easily the patch would apply.

How likely is this patch to cause regressions; how much testing does it need?: It's generally fairly straightforward, IMO.  The trace-functions are all quite clear about tracing up to |data->length| (and have been since bug 1263355 landed), and this patchwork fairly clearly adds trailing names exactly in sync with length-updates, when intervening operations could be fallible.  I don't foresee regressions.  But this is a place where finickiness applies, so I would probably say we get a moderate amount of testing -- say, land a week before branch drop-dead date or so, ideally.  But I could take landing later in the cycle if it came to it.
Attachment #9016483 - Flags: sec-approval?
Attachment #9016483 - Flags: review+
Attached patch Beta rollup (obsolete) — Splinter Review
No particular interesting changes were required to make this backport, and a readthrough of the patch indicates all the things that need to change, were changed as needed, and correctly so.  So this is good for whenever after the trunk patch lands, more or less.
Attachment #9016484 - Flags: review+
Attached patch ESR60 rollup (obsolete) — Splinter Review
We don't have the TrailingNamesArray helper on ESR60, but what we do have is pretty similar, and it wasn't too bad to make the necessary adjustments.

The Parser.cpp changes required similar sorts of not-difficult dealing, because ESR60 uses PodCopy to fill in binding data rather than playing nice with C++, but it was pretty good.  Of note: I carefully wrote the InitializeBindingData calls by looking at the existing ESR60 code, because some of the vectors/saved-indexes stuff in 60 is different from what exists on trunk -- so we shouldn't have mistakes due to brain-off backporting there.
Attachment #9016495 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96e3d756e9d1d6e52ac9b8a9635ce825b7fe69d0 for a one-platform try-run of the trunk patch, with an obfuscatory commit message that hopefully doesn't clue anyone watching in as to it being a sec issue since bug 1136954 isn't fixt yet.  :-|
Comment on attachment 9016483 [details] [diff] [review]
Rollup patch for trunk

> I would probably say we get a moderate amount of testing -- say, land a
> week before branch drop-dead date or so, ideally.

We are basically already there if you were thinking this is safe enough to uplift to Fx63. RCs will be built next week so there's no "nightly bake time" or even "beta test" time if you land now (there will be a few days of beta folks getting the RC before the actual ship date).

So is this patch safe enough for that aggressive schedule or should we punt until Firefox 64?
Flags: needinfo?(jwalden+bmo)
Attachment #9016483 - Flags: sec-approval? → sec-approval+
Mm.  I want to say this is safe.

But also this is somewhat similar in nature to bug 1462939 -- which I patched, which had issues of doing it wrongly in the first version, then required a few followup fix rounds after.  Which means there's probably greater risk of issue from this being finicky, but also less risk because I have practice getting it right.  :-)  So maybe it's a wash, but having to weigh odds both directions ups the riskiness.

Let's play it safe and not rush it into the last RC.  Let me know when I should land this *after* the next few days, so there's a little time for issues to arise before it goes into betas or RCs.
Flags: needinfo?(jwalden+bmo)
Marking as wontfix for 63 as per the risk analysis in comment #17.
Attached patch Beta rollupSplinter Review
[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1263355

User impact if declined: GC unsafety, sec-critical.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: The test in comment 0 ought be reduced to a landable testcase, but it's difficult to do so to satisfaction to land -- and we can't land immediately anyway, so I haven't tried to run that to ground.  :-\

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): See comment 17.  We should land this so there's enough time to suss out issues once landed, but not too early because it reveals an exploitable issue.

String changes made/needed: N/A
Attachment #9016484 - Attachment is obsolete: true
Attachment #9022807 - Flags: review+
Attachment #9022807 - Flags: approval-mozilla-beta?
Attached patch ESR60 rollupSplinter Review
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: GC unsafety, sec-critical.

User impact if declined: GC unsafety, sec-critical.

Fix Landed on Version: Ready to land on trunk whenever safe to.

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): See comment 17, and the comment on the beta rollup as well.

String or UUID changes made by this patch: N/A
Attachment #9016495 - Attachment is obsolete: true
Attachment #9022808 - Flags: review+
Attachment #9022808 - Flags: approval-mozilla-esr60?
[Security Approval Request]

How easily could an exploit be constructed based on the patch?: See comment 12.

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

Which older supported branches are affected by this flaw?: ESR60, beta are all we care about now, correct?  They're affected

If not all supported branches, which bug introduced the flaw?: Bug 1263355

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?: See comment 17.  We should land later in a cycle, but not *too* late.  I leave the Goldilocks estimation to you.
Attachment #9016483 - Attachment is obsolete: true
Attachment #9022811 - Flags: sec-approval?
Attachment #9022811 - Flags: review+
Comment on attachment 9022811 [details] [diff] [review]
Rollup patch for trunk

sec-approval+ and beta approval. I'll let release management do ESR60.
Attachment #9022811 - Flags: sec-approval? → sec-approval+
Attachment #9022807 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Landed on trunk:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3c1bdec432c

Past history suggests there are branch checkin elves who will cover beta/esr60 when those are each ready, so I will leave them to it.
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e3c1bdec432c
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment on attachment 9022808 [details] [diff] [review]
ESR60 rollup

Approved for 60.4.0esr also.
Attachment #9022808 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main64+][adv-esr60.4+]
JSBugMon: This bug has been automatically verified fixed on Fx64
Group: core-security-release
Keywords: bugmon
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: