1051224 - Regression: Can't cd into sandboxed iframe

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Daniel Huigens [:twiss]]

Attachment: cd-sandbox-testcase.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release)
Build ID: 20140804060850

Steps to reproduce:

- Open the attached testcase, or any page with a sandboxed iframe
- Open the Firefox Console
- cd(frames[0])


Actual results:

Error: Permission denied to pass object to chrome


Expected results:

Switch the console's context to the iframe, access its variables etc.

Output from mozregression:
> Last good revision: f5f3d2f7bc4e
> First bad revision: 2eddd81bb167
> Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f5f3d2f7bc4e&tochange=2eddd81bb167

Comment 1

[Loic]

May a dev with higher privileges add bug 930091 in the "Blocks" field and change the product component, please.

Comment 2

[:Gijs (he/him)]

I suspect this will actually need to be fixed in the developer tools rather than xpconnect...

Comment 3

[Bobby Holley (:bholley)]

We may need to add an option to exportFunction and friends to allow this.

I can do the fix pretty easily I suspect, but I'm not at all familiar with the test machinery we'll need here, so it'd be good for someone else to do that part. If someone can point me to where the 'cd' function is exposed and write the appropriate automated test, I will fix this bug. 

Rob, can you find an owner for the above?

Comment 4

[Rob Campbell [:rc] (:robcee)]

looking...

Comment 5

[Rob Campbell [:rc] (:robcee)]

Alex, do you have a moment to take a look at this? I see you had some input on bug 609872 during review and are working on the toolbox iframe targeting bug.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Bobby: quit it.

Comment 7

[Alexandre Poirot [:ochameau]]

I don't have immediate availability, but I'm keeping the needinfo in case I find some time next week.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Test console's cd() against sandboxed iframes

Here is a test case.

You may be interested in dumping the aResponse object from onFooObjectFromIframe
function.

Otherwise the cd() implementation is quite easy and is done over here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/utils.js#1638
Hopefully, you can, with some xpconnect/platform magic do so that `aWindow`
is a kind of unwrapped/unsafe/free-to-read window reference.

If such magic already exists, I would be happy to give it a try and verify if it fixes the console.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8474615 [details] [diff] [review]
Test console's cd() against sandboxed iframes

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

So, operating in chrome context means that the cd will always succeed (with or without the fix for this bug), because the web console is bound to a page that runs with system principal. The best thing to do would probably be to put the sandboxed iframe inside of a regular http:// iframe. We would first cd into the regular iframe, then into the sandboxed iframe, then cd(parent), then cd(parent) again.

The test presently fails for reasons unrelated to the bug at hand. The problem is that the use of document.createElement('iframe') on a XUL document creates a XUL iframe, which doesn't know what srcdoc is. Consider using createElementNS, or just putting the iframe inside of the HTML page.

Regardless, thanks for writing this test Alex! Should just require a couple of quick fixes and be good to go.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add an opt-out for cross-origin argument checking. v1

Some of this will probably seem familiar. ;-)

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use the opt-out for the webconsole helpers. v1

Comment 12

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8475724 [details] [diff] [review]
Part 1 - Add an opt-out for cross-origin argument checking. v1

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

::: js/xpconnect/src/ExportHelpers.cpp
@@ +208,5 @@
>  {
> +    // Consumers can explicitly opt out of this security check. This is used in
> +    // the web console to allow the utility functions to accept cross-origin Windows.
> +    if (options.allowCrossOriginArguments)
> +        return true;

Optional security check is almost like none at all... I'm sure we can audit usages of this flag internally but I'm concerned about add-ons. So I'm trying to juggle between removing the check entirely (and that kind of makes me uncomfortable, because the additional information it could leak to content) vs solving this problem differently. I don't fully grasp the web consoles use case. Could you shed some light on it for me?

Comment 13

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> Optional security check is almost like none at all... I'm sure we can audit
> usages of this flag internally but I'm concerned about add-ons. So I'm
> trying to juggle between removing the check entirely (and that kind of makes
> me uncomfortable, because the additional information it could leak to
> content) vs solving this problem differently.

Well, the default is that the security check is performed. This is just an opt-out for very specific use-cases. We could even avoid documenting it.

> I don't fully grasp the web
> consoles use case. Could you shed some light on it for me?

The web console needs to expose various utility functions, one of which is "cd", which changes the console context to a given global. cd(someCrossOriginWindow) doesn't work, because the argument is cross-origin.

Comment 14

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8475724 [details] [diff] [review]
Part 1 - Add an opt-out for cross-origin argument checking. v1

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

(In reply to (PTO 8/22 - 9/1) from comment #13)
> We could even avoid documenting it.

Or we can just ban it for add-ons in a few ways if it's really needed. Thinking it over, I don't think it's a huge threat in the current setup so I don't think we should care to do so for now. Patch looks great r=me.

::: js/xpconnect/src/ExportHelpers.cpp
@@ +208,5 @@
>  {
> +    // Consumers can explicitly opt out of this security check. This is used in
> +    // the web console to allow the utility functions to accept cross-origin Windows.
> +    if (options.allowCrossOriginArguments)
> +        return true;

I'm a bit hesitant about this. If a security check is optional it almost means we can remove that check entirely... Could you explain me why we need to do this exactly?

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Test console's cd() against sandboxed iframes

Ok, I think this is better now.
The test fails without the patch and pass with it.
Note that if you pass a selector string to cd(),
cd() works correctly with sandboxed iframes.
It only fails if you try to pass a content window object.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8475725 [details] [diff] [review]
Part 2 - Use the opt-out for the webconsole helpers. v1

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

Works great!

::: toolkit/devtools/server/actors/webconsole.js
@@ +919,5 @@
> +      if (typeof obj[name] != "function") {
> +        return;
> +      }
> +      obj[name] =
> +        Cu.exportFunction(obj[name], evalWindow, { allowCrossOriginArguments: true });

That would be great to add a comment about why we need to use exportFunction here.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> I'm a bit hesitant about this. If a security check is optional it almost
> means we can remove that check entirely... Could you explain me why we need
> to do this exactly?

It's like Xrays - we want a safe default behavior so that each and every API entry point doesn't have to worry about inadvertently stringifying cross-origin objects (like Location). But just like .wrappedJSObject, we occasionally need an opt-out, which is what this option does.

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8480448 [details] [diff] [review]
Test console's cd() against sandboxed iframes

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

Works great! Thanks. Once this is reviewed by msucan, I'll push everything together.

Comment 19

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=698b1fc40aac

Comment 20

[Bobby Holley (:bholley)]

Fixed a rooting hazard:

https://tbpl.mozilla.org/?tree=Try&rev=e5444baa5c09

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 8480448 [details] [diff] [review]
Test console's cd() against sandboxed iframes

Switching to Panos as Mihai is unavailable these days.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8480448 [details] [diff] [review]
Test console's cd() against sandboxed iframes

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

::: toolkit/devtools/webconsole/test/sandboxed_iframe.html
@@ +1,2 @@
> +<html>
> +<head><title></title></head>

Nit: adding a title like "Sandboxed iframe" might be helpful when looking at screenshots from tbpl.

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: Part 3 - Test console's cd() against sandboxed iframes

Comment 24

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1048b554fdf8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc38635dd4e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d13a843f2163

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1048b554fdf8
https://hg.mozilla.org/mozilla-central/rev/7fc38635dd4e
https://hg.mozilla.org/mozilla-central/rev/d13a843f2163

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Bobby, do you think we should uplift the patches to aurora & beta to have this bug fixed? Or just ride the train? Thanks

Comment 27

[Bobby Holley (:bholley)]

Yes, we should uplift it. SOrry for letting that slip through the cracks.

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 8483504 [details] [diff] [review]
Part 3 - Test console's cd() against sandboxed iframes

Request applies to all patches.

Approval Request Comment
[Feature/regressing bug #]: bug 930091
[User impact if declined]: cd(sandboxedIframe) won't work from console
[Describe test coverage new/current, TBPL]: Good automated test coverage
[Risks and why]: Low risk. These patches just introduce an opt-out to the new security behavior.
[String/UUID change made/needed]: None.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b2def63f735
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb724ae6dc06
https://hg.mozilla.org/releases/mozilla-aurora/rev/07d5135ca2bc

Part 1 needs rebasing for beta uplift.

Comment 30

[Bobby Holley (:bholley)]

So, part 1 isn't going to work on beta. The issue is that exportFunction was still making cloning (rather than non-cloning) function forwarders on FF33, and a cross-origin object will break as soon as it's passed to the structured clone machinery.

So I hacked something up in JS. Not pretty, but should do the trick. Quick feedback here would be appreciated, because we're getting close to release.

Comment 31

[Bobby Holley (:bholley)]

Attachment: Find a clever way to work around COW restrictions on beta. v1

Comment 32

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8491656 [details] [diff] [review]
Find a clever way to work around COW restrictions on beta. v1

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

This is a very clever trick :) I guess we will need to file a follow up bug to remove it?

::: toolkit/devtools/server/actors/webconsole.js
@@ +945,3 @@
>      for (let name in helpers.sandbox) {
>        let desc = Object.getOwnPropertyDescriptor(helpers.sandbox, name);
> +      maybeExport(desc, 'get');

I personally would avoid to use the word 'maybe' in code as much as possible and instead do the if checks here... but I'm not going to start a debate about a temporary code for sure, so feel free to leave it like this if you prefer.

Comment 33

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #32)
> Comment on attachment 8491656 [details] [diff] [review]
> Find a clever way to work around COW restrictions on beta. v1
> 
> Review of attachment 8491656 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a very clever trick :) I guess we will need to file a follow up bug
> to remove it?

It's only going to land on beta.

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Bobby, do you know when this is going to land to beta? Thanks

Comment 35

[Alexandre Poirot [:ochameau]]

Comment on attachment 8491656 [details] [diff] [review]
Find a clever way to work around COW restrictions on beta. v1

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

Sorry for the delay, but that looks good (even if very cryptic!).
And I gave it a try on beta on works great.

Comment 36

[Bobby Holley (:bholley)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #34)
> Bobby, do you know when this is going to land to beta? Thanks

Was just waiting for review.

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/6cdc428e3e62
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/0ae1af037f6e