674195 - Cross compartment Object.freeze misbehaves

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Here is test case to reproduce an error:

var sandbox = Components.utils.Sandbox("http://www.example.com/");
Components.utils.evalInSandbox('var exports = { foo: "foo" }', sandbox);
Object.freeze(sandbox.exports);
Object.isFrozen(sandbox.exports); // false <- must be true
Object.defineProperty(sandbox.exports, 'foo', { value: 'bar' });
sandbox.exports.foo // 'bar' <- must be 'foo'

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

It seems that this is broken on aurora and latest nightly but in a different way:

var sandbox = Components.utils.Sandbox("http://www.example.com/");
Components.utils.evalInSandbox('var exports = { foo: "foo" }', sandbox);
Object.freeze(sandbox.exports);
// Throws TypeError: can't change object's extensibility

Comment 2

[Boris Zbarsky [:bzbarsky]]

It's not clear yet how Object.freeze should behave on a proxy, exactly....

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

While I dislike current behavior a lot, it's still possible to leave with Object.freeze that throws. `Object.isFrozen` returning `false` on frozen objcets, is definitely something that should be addressed, as otherwise we have no way to detect if object is frozen without trying to freeze it.

Comment 4

[Jeff Walden [:Waldo]]

I think there was some sort of proxy hook for freezing, actually.  I'm a little surprised if we aren't calling it here.  That said, it's not clear to me how well a freeze hook on a proxy actually can work, given the dynamicity of proxies.  But I've given it little enough thought that I would say it can't work.

Compartment-per-global is going to have to address this one way or another, I suspect.

Comment 5

[Jeff Walden [:Waldo]]

Er, "wouldn't say it can't".

Comment 6

[David Bruant]

(In reply to Boris Zbarsky (:bz) from comment #2)
> It's not clear yet how Object.freeze should behave on a proxy, exactly....
It is:
It should call the fix trap (throw a TypeError if this one doesn't exist). If the return value of the trap is a property descriptor map, the proxy will become the object created with the description otherwise it will throw a TypeError (because the proxy refuses to be frozen).

However, I don't know how the proxies in game here work.

Has bug 600677 landed?

(In reply to Jeff Walden (remove +bmo to email) from comment #4)
> That said, it's not clear to
> me how well a freeze hook on a proxy actually can work, given the dynamicity
> of proxies.
At the end of the call, the proxy is expected to become another object (or throw an exception).

It has to be noted that this is all related to the original proxy design. Things are going ot be different with the new proxy proposal (if accepted at the TC39 November meeting): wiki.ecmascript.org/doku.php?id=strawman:direct_proxies

Comment 7

[Boris Zbarsky [:bzbarsky]]

The fix trap for XrayWrapper returns undefined, for what it's worth.  So per comment 6 this should throw, but doesn't for some reason.

This is probably wrong for transparent and same-origin situations....

Comment 8

[Eddy Bruel [:ejpbruel]]

Here's my understanding of what happens.
- obj_freeze calls Object.freeze
- Object.freeze calls JSObject::sealOrFreeze with FREEZE as immutability type
- JSObject::sealOrFreeze calls JSObject::isExtensible. If JSObject is a proxy, and the proxy is trapping, JSObject::isExtensible() should be true (http://wiki.ecmascript.org/doku.php?id=harmony:proxies&s=proxy)
- If JSObject::isExtensible() is true, JSObject::sealOrFreeze calls JSObject::preventExtensions
- JSObject::preventExtensions calls getOps()->fix

- By calling getOps()->fix, we end up in proxy_fix
- proxy_fix checks if the object we're trying to fix is a proxy, and if it is, calls FixProxy
- FixProxy calls Proxy::fix
- Proxy::fix calls the fix method on the actual wrapper object
- If the wrapper sets vp to undefined, as suggested in comment 7, we end up back in FixProxy, where Proxy::fix has returned false and the value of tvr is set to undefined
- FixProxy explicitly checks if tvr's value is undefined. If it is, it sets its out parameter bp to false and returns true
- Now we end up back in proxy_fix, where FixProxy has returned true, and the value of flag is set to false
- As a result, proxy_fix sets it out parameter vp to false and returns true

- Now we end up back in JSObject::preventExtensions, where fix has returned true, and success has assumed the value of vp, which is false
- JSObject::preventExtensions explicitly checks if success is false, and if it is, reports a JSMSG_CANT_CHANGE_EXTENSIBILITY error and returns false.

TL;DR:
The fix trap on the proxy returns undefined. As a result Object.freeze raises z type error. If I understand comment 4 correctly, this is exactly the behavior I'd expect.

In other words, it looks like this was broken on Firefox 8, but has since been resolved in the Nightly. That leaves open the issue of Object.isFrozen returning false for non-frozen objects  that Irakli mentioned in comment 3, although I suspect there might be some miscommunication here. In none of the examples that Irakli posted has the object actually been frozen, so I'd *expect* Object.isFrozen to return false there. Is there something I'm missing here?

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Eddy Bruel [:ejpbruel] from comment #8)
> In none of the
> examples that Irakli posted has the object actually been frozen, so I'd
> *expect* Object.isFrozen to return false there. Is there something I'm
> missing here?

Components.utils.evalInSandbox('Object.freeze({})', sandbox);
Object.isFrozen(Components.utils.evalInSandbox('Object.freeze({})', sandbox));
// Must be true, but returns false instead

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Oops I meant to write following instead:

var sandbox = Components.utils.Sandbox("http://www.example.com/");
Object.isFrozen(Components.utils.evalInSandbox('Object.freeze({})', sandbox));
// Must be true, but returns false instead

Comment 11

[Eddy Bruel [:ejpbruel]]

I've opened a new bug on the issue that freezing an object doesn't freeze any proxies towards that object. You can find it here: https://bugzilla.mozilla.org/show_bug.cgi?id=706041

The issue that Object.freeze doesn't throw when it should has been resolved in the Nightly, so I'm closing this bug.

Comment 13

[Boris Zbarsky [:bzbarsky]]

With cpg this is a much more serious issue, since web sites can hit it now.

Comment 14

[Eddy Bruel [:ejpbruel]]

akeybl asked me to write a risk analysis for this bug, in case we don't get to land it in time:

To my knowledge, other than the Jetpack team, who rely on the ability to freeze an object cross-compartment for their module system, there are very few people who actually care about this bug. The old proxies have never worked in such a way that making the proxy non-extensible would make the target non-extensible as well (in fact, this isn't even possible with the old proxies). Instead, it was possible to provide a 'snapshot' of a Proxy by calling the fix trap, and have that act as the frozen Proxy, but nobody relies on that either. In fact, the fix trap was already removed earlier during the implementation of direct proxies, without breaking any existing code.

Since the Jetpack team already has a workaround for this bug, I'd say the risk involved with not landing this is low. Even so, I've filed a patch for bug 789897, which should resolve this issue.

Comment 15

[Jasvir Nagra]

The Caja sandbox relies heavily on cross-frame Object.freeze() for it's security and this vulnerability would result in us failing back to a much much slower serverside rewriting solution for Firefox users on browsers that have this bug.

Comment 16

[Eddy Bruel [:ejpbruel]]

(In reply to Jasvir Nagra from comment #15)
> The Caja sandbox relies heavily on cross-frame Object.freeze() for it's
> security and this vulnerability would result in us failing back to a much
> much slower serverside rewriting solution for Firefox users on browsers that
> have this bug.

Thank you for pointing that out. You will be happy to know that a fix for this is underway: https://bugzilla.mozilla.org/show_bug.cgi?id=789897

Comment 17

[Alex Keybl [:akeybl]]

Given comment 14 and where we are in the cycle, untracking for release and wontfixing for FF16.

Comment 18

[Mark S. Miller]

What is the status of this? It still prevents Caja from running untranslated on Firefox.

Comment 19

[Bobby Holley (:bholley)]

Eddy, it's not clear to me whether this is a pure dupe of bug 789897 or whether we need another patch here. Can you clarify?

Comment 20

[Eddy Bruel [:ejpbruel]]

(In reply to Bobby Holley (:bholley) from comment #19)
> Eddy, it's not clear to me whether this is a pure dupe of bug 789897 or
> whether we need another patch here. Can you clarify?

Properly forwarding Object.preventExtensions to the underlying target should automatically solve the issues we're having with Object.freeze here.

Comment 21

[Mark S. Miller]

Is this now fixed?

Comment 22

[Eddy Bruel [:ejpbruel]]

(In reply to Mark S. Miller from comment #21)
> Is this now fixed?

It should be now that bug 789897 has landed, but I haven't checked it yet. If you could confirm, that would be great :-)

Comment 23

[Mark S. Miller]

As far as our SES tests can tell, yes this is fixed.

Comment 24

[Eddy Bruel [:ejpbruel]]

(In reply to Mark S. Miller from comment #23)
> As far as our SES tests can tell, yes this is fixed.

Awesome! We should probably add a test for this before we close it. I can look into that tomorrow if you want.

Comment 25

[Mark S. Miller]

That would be wonderful, thanks!

Comment 26

[Ihab Awad]

This bug makes Google Caja's ES5-native sandbox completely insecure on current Firefox releases, I'm afraid. We have started tracking this on our issue tracker here --

  https://code.google.com/p/google-caja/issues/detail?id=1698

I verified the bug exists on Firefox 20.0 and has been fixed on current Nightly (23.0a1 (2013-04-10)).

Could you give us an idea of when this will show up in a FF consumer build as this affects rollout of ES5-native sandboxing for Caja's downstream dependents?

Comment 27

[Ihab Awad]

This bug makes Google Caja's ES5-native sandbox completely insecure on current Firefox releases, I'm afraid. We have started tracking this on our issue tracker here --

  https://code.google.com/p/google-caja/issues/detail?id=1698

I verified the bug exists on Firefox 20.0 and has been fixed on current Nightly (23.0a1 (2013-04-10)).

Could you give us an idea of when this will show up in a FF consumer build as this affects rollout of ES5-native sandboxing for Caja's downstream dependents?

Comment 28

[Bobby Holley (:bholley)]

(In reply to Ihab Awad from comment #27)
> Could you give us an idea of when this will show up in a FF consumer build
> as this affects rollout of ES5-native sandboxing for Caja's downstream
> dependents?

The release calendar is here: https://wiki.mozilla.org/RapidRelease/Calendar

Comment 29

[Bobby Holley (:bholley)]

What does Caja do, exactly? From the docs, I get the sense that it runs the pre-processes the code and runs it in an iframe with some type of membrane across object access. Is that right? What does it use to implement the membrane? Direct Proxies? Indirect Proxies?

Comment 30

[Ihab Awad]

Hi Bobby -- the new version of Caja (which is the one affected by this bug) does not rewrite the code (well -- not much anyway) and does not rely on iframes as the defense mechanism. It relies on native ES5 mechanisms -- typically Object.freeze() -- to isolate stuff. But, you might ask, where do iframes (and cross-frame stuff) come in? ;)

We have the capability to run untrusted and trusted code in the same frame and maintain our security guarantees. But doing that requires that we freeze primordials (Object.*, Array.*, ...) in that frame, so attacker code can't ruin the global environment.

Well, unfortunately, a lot of the code in the wild, which we must run, assumes it can monkey patch Object.* and Array.*. So, to maintain compatibility, we run the sandboxed code in its own iframe where it *can* monkey patch these primordials, but we also use ES5 language mechanisms to limit the scope of the code so it can't do anything evil even with that.

Now, to implement our trusted computing base (TCB) code, we need a good clean place to do it. So we create another frame, where we *do* freeze the primordials and lock things down good and tight.

It is in the communication between these two frames -- the sandbox and the TCB frame -- that we need the freezing invariant to continue to operate, and the lack of it is why this bug causes us to fall over with no chance of getting up. :)

Does that explain things?

Comment 31

[Bobby Holley (:bholley)]

(In reply to Ihab Awad from comment #30)
> It is in the communication between these two frames -- the sandbox and the
> TCB frame -- that we need the freezing invariant to continue to operate, and
> the lack of it is why this bug causes us to fall over with no chance of
> getting up. :)

I see! What kind of mechanism do you use to implement the membrane, though? Proxies?

Also, what do you do about DOM objects that are effectively unfreezable, like the Window?

Comment 32

[Ihab Awad]

We don't [yet] use native proxies (supporting "wildcard" property names) for our membrane. But we do build intermediating objects on a property by property basis using plain ES5 property controls. So far, that has been enough. A future and better membrane based on proxies would be ... um ... better? ;)

As far as "Window" and its friends -- we build a whole DOM emulation layer generated in part from an HTML schema and in part from carefully crafted hand-rolled code. It's not possible to trivially intermediate the DOM: (a) Many DOM operations are different spellings of "eval" that will, by default, execute outside our sandbox; (b) We must also sandbox the CSS styling; (c) We need to sandbox stuff like getElementsByTagName and friends.

Comment 33

[Alex Keybl [:akeybl]]

(In reply to Ihab Awad from comment #26)
> Could you give us an idea of when this will show up in a FF consumer build
> as this affects rollout of ES5-native sandboxing for Caja's downstream
> dependents?

We don't typically uplift this kind of code change when the defect has had very little user impact in recent releases. We'll still consider this before making a final decision.

Comment 34

[Alex Keybl [:akeybl]]

Given the fact that Google Caja doesn't have significant penetration on the web and is functional (albeit in a slower state) currently, no need to take this patch any sooner than 23 where it landed.

Comment 35

[Jasvir Nagra]

Kevin Reid has come up with a workaround that allows Caja to continue to be functional safely in spite of this spec violation.  As such we do not require the escalation to port this to an earlier version of FF.

Thanks for your help with triaging this bug!

Comment 36

[Eddy Bruel [:ejpbruel]]

Is this confirmed fixed now?

Comment 37

[Eddy Bruel [:ejpbruel]]

Irakli, can you confirm that this problem is now gone?

Comment 38

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Yes I confirm that both of my original test cases work as expected on current nightly.

Comment 39

[Jeff Walden [:Waldo]]

I think we can declare this fixt.