"Assertion failure: hasScript()" with setTimeout(Array.prototype.indexOf)

RESOLVED FIXED in Firefox 20

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: till)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla20
x86_64
Mac OS X
assertion, regression, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox19 unaffected, firefox20+ fixed, firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main20-])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 685008 [details]
testcase (asserts fatally when loaded)

Assertion failure: hasScript(), at js/src/jsfun.h:203
(Reporter)

Comment 1

5 years ago
Created attachment 685009 [details]
stack trace
Not an XPConnect problem.  This API should be safe to call on all JSFunctions and should return null if there is no script.

In this case, it looks like isInterpreted() is true but hasScript() is false, so JSFunction::maybeNonLazyScript totally fails to do the right thing.

I will bet money this is fallout from bug 784294.
Assignee: nobody → general
Component: XPConnect → JavaScript Engine
Oh, and this seems security-sensitive if the assert really means we'll treat random memory as a JSScript*....
Group: core-security
tracking-firefox20: --- → ?
(Assignee)

Comment 4

5 years ago
Created attachment 685090 [details] [diff] [review]
Initialize lazy script in JS_GetFunctionScript

No call on your bet from my side: that's spot on.

maybeNonLazyScript does what was intended for interpreted functions, but that's not what's desirable in this case.

The expectation is for the (maybeN|n)onLazyScript functions to be used iff the call site can guarantee the script to be initialized. E.g., during execution of the script itself. JS_GetFunctionScript isn't such a call site and should have used getOrCreateScript.

The attached patch changes to that and correctly returns NULL for native functions or the (possibly just initialized) script for interpreted ones.
Assignee: general → tschneidereit
Status: NEW → ASSIGNED
Attachment #685090 - Flags: review?(luke)
Attachment #685090 - Flags: review?(bzbarsky)
Comment on attachment 685090 [details] [diff] [review]
Initialize lazy script in JS_GetFunctionScript

Seems reasonable at first glance.  One question: For self-hosted stuff, will this return a non-null JSScript?  If so, what principal is that compiled with?
Attachment #685090 - Flags: review?(bzbarsky) → review+
(Reporter)

Updated

5 years ago
Blocks: 784294
(Assignee)

Comment 6

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #5)
> Comment on attachment 685090 [details] [diff] [review]
> Initialize lazy script in JS_GetFunctionScript
> 
> Seems reasonable at first glance.  One question: For self-hosted stuff, will
> this return a non-null JSScript?  If so, what principal is that compiled
> with?

Yes, it will always return a JSScript for interpreted functions. For self-hosted functions that haven't yet been called, it will clone the script in getOrCreateScript.

The script isn't compiled for the global that it's cloned into. Instead, the version that was compiled during self-hosting initialization is cloned.

Self-hosted code is compiled with a NULL principal, but I now see that cloned scripts, while getting the source script's originPrincipals, get the current compartment's principals. (at jsscript.cpp:2252)

Luke, can you comment on the implications of that?
In practice, I suspect it doesn't matter that much.  The only question is what should happen when Array.prototype.whatever from one window (e.g. chrome) is poked at from another window (e.g. content).  Bobby?

Comment 8

5 years ago
Using the current compartment for the principals should be fine as long as cx->compartment matches script->compartment.  Can you add assertSameCompartment(cx, script)?

Second: if getOrCreateScript fails, an exception will be thrown which means that a NULL return from JS_GetFunctionScript will have 2 different meanings (the other being "the function is native").  Rather than gunking up this API, I propose we MOZ_ABORT if getOrCreateScript fails.

Updated

5 years ago
Attachment #685090 - Flags: review?(luke)
tracking-firefox20: ? → +
(Assignee)

Comment 9

5 years ago
Created attachment 690245 [details] [diff] [review]
Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript

I included both changes, but I'm not entirely sure if I understand the value of the assertSameCompartment. In the case of lazily cloned scripts, this holds by necessity, doesn't it? If so, shouldn't we rather assert that the given function is in the same compartment as the current context?
Attachment #690245 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Attachment #685090 - Attachment is obsolete: true

Comment 10

5 years ago
Comment on attachment 690245 [details] [diff] [review]
Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript

You're right, I meant to ask that you assert the *function* is in the same compartment as cx, as a precondition.
Attachment #690245 - Flags: review?(luke) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 690386 [details] [diff] [review]
Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript.

Assert that function, not script, is in same compartment as cx
(Assignee)

Updated

5 years ago
Attachment #690245 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Comment on attachment 690386 [details] [diff] [review]
Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript.

Carrying r=luke
Attachment #690386 - Flags: review+
(Assignee)

Comment 13

5 years ago
Comment on attachment 690386 [details] [diff] [review]
Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript.

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Pretty easily: it's a clear NULL pointer return.

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

Which older supported branches are affected by this flaw?

None, it was introduced in the current Nightly.

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

Bug 791850

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

n/a

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

It introduces a assertion the pre-flaw code didn't have: that the current JSContext is in the same compartment as the function whose script is queried. I don't know how likely it is that that assert doesn't always hold. It would have been a sec-sensitive issue before this flaw, though.
Attachment #690386 - Flags: sec-approval?
Blocks: 791850
status-firefox-esr10: --- → unaffected
status-firefox19: --- → unaffected
status-firefox20: --- → affected
status-firefox-esr17: --- → unaffected
Keywords: regression, sec-critical
Comment on attachment 690386 [details] [diff] [review]
Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript.

sec-approval+.

That said, if the bug only exists in trunk, you don't need to ask for approval to fix it, even if it is a security issue.
Attachment #690386 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/531eb76b7ab6

(In reply to Al Billings [:abillings] from comment #14)
> That said, if the bug only exists in trunk, you don't need to ask for
> approval to fix it, even if it is a security issue.

Ah, I didn't know that. Sorry for the extra work, then.
Backed out due to test failures.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=531eb76b7ab6

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebfa6888f98e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0368b4c03c63
(Assignee)

Comment 17

5 years ago
Created attachment 692286 [details] [diff] [review]
Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript.

So it turns out that the function isn't in the same compartment as the context quite oftenly. Stupid of me not to test that on try before pushing, sorry.

The only option I see here is to temporarily switch the compartment if we have to initialize the script, which this version does.

Try-servering here: http://tbpl.mozilla.org/?tree=Try&rev=7e98b9f06910
Attachment #692286 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Attachment #690386 - Attachment is obsolete: true

Comment 18

5 years ago
Comment on attachment 692286 [details] [diff] [review]
Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript.

Whew, lameness.  Sorry for not knowing either.
Attachment #692286 - Flags: review?(luke) → review+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff71c0ea0cb2
https://hg.mozilla.org/mozilla-central/rev/ff71c0ea0cb2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox20: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

Comment 21

5 years ago
Can there be a comment explaining the use of MOZ_CRASH()? It's a rather startling statement to come across in normal API code.
status-b2g18: --- → unaffected
Whiteboard: [adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.