Christian Holler (:decoder) Reporter
	Description • 6 years ago

The following testcase crashes on mozilla-central revision 32c6c1848f14 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe):

let m = parseModule(`
  function f(x,y,z) {
    delete arguments[2];
    import.meta[2]
  }
  f(1,2,3)
`);
m.declarationInstantiation();
m.evaluation();


Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  JS::Value::setObject (obj=..., this=<optimized out>) at js/Value.h:490
#1  JS::ObjectValue (obj=...) at js/Value.h:1074
#2  js::jit::IonBuilder::jsop_importmeta (this=0x7fffffffc460) at js/src/jit/IonBuilder.cpp:13183
#3  0x0000000000754996 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffffffc460, op=op@entry=JSOP_IMPORTMETA) at js/src/jit/IonBuilder.cpp:2393
#4  0x00000000007559d7 in js::jit::IonBuilder::visitBlock (this=this@entry=0x7fffffffc460, cfgblock=cfgblock@entry=0x7ffff4af9070, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1572
#5  0x0000000000755cc2 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffffffc460) at js/src/jit/IonBuilder.cpp:1489
#6  0x0000000000756450 in js::jit::IonBuilder::build (this=this@entry=0x7fffffffc460) at js/src/jit/IonBuilder.cpp:864
#7  0x000000000075ed5f in js::jit::AnalyzeArgumentsUsage (cx=0x7ffff5f15000, scriptArg=<optimized out>) at js/src/jit/IonAnalysis.cpp:4496
#8  0x0000000000592b5c in JSScript::ensureHasAnalyzedArgsUsage (cx=<optimized out>, this=<optimized out>) at js/src/vm/JSScript-inl.h:203
#9  Interpret (cx=0x7ffff5f15000, state=...) at js/src/vm/Interpreter.cpp:3554
[...]
#28 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9669
rax	0x1ce8020	30310432
rbx	0x7fffffffc460	140737488340064
rcx	0xf07ab8	15760056
rdx	0x0	0
rsi	0x5	5
rdi	0x7ffff4d7c1a0	140737301168544
rbp	0x7fffffffc460	140737488340064
rsp	0x7fffffffc0c0	140737488339136
r8	0xfff8800000000002	-2111062325329918
r9	0x0	0
r10	0x1	1
r11	0x100	256
r12	0xf67920	16152864
r13	0xe8	232
r14	0x7ffff4af8020	140737298530336
r15	0x7ffff5f15000	140737319620608
rip	0x7222e1 <js::jit::IonBuilder::jsop_importmeta()+145>
=> 0x7222e1 <js::jit::IonBuilder::jsop_importmeta()+145>:	movl   $0x0,0x0
   0x7222ec <js::jit::IonBuilder::jsop_importmeta()+156>:	ud2

	Christian Holler (:decoder) Reporter
	Comment 1 • 6 years ago

This is an automated crash issue comment:

Summary: Crash [@ js::gc::detail::GetCellLocation]
Build version: mozilla-central revision 32c6c1848f14
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --enable-fuzzing
Runtime options: --fuzzing-safe

Testcase can be provided if necessary.

Backtrace:

==21844==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000fffe8 (pc 0x00000103c837 bp 0x7ffd16911b50 sp 0x7ffd16911b40 T0)
==21844==The signal is caused by a READ memory access.
    #1 0x103c836 in js::gc::IsInsideNursery(js::gc::Cell const*) dist/include/js/HeapAPI.h:486
    #2 0x103c836 in js::jit::IonBuilder::checkNurseryObject(JSObject*) js/src/jit/IonBuilder.cpp:13657
    #3 0x103c836 in js::jit::IonBuilder::constant(JS::Value const&) js/src/jit/IonBuilder.cpp:13676
    #4 0x1045538 in js::jit::IonBuilder::pushConstant(JS::Value const&) js/src/jit/IonBuilder.cpp:3255:19
    #5 0x1045538 in js::jit::IonBuilder::jsop_importmeta() js/src/jit/IonBuilder.cpp:13183
    #6 0x1045538 in js::jit::IonBuilder::inspectOpcode(JSOp) js/src/jit/IonBuilder.cpp:2393
    #7 0x103e3df in js::jit::IonBuilder::visitBlock(js::jit::CFGBlock const*, js::jit::MBasicBlock*) js/src/jit/IonBuilder.cpp:1572:9
    #8 0x1034352 in js::jit::IonBuilder::traverseBytecode() js/src/jit/IonBuilder.cpp:1489:9
    #9 0x102314e in js::jit::IonBuilder::build() js/src/jit/IonBuilder.cpp:864:5
    #10 0x1024825 in js::jit::AnalyzeArgumentsUsage(JSContext*, JSScript*) js/src/jit/IonAnalysis.cpp:4496:45
    #11 0x9da1fd in JSScript::ensureHasAnalyzedArgsUsage(JSContext*) js/src/vm/JSScript-inl.h:203:12
    #12 0x9da1fd in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3554
    #13 0x9ca8b8 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:429:12
    #14 0xa02da8 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:777:15
    #15 0xa03542 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:809:12
    #16 0xb0a638 in js::ModuleObject::execute(JSContext*, JS::Handle<js::ModuleObject*>, JS::MutableHandle<JS::Value>) js/src/builtin/ModuleObject.cpp:1105:12
    #17 0x1cb8517 in intrinsic_ExecuteModule(JSContext*, unsigned int, JS::Value*) js/src/vm/SelfHosting.cpp:2221:12
    #18 0x9fd0d6 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/vm/Interpreter.cpp:449:15
    #19 0x9fd0d6 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:537
    #20 0x9e5fbf in js::CallFromStack(JSContext*, JS::CallArgs const&) js/src/vm/Interpreter.cpp:594:12
    #21 0x9e5fbf in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3243
    #22 0x9ca8b8 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:429:12
    #23 0x9fdbee in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:561:15
    #24 0xda1bfa in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:2582:14
    #25 0x35ceb30ec8e0  (<unknown module>)


Marking s-s because this can also crash in GC with non-null crash address.

	Jon Coppeard (:jonco)
	Comment 2 • 6 years ago

I wrongly assumed that when IonBuilder saw bytecode it had already been compiled by Baseline.  The safe and easy thing to do to fix this is to check whether the meta object is null and abort.

I'm not sure whether the correct thing to do is to try and create the object here or not.  Can this analysis can run off-thread?

	Fuzzing Team
	Comment 3 • 6 years ago

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/847453f52aab
user:        Jon Coppeard
date:        Wed May 23 08:47:28 2018 +0100
summary:     Bug 1427610 - Support import.meta in the JITs r=jandem

This iteration took 245.065 seconds to run.

	Daniel Veditz [:dveditz]
	Comment 4 • 6 years ago

How do we reconcile the patch, which is basically a null check, with the non-null ASAN crash in comment 1?

	Jon Coppeard (:jonco)
	Comment 5 • 6 years ago

(In reply to Daniel Veditz [:dveditz] from comment #4)
> How do we reconcile the patch, which is basically a null check, with the
> non-null ASAN crash in comment 1?

js::gc::IsInsideNursery() is trying to access the chunk trailer for the chunk containing the pointer.  This is at offset 0xfffe8, hence this happens when it's passed a null pointer:

==21844==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000fffe8 (pc 0x00000103c837 bp 0x7ffd16911b50 sp 0x7ffd16911b40 T0)
==21844==The signal is caused by a READ memory access.
    #1 0x103c836 in js::gc::s(js::gc::Cell const*) dist/include/js/HeapAPI.h:486

	Fuzzing Team
	Comment 6 • 6 years ago

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 49b70f7e6817).

	Jan de Mooij [:jandem]
	Comment 7 • 6 years ago

Comment on attachment 9003501 [details] [diff] [review]
bug1485698-import-meta

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

::: js/src/jit/IonBuilder.cpp
@@ +13180,5 @@
> +
> +    // The meta object might not have been created yet if we are in an analysis
> +    // phase.
> +    if (!metaObject)
> +        return abort(AbortReason::Error);

Instead of aborting (I'm not sure how AbortReason::Error is treated by this analysis) you could do something like this at the start of the function to proceed without the object:

https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/js/src/jit/IonBuilder.cpp#9458-9465

	Jan de Mooij [:jandem]
	Comment 8 • 6 years ago

(In reply to Jon Coppeard (:jonco) from comment #2)
> Can this analysis can run off-thread?

It always runs on the main thread, but the arguments analysis only cares about how |arguments| is used so anything unrelated to that can just push a dummy value/constant as it will be ignored anyway.

	Jon Coppeard (:jonco)
	Comment 9 • 6 years ago

Updated patch with review feedback.

	Jon Coppeard (:jonco)
	Comment 10 • 6 years ago

Comment on attachment 9003775 [details] [diff] [review]
bug1485698-import-meta v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch? This is a null pointer dereference so I'm not sure if it's exploitable, but you can see how to do it fairly easily from the patch.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? As above, you can see from the patch how to get a null pointer dereference.

Which older supported branches are affected by this flaw? FF 62.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same patch should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions since it's such a localised change.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 6 years ago

We're about to build 62 RC1 and I don't think this is the kind of change we should be taking in a late RC (but feel free to disagree if you feel strongly otherwise).

	Al Billings [:abillings - ex-MoCo]
	Comment 12 • 6 years ago

This isn't going to get sec-approval until after 62 ships. It is too late in the cycle since we're building the release candidate and are out of betas.

We also need to figure out a security rating.

	Randell Jesup [:jesup] (needinfo me)
	Comment 13 • 6 years ago

Note: null-deref's I normally don't consider sec bugs at all, if they're only null-pointer derefs (and the fffe8 offset is a nullptr deref).  I would land this in 63 or in 64-and-uplift-to-63, and I'd unhide the bug.  IMHO

	Jon Coppeard (:jonco)
	Comment 14 • 6 years ago

(In reply to Al Billings [:abillings] from comment #12)
Since this is a null dereference and given comment 13 I'd at least like to land this on 63.  What do you think?

	Al Billings [:abillings - ex-MoCo]
	Comment 15 • 6 years ago

(In reply to Jon Coppeard (:jonco) from comment #14)
> (In reply to Al Billings [:abillings] from comment #12)
> Since this is a null dereference and given comment 13 I'd at least like to
> land this on 63.  What do you think?

I think that's fine. I hadn't looked at it in detail before. I'll clear the sec-approval.

	Jon Coppeard (:jonco)
	Comment 16 • 6 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 17 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/d86165aa128f

	Fuzzing Team
	Comment 18 • 6 years ago

JSBugMon: This bug has been automatically verified fixed.