822340 - Ion DOM method call optimization is currently unsound

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: 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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Make the Ion optimization for DOM method calls sound. static functions just got moved with no changes made to them.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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....

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with a test

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with another test, and a fix to make it work too

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: With the unused test file removed

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for branches: just disable the optimization there

Comment 7

[Jan de Mooij [:jandem]]

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).

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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!

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

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?

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Unless this is IonMonkey and I missed that...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Lukas, this bug is Ion-only.  Sorry, I should have made that clearer in the summary and comment 0.

Comment 14

[Daniel Veditz [:dveditz]]

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).

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 693054 [details] [diff] [review]
With the unused test file removed

sec-approval+ per comment 14

Comment 16

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3d2011652b37

Comment 17

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ec8afc7ac4da
https://hg.mozilla.org/releases/mozilla-beta/rev/8b2da786634a

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3d2011652b37

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8b2da786634a