468065 - ScriptSecurityManager accessors need to be fixed

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[timeless]

Attachment: example changes

There are a couple of accessors for SSM, the two main ones according to:

http://mxr-test.konigsberg.mozilla.org/mozilla-central/ident?i=GetSecurityManager

    * content/base/public/nsContentUtils.h
487   static nsIScriptSecurityManager* GetSecurityManager()
488   {
489     return sSecurityManager;
490   }

    * js/src/xpconnect/src/XPCWrapper.h
184   static nsIScriptSecurityManager *GetSecurityManager() {
185     extern nsIScriptSecurityManager *gScriptSecurityManager;
186 
187     return gScriptSecurityManager;
188   }

The official API is this:
http://mxr-test.konigsberg.mozilla.org/mozilla-central/ident?i=getSecurityManagerForJSContext&scriptidly=1

which no one seems to use. And given that the used API is (void), afaict, it'll require all consumers to try to get a cx.

I looked at one example:

http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#183

181 static nsresult IsCapabilityEnabled(const char *capability, PRBool *enabled)
182 {
183   nsIScriptSecurityManager *secMan = nsContentUtils::GetSecurityManager();
184   if (!secMan)
185     return NS_ERROR_FAILURE;
186 
187   return secMan->IsCapabilityEnabled(capability, enabled);
188 }

this part should probably go away entirely.

1068 nsXMLHttpRequest::Init()
1069 {
1079   JSContext *cx;

1085   nsIScriptSecurityManager *secMan = nsContentUtils::GetSecurityManager();

this needs to be changed.

1087   if (secMan) {
1088     secMan->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
1089   }

1093   nsIScriptContext* context = GetScriptContextFromJSContext(cx);

And yes, I'm vaguely aware that dom likes being fast, but it's also too easy with the current apis for something like XMLHttpRequest to be written broken

Comment 1

[Boris Zbarsky [:bzbarsky]]

That patch is broken, since there might not be a JSContext.

Further, do we actually have different security managers in practice per JSContext?  If we do, we have a serious problem, since script can easily cross contexts in practice.  I don't think we should be supporting that mode of operation.

Comment 2

[timeless]

we do as I recently discovered, DOM Workers provide their own.

I'm all in favor of disabling DOM Workers until things are resolved

Comment 3

[Boris Zbarsky [:bzbarsky]]

I only see two implementations of nsIScriptSecurityManager in our tree: the one in caps/ and the one xpcshell uses.  What am I missing?

Comment 4

[timeless]

475 NS_IMPL_ISUPPORTS5(nsScriptSecurityManager,
476                    nsIScriptSecurityManager,
477                    nsIXPCSecurityManager,
478                    nsIPrefSecurityCheck,
479                    nsIChannelEventSink,
480                    nsIObserver)

http://mxr-test.konigsberg.mozilla.org/mozilla-central/ident?i=NS_DECL_NSIXPCSECURITYMANAGER

Referenced (in 5 files total)

the XPConnect interface that's interesting is nsIXPCSecurityManager, and there are a number of implementations.

54 interface nsIScriptSecurityManager : nsIXPCSecurityManager

is an extension of that interface, for which there are indeed only two implementations. I suppose it depends on what you're doing, but I think that people implementing nsIXPCSecurityManager expected to be called, certainly that'd be why I'd write it (if i weren't a test).

Comment 5

[Boris Zbarsky [:bzbarsky]]

> the XPConnect interface that's interesting is nsIXPCSecurityManager

But you're claiming that the code that should be changed is code that gets an nsIScriptSecurityManager, and in particular that it should be changed to use a different method that returns an nsIScriptSecurityManager.  That would always return null in DOM Workers, no?

I agree that if the caller wanted to use nsIXPCSecurityManager functions then it would be an interesting question as to which one to use.  But that's not what's happening here.  The caller really does want nsIScriptSecurityManager.

The nsIXPCSecurityManager in question is presumably called by internal XPConnect stuff, of course.

I wonder whether it would have killed you to make the situation clear in comment 0, btw?

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, I just talked to timeless.  He's worried about XHR touching the SSM from DOM workers.  But we proxy all the XHR stuff to the main thread, right?

Comment 7

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

Yes. All XHRs happen on the main thread, and they never interact with the DOM worker JSContexts.

Comment 8

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.