239969 - implement a full security manager for xpcshell

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[timeless]

Code:
a=Components.classes["@mozilla.org/xmlextras/soap/call;1"].createInstance()
a.transportURI="http://simpledotnetthingee/SimpleService/Service1.asmx"
a.encode(0, "HelloWorld", "http://dotnet.com/webservices/", 0, null, [].length, 
[])
a.asyncInvoke(function(response, soapcall, error) {dump(response, soapcall, 
error);})

expected results:
js> a=Components.classes["@mozilla.org/xmlextras/soap/call;1"].createInstance()
[xpconnect wrapped (nsISupports, nsISOAPMessage, nsISOAPCall)]
js> a.transportURI="http://simpledotnetthingee/SimpleService/Service1.asmx"
http://simpledotnetthingee/SimpleService/Service1.asmx
js> a.encode(0, "HelloWorld", "http://dotnet.com/webservices/", 0, null, 
[].length, [])
js> a.asyncInvoke(function(response, soapcall, error) {dump(response, soapcall, 
error);})
[xpconnect wrapped (nsISupports, nsIDOMEventListener, nsISOAPCallCompletion)]

Actual results:
...
js> a.asyncInvoke(function(response, soapcall, error) {dump(response, soapcall, 
error);})
uncaught exception: [Exception... "Component returned failure code: 0x80004005 
(NS_ERROR_FAILURE) [nsISOAPCall.asyncInvoke]"  nsresult: "0x80004005 
(NS_ERROR_FAILURE)"  location: "JS frame :: typein :: <TOP_LEVEL> :: line 36"  
data: no]

The problem is that soap asks for the global ssm service and decides that the 
stuff just isn't privy to it.

There are a few solutions (one is to fix soap), but for the time being, i 
decided to hack xpcshell

Comment 1

[timeless]

Attachment: security manager (draft)

Comment 2

[timeless]

Comment on attachment 145671 [details] [diff] [review]
security manager (draft)


+NS_IMPL_ISUPPORTS1(FullTrustSecMan, nsIScriptSecurityManager)

should be:

+NS_IMPL_ISUPPORTS2(FullTrustSecMan, nsIScriptSecurityManager, 
nsIXPCScriptSecurityManager)

Comment 3

[Jean-Marc Paulin]

Hi,

I had a similar issue not being able to load an XML Document from a javascript
running from xpcshell... I tracked it down to : the script is compiled without
any principles).

I have hacked xpcshell as follow:
1. Get the all powerfull SystemPrincipal :
2. Then compile the script using the System Principals.

replace xpcshell.cpp line 576:

-         script = JS_CompileFileHandle(cx, obj, filename, file);

with:

+{
+  nsCOMPtr<nsIPrincipal> princ ;
+  JSPrincipals *jsp=NULL;
+  nsresult rv = NS_OK;
+
+  nsCOMPtr<nsIScriptSecurityManager> securityManager =
do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
+  if ( securityManager && NS_SUCCEEDED( rv ) ) {
+    rv=securityManager->GetSystemPrincipal(getter_AddRefs(princ));
+  }
+  rv = princ->GetJSPrincipals( cx , &jsp);
+  if ( NS_SUCCEEDED( rv ) ) {
+    script = JS_CompileFileHandleForPrincipals(cx, obj,filename, file , jsp); 
           
+  }
+}

Hope this helps. I know this is gives full control to js executed this way, but
this is executed from the command line, so I would assume user knows what thay
are doing...

Let me know if this helps...

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 145671 [details] [diff] [review]
security manager (draft)

Yeah, do we need a secman at all if we just give the sysprin to those scripts? 
I'd be surprised if we did, but maybe there are some cases where sysprin isn't
enough?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 239969-xpcshell-principals-0.patch

Patch based on Jean-Marc's diff.  Wrapped relevant sections with
XPCONNECT_STANDALONE, also do some error checking.   Right now it bails if
GetSystemPrincipals or GetJSPrincipals fails, do we want to try without
principals instead?  I'm not sure if it'll make a difference what we do in
practice.

Comment 6

[timeless]

Comment on attachment 166736 [details] [diff] [review]
239969-xpcshell-principals-0.patch

>         DoBeginRequest(cx);
see beginrequest^^^

>+                return;
thou shalt not return without ending thy request.

>         DoEndRequest(cx);
see endrequest^^^

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 239969-xpcshell-principals-1.patch

Whoops, yep, forgot the EndRequests.

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 166783 [details] [diff] [review]
239969-xpcshell-principals-1.patch

Please condition the DEPENDS on XPCONNECT_STANDALONE as well.

Also, printing a warning if we can't get the securityManager will save people
_just_like_us_ from chasing weird sysprin bugs in the future.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 239969-xpcshell-principals-2.patch

Updated; there were a few other places that needed to be cought (the Load()
function, and also interactive reading from the console).  So, I store away the
jsprin in a global and just use it everywhere.	All 3 usages seem to work (-f
from the command line, load("foo.js"), and interactive).

(There's no makefile flag for XPCONNECT_STANDALONE; the loader seems to have
caps unconditionally in REQUIRES, so it doesn't seem to hurt anything.)

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 166820 [details] [diff] [review]
239969-xpcshell-principals-2.patch

r=shaver with the irc nits picked.  thanks!

Comment 11

[David Bradley]

Is there an alternative to XPCONNECT_STANDALONE here? alecf mentioned a desire
to try to eliminate it in comment here
https://bugzilla.mozilla.org/show_bug.cgi?id=121438#c34.

Off the top of my head I don't know what an alternative would be and I'm fuzzy
on why this needs to be standalone only. Just wanted to bring it up for discussion. 

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

*** Bug 270497 has been marked as a duplicate of this bug. ***

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Whatever happened to this? 

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I honestly don't remember; I must've dropped it on the floor for some reason.  Is this still needed?

Comment 15

[Jeff Walden [:Waldo]]

Yes; a security manager is needed at the very least for bug 374071.  It has a patch which was committed, but the patch requires a security manager.  xpcshell's lack of said security manager results in the unit test crashing, so the patch had to be backed out.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

It seems that Vlad's patch was checked in, but timeless' patch wasn't. I think that I'll need the latter to land in order to land bug 374071.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Stubbed nsIScriptSecurityManager

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fixing a couple of problems

I'd missed a couple of *_retval = PR_TRUE s, and and added an incorrect HOLD (the DROP was necessary, though).

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Last one, I promise!

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

This bug is fully fixed now.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

I backed this patch out, because it caused bustage and was checked in when the tree was closed.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in again.

Comment 23

[Jeff Walden [:Waldo]]

The code exercised by test_protocolproxyservice.js depends on the presence of the security manager, now that bug 374071 is fixed, so we can call this enough for now.