Closed Bug 1051224 Opened 7 years ago Closed 7 years ago

Regression: Can't cd into sandboxed iframe

Categories

(Core :: XPConnect, defect)

33 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox33 + fixed
firefox34 + fixed
firefox35 --- fixed

People

(Reporter: d.huigens, Assigned: bholley)

References

Details

(Keywords: regression, testcase)

Attachments

(5 files, 2 obsolete files)

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
May a dev with higher privileges add bug 930091 in the "Blocks" field and change the product component, please.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bobbyholley)
Keywords: regression, testcase
Version: 34 Branch → 33 Branch
Component: Untriaged → XPConnect
Product: Firefox → Core
I suspect this will actually need to be fixed in the developer tools rather than xpconnect...
Blocks: 930091
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?
Flags: needinfo?(bobbyholley) → needinfo?(rcampbell)
looking...
Flags: needinfo?(rcampbell)
Flags: needinfo?(rcampbell)
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.
Flags: needinfo?(poirot.alex)
Bobby: quit it.
Flags: needinfo?(rcampbell)
I don't have immediate availability, but I'm keeping the needinfo in case I find some time next week.
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.
Attachment #8474615 - Flags: feedback?(bobbyholley)
Flags: needinfo?(poirot.alex)
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.
Attachment #8474615 - Flags: feedback?(bobbyholley) → feedback-
Some of this will probably seem familiar. ;-)
Attachment #8475724 - Flags: review?(gkrizsanits)
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?
(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 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?
Attachment #8475724 - Flags: review?(gkrizsanits) → review+
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.
Attachment #8474615 - Attachment is obsolete: true
Attachment #8480448 - Flags: feedback?
Attachment #8480448 - Flags: feedback? → feedback?(bobbyholley)
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.
Attachment #8475725 - Flags: review?(poirot.alex) → review+
Attachment #8480448 - Flags: review?(mihai.sucan)
(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 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.
Attachment #8480448 - Flags: feedback?(bobbyholley) → feedback+
Comment on attachment 8480448 [details] [diff] [review]
Test console's cd() against sandboxed iframes

Switching to Panos as Mihai is unavailable these days.
Attachment #8480448 - Flags: review?(mihai.sucan) → review?(past)
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.
Attachment #8480448 - Flags: review?(past) → review+
Bobby, do you think we should uplift the patches to aurora & beta to have this bug fixed? Or just ride the train? Thanks
Flags: needinfo?(bobbyholley)
Yes, we should uplift it. SOrry for letting that slip through the cracks.
Assignee: nobody → bobbyholley
Flags: needinfo?(bobbyholley)
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.
Attachment #8483504 - Flags: approval-mozilla-beta?
Attachment #8483504 - Flags: approval-mozilla-aurora?
Attachment #8483504 - Flags: approval-mozilla-beta?
Attachment #8483504 - Flags: approval-mozilla-beta+
Attachment #8483504 - Flags: approval-mozilla-aurora?
Attachment #8483504 - Flags: approval-mozilla-aurora+
Attachment #8475725 - Flags: approval-mozilla-beta+
Attachment #8475725 - Flags: approval-mozilla-aurora+
Attachment #8475724 - Flags: approval-mozilla-beta+
Attachment #8475724 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(bobbyholley)
Attachment #8491656 - Flags: review?(poirot.alex)
Attachment #8491656 - Flags: review?(gkrizsanits)
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.
Attachment #8491656 - Flags: review?(gkrizsanits) → review+
(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.
Bobby, do you know when this is going to land to beta? Thanks
Flags: needinfo?(bobbyholley)
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.
Attachment #8491656 - Flags: review?(poirot.alex) → review+
(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
Flags: needinfo?(bobbyholley)
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.