Closed Bug 1254164 Opened 4 years ago Closed 4 years ago

Assertion failure: !IsUninitializedLexical(obj.aliasedVar(sc)), at js/src/vm/Interpreter-inl.h:245

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + verified
firefox48 + verified
firefox-esr38 46+ fixed
firefox-esr45 46+ fixed

People

(Reporter: decoder, Assigned: shu)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update][adv-main46+][adv-esr45.1+][adv-esr38.8+])

Attachments

(2 files)

The following testcase crashes on mozilla-central revision b6acf4d4fc20 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2):

var s = '';
for (var i = 0; i < 70000; i++)
    s += 'function x' + i + '() { x' + i + '(); }\n';
eval("(function() { " + s + " })();");



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000467d12 in js::SetAliasedVarOperation (checkLexical=js::CheckLexical, val=..., sc=..., obj=..., pc=0x7fffef2b1ff1 "\211", script=0x7ffff7e6e300, cx=0x7ffff6907800) at js/src/vm/Interpreter-inl.h:245
#0  0x0000000000467d12 in js::SetAliasedVarOperation (checkLexical=js::CheckLexical, val=..., sc=..., obj=..., pc=0x7fffef2b1ff1 "\211", script=0x7ffff7e6e300, cx=0x7ffff6907800) at js/src/vm/Interpreter-inl.h:245
#1  0x0000000000ab3fc5 in SetAliasedVarOperation (checkLexical=js::CheckLexical, val=..., sc=..., obj=..., pc=0x7fffef2b1ff1 "\211", script=0x7ffff7e6e300, cx=0x7ffff6907800) at js/src/vm/Interpreter-inl.h:245
#2  Interpret (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:3166
#3  0x0000000000ab8828 in js::RunScript (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:428
#4  0x0000000000aba373 in js::ExecuteKernel (cx=cx@entry=0x7ffff6907800, script=..., script@entry=..., scopeChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=<optimized out>) at js/src/vm/Interpreter.cpp:684
#5  0x00000000005e6e7d in EvalKernel (cx=cx@entry=0x7ffff6907800, args=..., evalType=evalType@entry=DIRECT_EVAL, caller=..., scopeobj=..., scopeobj@entry=..., pc=<optimized out>) at js/src/builtin/Eval.cpp:332
#6  0x00000000005e7806 in js::DirectEval (cx=cx@entry=0x7ffff6907800, args=...) at js/src/builtin/Eval.cpp:439
#7  0x00000000006152bf in js::jit::DoCallFallback (cx=0x7ffff6907800, frame=0x7fffffffc548, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffc4f8, res=...) at js/src/jit/BaselineIC.cpp:6125
#8  0x00007ffff7ff1abf in ?? ()
[...]
#30 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff6907800	140737330051072
rcx	0x7ffff6ca588d	140737333844109
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffb080	140737488334976
rsp	0x7fffffffb080	140737488334976
r8	0x7ffff7fdf7c0	140737354004416
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffae40	140737488334400
r11	0x7ffff6c27ee0	140737333329632
r12	0x7ffff7e69100	140737352470784
r13	0x7ffff6907830	140737330051120
r14	0x1be1d20	29236512
r15	0x4000000	67108864
rip	0x467d12 <js::SetAliasedVarOperation(js::MaybeCheckLexical, JS::Value const&, js::ScopeCoordinate, js::ScopeObject&, jsbytecode*, JSScript*, JSContext*)+28>
=> 0x467d12 <js::SetAliasedVarOperation(js::MaybeCheckLexical, JS::Value const&, js::ScopeCoordinate, js::ScopeObject&, jsbytecode*, JSScript*, JSContext*)+28>:	movl   $0xf5,0x0
   0x467d1d <js::SetAliasedVarOperation(js::MaybeCheckLexical, JS::Value const&, js::ScopeCoordinate, js::ScopeObject&, jsbytecode*, JSScript*, JSContext*)+39>:	callq  0x4a6f30 <abort()>


Note: This test might run up to 20 seconds. I'm marking this s-s until investigated because I remember that at some point we had a security bug with this assertion.
Assignee: nobody → jorendorff
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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/0fd815999686
user:        Shu-yu Guo
date:        Wed Oct 29 19:41:42 2014 -0700
summary:     Bug 1089761 - Fix initializing lexicals to throw on touch on CallObject. (r=jandem,Waldo)

This iteration took 181.768 seconds to run.
Flags: needinfo?(shu)
I haven't had time to track this down yet, but I can report that it happens when initializing the 65534th (or thereabouts) function, and the deal is, the CallObject's slots were only initialized to UndefinedValue from slot 0 to slot 65535 (inclusive). The next slot is still poisoned.

Not sure if this is security-sensitive.
I don't think this is s-s. The worst that can happen is we crash on accessing a magic value as a pointer, in this case, JS_UNINITIALIZED_LEXICAL, which is 0x10 or something.
Flags: needinfo?(shu)
Attachment #8731910 - Flags: review?(jwalden+bmo)
If I'm reading the code right, on little-endian systems Value is going to be { JSWhyMagic; uint32_t tag }.  The latter is going to be a constant.  The former is also a constant.  If we access the overall thing as a pointer, it's going to be a constant pointer.  But the combo of the two isn't going to produce a low-valued pointer, so *in theory* the overall value could be addressable memory.  Right?  Unless we *know* that overall combo, interpreted as a pointer, is low, I don't think we're quite out of the woods here.
Comment on attachment 8731910 [details] [diff] [review]
Make aliasedBodyLevelLexicalBegin a uint32.

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

::: js/src/jit-test/tests/parser/bug-1254164.js
@@ +1,1 @@
> +// |jit-test| slow;

How slow is this?  I wouldn't expect a function with 70k nested functions would take that long to parse/execute.  I'd say you should be hitting at least 30s or so in a debug build (if not more) before applying this.

::: js/src/jsscript.cpp
@@ +130,5 @@
>      // aliased variables. While the debugger may observe any scope object at
>      // any time, such accesses are mediated by DebugScopeProxy (see
>      // DebugScopeProxy::handleUnaliasedAccess).
>      uint32_t nslots = CallObject::RESERVED_SLOTS;
> +    uint32_t aliasedBodyLevelLexicalBegin = LOCALNO_LIMIT;

A comment like "// an impossible slot number, such that no lexicals will be recognized" or something would be nice here, referring to the frontend's restriction on the number of locals in a function, and referring to initAliasedLexicalsToThrowOnTouch, would be nice.  Not sure if comment should be in this, or in a separate patch.  I might go for separate patch, for paranoia.

::: js/src/jsscript.h
@@ +231,2 @@
>      uint16_t numUnaliasedBodyLevelLexicals_;
> +    uint32_t aliasedBodyLevelLexicalBegin_;

Not here, but we should probably reorganize these from biggest to smallest type at some point.  Given we had five 16-bits in a row, this shouldn't throw off existing alignment at all, I'll assume.

@@ +357,5 @@
>  };
>  
> +// If this fails, add/remove padding within Bindings.
> +static_assert(sizeof(Bindings) % js::gc::CellSize == 0,
> +              "Size of Bindings must be an integral multiple of js::gc::CellSize");

\o/ and /o\ we didn't have this earlier.
Attachment #8731910 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> Comment on attachment 8731910 [details] [diff] [review]
> Make aliasedBodyLevelLexicalBegin a uint32.
> 
> Review of attachment 8731910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/tests/parser/bug-1254164.js
> @@ +1,1 @@
> > +// |jit-test| slow;
> 
> How slow is this?  I wouldn't expect a function with 70k nested functions
> would take that long to parse/execute.  I'd say you should be hitting at
> least 30s or so in a debug build (if not more) before applying this.
> 

On what machine though? It's less than 30s on mine but it'll be worse on our CI instances.

> ::: js/src/jsscript.cpp
> @@ +130,5 @@
> >      // aliased variables. While the debugger may observe any scope object at
> >      // any time, such accesses are mediated by DebugScopeProxy (see
> >      // DebugScopeProxy::handleUnaliasedAccess).
> >      uint32_t nslots = CallObject::RESERVED_SLOTS;
> > +    uint32_t aliasedBodyLevelLexicalBegin = LOCALNO_LIMIT;
> 
> A comment like "// an impossible slot number, such that no lexicals will be
> recognized" or something would be nice here, referring to the frontend's
> restriction on the number of locals in a function, and referring to
> initAliasedLexicalsToThrowOnTouch, would be nice.  Not sure if comment
> should be in this, or in a separate patch.  I might go for separate patch,
> for paranoia.

The sentinel logic is already there, just the sentinel was wrong. I think the comment is fine.

> 
> ::: js/src/jsscript.h
> @@ +231,2 @@
> >      uint16_t numUnaliasedBodyLevelLexicals_;
> > +    uint32_t aliasedBodyLevelLexicalBegin_;
> 
> Not here, but we should probably reorganize these from biggest to smallest
> type at some point.  Given we had five 16-bits in a row, this shouldn't
> throw off existing alignment at all, I'll assume.
> 
> @@ +357,5 @@
> >  };
> >  
> > +// If this fails, add/remove padding within Bindings.
> > +static_assert(sizeof(Bindings) % js::gc::CellSize == 0,
> > +              "Size of Bindings must be an integral multiple of js::gc::CellSize");
> 
> \o/ and /o\ we didn't have this earlier.

Yeah :(
Comment on attachment 8731910 [details] [diff] [review]
Make aliasedBodyLevelLexicalBegin a uint32.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Don't know. I don't think it's exploitable by itself but could be used as a widget maybe?

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

I'm unclear on whether it's a security problem. But the patch itself makes clear what the problem is.

Which older supported branches are affected by this flaw?

All branches.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

No, and haven't looked. Should be easy.

How likely is this patch to cause regressions; how much testing does it need?

Very unlikely to cause regressions.
Attachment #8731910 - Flags: sec-approval?
Thank you for stealing.
Sec-approval+ for trunk. We'll want Aurora and Beta patches made and nominated for sure. We'll probably want patches on both of the ESR branches as well.
Attachment #8731910 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/3a6988962137
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Comment on attachment 8731910 [details] [diff] [review]
Make aliasedBodyLevelLexicalBegin a uint32.

Approval Request Comment
[Feature/regressing bug #]: 1089761
[User impact if declined]: Crashes when trying to create scripts with more than 2^15 block-local bindings.
[Describe test coverage new/current, TreeHerder]: on TH.
[Risks and why]: low, bug fix.
[String/UUID change made/needed]: none.
Attachment #8731910 - Flags: approval-mozilla-beta?
Attachment #8731910 - Flags: approval-mozilla-aurora?
Shu, could you also nominate the patch for uplift to esr38 and esr45? Thanks!
Flags: needinfo?(shu)
Comment on attachment 8731910 [details] [diff] [review]
Make aliasedBodyLevelLexicalBegin a uint32.

Crash fix, sec-high, please uplift to aurora and beta.
Attachment #8731910 - Flags: approval-mozilla-beta?
Attachment #8731910 - Flags: approval-mozilla-beta+
Attachment #8731910 - Flags: approval-mozilla-aurora?
Attachment #8731910 - Flags: approval-mozilla-aurora+
JSBugMon: This bug has been automatically verified fixed on Fx47
Comment on attachment 8731910 [details] [diff] [review]
Make aliasedBodyLevelLexicalBegin a uint32.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: see comment 8
Fix Landed on Version: nightly, aurora, beta
Risk to taking this patch (and alternatives if risky): see comment 8
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(shu)
Attachment #8731910 - Flags: approval-mozilla-esr45?
Attachment #8731910 - Flags: approval-mozilla-esr38?
Comment on attachment 8731910 [details] [diff] [review]
Make aliasedBodyLevelLexicalBegin a uint32.

Sec-high fix that also needs to land on ESR branches.
Attachment #8731910 - Flags: approval-mozilla-esr45?
Attachment #8731910 - Flags: approval-mozilla-esr45+
Attachment #8731910 - Flags: approval-mozilla-esr38?
Attachment #8731910 - Flags: approval-mozilla-esr38+
has problems uplifting to esr38 

grafting 336841:aa0b35910b3a "Bug 1254164 - Make aliasedBodyLevelLexicalBegin a uint32. r=Waldo, a=lizzard"
merging js/src/jsscript.cpp
merging js/src/jsscript.h
warning: conflicts while merging js/src/jsscript.h! (edit, then use 'hg resolve --mark')
Flags: needinfo?(jorendorff)
Assignee: jorendorff → shu
Flags: needinfo?(jorendorff) → needinfo?(shu)
Patch for ESR38 posted.
Flags: needinfo?(shu)
Flags: needinfo?(cbook)
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main46+][adv-esr45.1+][adv-esr38.8+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.