Ion DOM method call optimization is currently unsound

RESOLVED FIXED in Firefox 18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(4 keywords)

unspecified
mozilla20
x86
Mac OS X
crash, regression, sec-moderate, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main18-])

Attachments

(3 attachments, 3 obsolete attachments)

Created attachment 693005 [details]
Testcase

We flag the function as "DOM" at getprop time, then use that at call time, but nothing says our "this" object at call time matches the object we got the function from!  The attached testcase crashes for me, for example.  The only good news is it crashes on a null deref, but I'm not sure whether that's guaranteed.
(Assignee)

Comment 1

6 years ago
Created attachment 693019 [details] [diff] [review]
Make the Ion optimization for DOM method calls sound.   static functions just got moved with no changes made to them.
Attachment #693019 - Flags: review?(jdemooij)
(Assignee)

Comment 2

6 years ago
For branches, is it better to do the above patch or just disable the optimization completely?  The latter seems like it might be less risky....
(Assignee)

Comment 3

6 years ago
Created attachment 693025 [details] [diff] [review]
Now with a test
Attachment #693025 - Flags: review?(jdemooij)
(Assignee)

Updated

6 years ago
Attachment #693019 - Attachment is obsolete: true
Attachment #693019 - Flags: review?(jdemooij)
(Assignee)

Comment 4

6 years ago
Created attachment 693037 [details] [diff] [review]
Now with another test, and a fix to make it work too
Attachment #693037 - Flags: review?(jdemooij)
(Assignee)

Updated

6 years ago
Attachment #693025 - Attachment is obsolete: true
Attachment #693025 - Flags: review?(jdemooij)
(Assignee)

Comment 5

6 years ago
Created attachment 693054 [details] [diff] [review]
With the unused test file removed
Attachment #693054 - Flags: review?(jdemooij)
(Assignee)

Updated

6 years ago
Attachment #693037 - Attachment is obsolete: true
Attachment #693037 - Flags: review?(jdemooij)
(Assignee)

Comment 6

6 years ago
Created attachment 693061 [details] [diff] [review]
Patch for branches: just disable the optimization there
Attachment #693061 - Flags: review?(jdemooij)
(Assignee)

Updated

6 years ago
Blocks: 773549

Updated

6 years ago
Attachment #693061 - Flags: review?(jdemooij) → review+
Comment on attachment 693054 [details] [diff] [review]
With the unused test file removed

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

Makes sense. r=me with comment below addressed. Can you also make sure the optimization still works in the most important cases?

::: js/src/ion/IonBuilder.cpp
@@ +4194,5 @@
>  MCall *
>  IonBuilder::makeCallHelper(HandleFunction target, uint32_t argc, bool constructing)
>  {
>      // This function may be called with mutated stack.
>      // Querying TI for popped types is invalid.

Note this comment...

@@ +4255,5 @@
> +    if (target) {
> +        // We know we have a single call target.  Check whether the "this" types
> +        // are DOM types and our function a DOM function, and if so flag the
> +        // MCall accordingly.
> +        types::StackTypeSet *thisTypes = oracle->getCallArg(script(), argc, 0, pc);

We *are* querying TI for popped types here. What's scary is that makeCallHelper and makeCallBarrier are also used for getters/setters. So can you change the "if (target) {" to:

if (target && JSOp(*pc) == JSOP_CALL) {

?

That should avoid querying popped types with a mutated stack, and it's also good to be conservative about the cases we optimize (e.g. don't mark as DOM function for everything other than JSOP_CALL, like JSOP_NEW/getter/setter etc).
Attachment #693054 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 8

6 years ago
> Can you also make sure the optimization still works in the most important cases?

I did that manually for a few microbenchmarks.  Unfortunately, we don't have a way I can think of to test this in an actual automated test, which makes it impossible to regression-test and makes generating more interesting tests hard.  :(

> We *are* querying TI for popped types here.

I have no idea what this means....

> What's scary is that makeCallHelper and makeCallBarrier are also used for
> getters/setters.

Ah.  Did not know that.

> if (target && JSOp(*pc) == JSOP_CALL) {

This I can certainly do, though!
(Assignee)

Comment 9

6 years ago
Comment on attachment 693061 [details] [diff] [review]
Patch for branches: just disable the optimization there

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Ion landing
User impact if declined: Crashes due to invariants being violated.  At least
   null-deref, but possibly exploitable.
Testing completed (on m-c, etc.): Fixes attached automated tests.
Risk to taking this patch (and alternatives if risky):   This just turns off an
   optimization that's not critically important on branches yet, so should be
   very safe.
String or UUID changes made by this patch: None.
Attachment #693061 - Flags: review-
Attachment #693061 - Flags: review+
Attachment #693061 - Flags: approval-mozilla-beta?
Attachment #693061 - Flags: approval-mozilla-aurora?
Comment on attachment 693054 [details] [diff] [review]
With the unused test file removed

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  I don't know. 
   The tests indicate a null-deref crash.  I'm not sure whether more
   complicated things are possible.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  If there is one, probably.

Which older supported branches are affected by this flaw?  Aurora and Beta

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Yes, see the "disable" patch.  It's very low
   risk.

How likely is this patch to cause regressions; how much testing does it need?
   Unclear, but we would not be taking this patch on branches.
Attachment #693054 - Flags: sec-approval?
How is this ESR17 unaffected already?  Should we get a sec-rating here first and then assess if it needs to go on ESR17 branch?
tracking-firefox18: ? → +
tracking-firefox19: ? → +
tracking-firefox20: ? → +
Unless this is IonMonkey and I missed that...
Lukas, this bug is Ion-only.  Sorry, I should have made that clearer in the summary and comment 0.
Summary: DOM method call optimization is currently unsound → Ion DOM method call optimization is currently unsound
Keywords: crash, regression, sec-moderate, testcase
Comment on attachment 693054 [details] [diff] [review]
With the unused test file removed

sec-approval+ for m-c.

Please don't check in the tests now unless you get approval to land the safer patch on Beta (Fx 18). If you get approval for Aurora only we should wait until January to land the tests (I see no reason to keep this regression fix out of Fx18).
(Assignee)

Updated

6 years ago
Attachment #693061 - Flags: review- → review+

Updated

6 years ago
Attachment #693061 - Flags: approval-mozilla-beta?
Attachment #693061 - Flags: approval-mozilla-beta+
Attachment #693061 - Flags: approval-mozilla-aurora?
Attachment #693061 - Flags: approval-mozilla-aurora+
Comment on attachment 693054 [details] [diff] [review]
With the unused test file removed

sec-approval+ per comment 14
Attachment #693054 - Flags: sec-approval? → sec-approval+
http://hg.mozilla.org/integration/mozilla-inbound/rev/3d2011652b37
Assignee: general → bzbarsky
Flags: in-testsuite+
Target Milestone: --- → mozilla20
https://hg.mozilla.org/releases/mozilla-aurora/rev/ec8afc7ac4da
https://hg.mozilla.org/releases/mozilla-beta/rev/8b2da786634a
status-firefox18: affected → fixed
status-firefox19: affected → fixed
status-firefox20: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/3d2011652b37
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.