Last Comment Bug 777705 - Access to Components.interfaces throws NS_ERROR_OUT_OF_MEMORY when using expanded principals
: Access to Components.interfaces throws NS_ERROR_OUT_OF_MEMORY when using expa...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Gabor Krizsanits [:krizsa :gabor] (PTO until july 3)
:
Mentors:
Depends on:
Blocks: 734891
  Show dependency treegraph
 
Reported: 2012-07-26 06:57 PDT by Alexandre Poirot [:ochameau]
Modified: 2012-08-21 06:31 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Bug 777705 - Default policy for expanded principals (3.45 KB, patch)
2012-07-27 12:18 PDT, Gabor Krizsanits [:krizsa :gabor] (PTO until july 3)
mrbkap: feedback+
Details | Diff | Review
Bug 777705 - Default policy for expanded principals (4.22 KB, patch)
2012-08-02 04:00 PDT, Gabor Krizsanits [:krizsa :gabor] (PTO until july 3)
no flags Details | Diff | Review
Default policy for expanded principals (4.91 KB, patch)
2012-08-14 05:15 PDT, Gabor Krizsanits [:krizsa :gabor] (PTO until july 3)
mrbkap: review+
Details | Diff | Review

Description Alexandre Poirot [:ochameau] 2012-07-26 06:57:36 PDT
Here is minimal testcase:
  var sb = Components.utils.Sandbox(["http://mozilla.org"]);
  Components.utils.evalInSandbox("Components.interfaces", sb);

/*
Exception: Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIXPCComponents_Utils.evalInSandbox]
undefined:2
*/

Expanded principal being introduced by bug 734891.
Comment 1 Alexandre Poirot [:ochameau] 2012-07-26 07:16:01 PDT
gabor, here is an SDK branch which uses expanded principals:
https://github.com/ochameau/addon-sdk/compare/master...cross-domain-content-script
All tests should pass. Only one may fail "testCrossDomainFeatures" which uses multiple domains and I may have miswritten it.
Comment 2 Gabor Krizsanits [:krizsa :gabor] (PTO until july 3) 2012-07-27 04:50:09 PDT
Right. So this all rooted in the security check in XPCWrappedNative::CallMethod (Bug 735281). Now there is two way to fix this. Remove that check finally, which would be the proper solution imo, even though it's a bit risky... Or I can just add some custom hack for nsExpandedPrincipals to nsScriptSecurityManager::LookupPolicy to return some policy instead of throwing because it has no origin/uri. Bobby, what do you think?
Comment 3 Bobby Holley (busy) 2012-07-27 08:50:25 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> Right. So this all rooted in the security check in
> XPCWrappedNative::CallMethod (Bug 735281). Now there is two way to fix this.
> Remove that check finally, which would be the proper solution imo, even
> though it's a bit risky... Or I can just add some custom hack for
> nsExpandedPrincipals to nsScriptSecurityManager::LookupPolicy to return some
> policy instead of throwing because it has no origin/uri. Bobby, what do you
> think?

How nasty would the hack be? I'd kind of like to limp along with this security check for the time being, given how devastating it would be if content could get unfettered Components access, I'd be in favor of keeping it in (unless there was a pressing need to remove it). I'll hopefully be making it inaccessible to content soon (by moving XBL to its own compartment/scope), at which point we can remove all this stuff.
Comment 4 Gabor Krizsanits [:krizsa :gabor] (PTO until july 3) 2012-07-27 12:17:44 PDT
(In reply to Bobby Holley (:bholley) from comment #3)
> How nasty would the hack be? 

It's nothing really scary I'd say. I attached a quick fix, but it's not very clean from the diff so it's really only this check: 

if (nsCOMPtr<nsIExpandedPrincipal> exp = do_QueryInterface(aPrincipal)) 
{
    // For expanded principals domain origin is not defined so let's just
    // use the default policy
    dpolicy = mDefaultPolicy;
}

before the GetPrincipalDomainOrigin call and all the domain string magic is moved into the else branch. It is done only once per principal btw. So shall we stick to this version then?
Comment 5 Gabor Krizsanits [:krizsa :gabor] (PTO until july 3) 2012-07-27 12:18:30 PDT
Created attachment 646667 [details] [diff] [review]
Bug 777705 - Default policy for expanded principals
Comment 6 Bobby Holley (busy) 2012-07-28 01:43:01 PDT
Comment on attachment 646667 [details] [diff] [review]
Bug 777705 - Default policy for expanded principals

Hm, could we instead just return NULL in GetDomain like SystemPrincipal does?

Anyway, this getting into caps stuff that I don't at all understand. I think we need mrbkap here.
Comment 7 Gabor Krizsanits [:krizsa :gabor] (PTO until july 3) 2012-07-28 06:14:44 PDT
(In reply to Bobby Holley (:bholley) from comment #6)
> Comment on attachment 646667 [details] [diff] [review]
> Bug 777705 - Default policy for expanded principals
> 
> Hm, could we instead just return NULL in GetDomain like SystemPrincipal does?

We do, that's the problem here. SystemPrincipal does never reach this code (for that we just return sooner allowing everything). GetPrincipalDomainOrigin will return error if cannot find an uri. So for ExpandedPrincipal I could either allow everyting like for system principal (I didn't like that plus I should do a QI in a place that is hotter) or use the default policy (so QI only once then it's cached)

> 
> Anyway, this getting into caps stuff that I don't at all understand. I think
> we need mrbkap here.

Ok.
Comment 8 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-08-01 16:02:52 PDT
Comment on attachment 646667 [details] [diff] [review]
Bug 777705 - Default policy for expanded principals

This stuff needs to simply die. Do we have a bug on doing that? I wonder if we can kill the site policy stuff with less lead time than the capabilities stuff. I can't even find a use of it in our own code (testing or not) and I have a hard time believing anyone external is using it. I guess I'll send a newsgroup message asking about it.

The patch is fine for me. It's unfortunate that you had to add yet another level of nesting here, but given how the code is written, I don't see a way around it.
Comment 9 Gabor Krizsanits [:krizsa :gabor] (PTO until july 3) 2012-08-02 04:00:05 PDT
Created attachment 648304 [details] [diff] [review]
Bug 777705 - Default policy for expanded principals

(In reply to Blake Kaplan (:mrbkap) from comment #8)
> Comment on attachment 646667 [details] [diff] [review]
> Bug 777705 - Default policy for expanded principals
> 
> This stuff needs to simply die. Do we have a bug on doing that? 

Sounds great :)

> 
> The patch is fine for me. It's unfortunate that you had to add yet another
> level of nesting here, but given how the code is written, I don't see a way
> around it.

Me neither, except maybe some big refactoring, but getting rid of the whole thing sounds so much more tempting... I might regret saying this but I'm even willing to put some work in this if someone can give me some help. Although it wont happen in this month for sure.

Anyway, I added the line that was throwing to the ExpandedPrincipal related test, and fixed the line indentation in this function. (for some reason the char* parsing related magic used 2 spaces while the whole file is using 4). Tested it on try as well just in case https://tbpl.mozilla.org/?tree=Try&rev=87538bb3c5df looks good to me.
Comment 10 Alexandre Poirot [:ochameau] 2012-08-02 07:34:17 PDT
Gabor, with this try server build, I get some crashes.
That ends up being one of my most complex bug recipe I cooked up so far!!
The list of ingredient is quite impressive:
 - Two sandboxes:
   - One with just system principal 
    (No idea why it works if you execute its content in scratchpad)
   - Another one using expanded principal, with just one origin
 - Use loadSubScript to load the crashing code
 - Use new Error() which trigger the crash

Here is a scratchpad crashing code:
  // Here window is the toplevel xul one, so equivalent to give systemPrincipal
  var s1 = Components.utils.Sandbox(window);
  Components.utils.evalInSandbox("new " + function () {
    // If you take this 3 lines and put them directly in scratchpad, it works?!
    Components.utils.import("resource://gre/modules/Services.jsm");
    // Use expanded principals here
    var s2 = Components.utils.Sandbox(["http://mozilla.org"]);
    // You need to create a file somewhere and reference it here
    // test.js just contains: new Error('foo');
    Services.scriptloader.loadSubScript("file:///C:/test.js", s2);
  }, s1);
Comment 11 Gabor Krizsanits [:krizsa :gabor] (PTO until july 3) 2012-08-02 08:24:09 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #10)
> Gabor, with this try server build, I get some crashes.

Seems like an unrelated bug so I just create a new bug for it. Bug 779858.

Or is this something this patch has introduced? (shouldn't be...)
Comment 12 Gabor Krizsanits [:krizsa :gabor] (PTO until july 3) 2012-08-02 09:02:35 PDT
Not unrelated. So in this example the CheckXPCPermissions call fails, and when the error string is generated, the code does not expect the URI to be null.

So my patch is not ready for sure... I also need to figure out why the CheckXPCPermissions fails in this case.
Comment 13 Gabor Krizsanits [:krizsa :gabor] (PTO until july 3) 2012-08-14 05:15:04 PDT
Created attachment 651710 [details] [diff] [review]
Default policy for expanded principals
Comment 14 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-08-14 16:42:22 PDT
Comment on attachment 651710 [details] [diff] [review]
Default policy for expanded principals

Looks good.
Comment 16 Ed Morley [:emorley] 2012-08-21 06:31:08 PDT
https://hg.mozilla.org/mozilla-central/rev/b48a2499872c

Note You need to log in before you can comment on or make changes to this bug.