Skipping argument type checks is unsafe when the callee is relazified

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

({sec-critical})

unspecified
mozilla38
Points:
---

Firefox Tracking Flags

(firefox35 wontfix, firefox36+ fixed, firefox37+ fixed, firefox38+ fixed, firefox-esr3136+ fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main36+][adv-esr31.5+])

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
I did some Try pushes with more eager JIT compilation, and it triggered a bunch of mysterious argument-check failures on various test suites. I think what's happening is this:

(1) We have a callee function (ArrayIndexOf in the case I looked at), that has a TypeScript. Let's call this function G.

(2) We Ion-compile a function (F) that calls G. We use G's TypeScript to determine we can skip the argument type checks for this call.

(3) We relazify G.

(4) G is unlazified, gets a new JSScript with a new TypeScript, new Baseline/Ion code.

(5) F's Ion code is still the same and it can call G and skip the argument checks. However, this is no longer sound, because G has a new TypeScript and its argument types are no longer guaranteed to match...
(Assignee)

Comment 1

4 years ago
Btw, it seems like this can happen whenever we discard a script's TypeScript without invalidating *all* Ion code in the compartment... Relazification can do this but I'm not sure it's the only way.
(Assignee)

Comment 2

4 years ago
[Tracking Requested - why for this release]:
Blergh. Either we change relazification to only be triggered when we also invalidate all Ion code, or we keep the TypeScript around, I guess. I kind of like the former, because it might also get rid of other subtle issues, and the memory usage regresssions shouldn't be too important as long as we still relazify on memory pressure.
(Assignee)

Comment 4

4 years ago
Posted file Shell testcase
Repros at inbound revision 9758962dd013.

$ js --no-threads test.js
Assertion failure: Argument check fail., at MacroAssembler.cpp:1334

Very hard to generate for a fuzzer though, the test relies on a lot of factors and heuristics.
(Assignee)

Comment 5

4 years ago
(In reply to Till Schneidereit [:till] from comment #3)
> Either we change relazification to only be triggered when we also
> invalidate all Ion code, or we keep the TypeScript around, I guess.

I don't know if the former is safe with incremental GC, unless we discard all Ion code on every slice.

Another option is to only relazify if the script has no TypeScript. TypeScripts are not discarded on every GC (see JIT_SCRIPT_RELEASE_TYPES_PERIOD), but after a number of GCs we should discard the types and then we can relazify...
(Assignee)

Comment 6

4 years ago
Also, it'd be nice if we could get rid of this footgun:

(In reply to Jan de Mooij [:jandem] from comment #1)
> it seems like this can happen whenever we discard a script's TypeScript
> without invalidating *all* Ion code in the compartment...

Maybe we should change our skip-argument-checks code to invalidate the caller whenever the callee's TypeScript goes away. Or something...
(In reply to Jan de Mooij [:jandem] from comment #5)
> Another option is to only relazify if the script has no TypeScript.
> TypeScripts are not discarded on every GC (see
> JIT_SCRIPT_RELEASE_TYPES_PERIOD), but after a number of GCs we should
> discard the types and then we can relazify...

That makes sense to me, yes.

(In reply to Jan de Mooij [:jandem] from comment #6)
> Maybe we should change our skip-argument-checks code to invalidate the
> caller whenever the callee's TypeScript goes away. Or something...

And this, too.
(Assignee)

Updated

4 years ago
Flags: needinfo?(jdemooij)
Group: core-security
(Assignee)

Comment 8

4 years ago
Posted patch PatchSplinter Review
Don't relazify scripts with a TypeScript.

Later we can add a comment and remove the hasBaselineScript and hasIonScript checks but this is just the simplest/safest fix for now.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8559998 - Flags: review?(till)
Comment on attachment 8559998 [details] [diff] [review]
Patch

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

Delightful.
Attachment #8559998 - Flags: review?(till) → review+
(Assignee)

Comment 10

4 years ago
Comment on attachment 8559998 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily, it'd require quite a lot of work.

> 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?
All of them, I think.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy, patch is a one-liner.

> How likely is this patch to cause regressions; how much testing does it need?
It could regress memory usage as we will potentially relazify functions at a later point, but I don't think this will be a problem because we will still relazify them at some point.
Attachment #8559998 - Flags: sec-approval?
Comment on attachment 8559998 [details] [diff] [review]
Patch

sec-approval+.

Let's get this on Aurora, Beta, and ESR31 ASAP too.
Attachment #8559998 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 13

4 years ago
Comment on attachment 8559998 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 886193.
[User impact if declined]: Crashes, security bugs.
[Describe test coverage new/current, TreeHerder]: Just landed on m-i; safe one-liner.
[Risks and why]: Could regress memory usage slightly, because we now have to wait with relazifying functions until the TypeScripts are discarded. However I'm not too worried about this.
[String/UUID change made/needed]: None.
Attachment #8559998 - Flags: approval-mozilla-esr31?
Attachment #8559998 - Flags: approval-mozilla-beta?
Attachment #8559998 - Flags: approval-mozilla-aurora?
Comment on attachment 8559998 [details] [diff] [review]
Patch

Best to get this on all channels at once. Approving for ESR31, Beta, and Aurora.
Attachment #8559998 - Flags: approval-mozilla-esr31?
Attachment #8559998 - Flags: approval-mozilla-esr31+
Attachment #8559998 - Flags: approval-mozilla-beta?
Attachment #8559998 - Flags: approval-mozilla-beta+
Attachment #8559998 - Flags: approval-mozilla-aurora?
Attachment #8559998 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/1938c82eaf52
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Whiteboard: [adv-main36+][adv-esr31.5+]
Group: javascript-core-security

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.