Closed
Bug 397791
Opened 17 years ago
Closed 16 years ago
XOW used for same-origin "document" access
Categories
(Core :: XPConnect, defect, P1)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: jst)
References
Details
(Keywords: dev-doc-complete, perf)
Attachments
(1 file, 2 obsolete files)
12.46 KB,
patch
|
jst
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
BUILD: Current trunk build STEPS TO REPRODUCE: 1) Profile the "HTML" part of the testcase in bug 375470. 2) Look at the results ACTUAL RESULTS: 10% of the time in the testcase is spent under IsWrapperSameOrigin EXPECTED RESULTS: Shouldn't be any XOW involved ADDITIONAL INFORMATION: In particular, in nsWindowSH::GetProperty we have *vp being an XOW for the XPCWrappedNative for the HTMLDocument. So we do a security check when doing the GetWrappedNativeOfJSObject() there. We also do security checks when getting the "body" property. I frankly don't see the point of that code in the tail of nsWindowSH::GetProperty; it was there to bail out early without doing a security check on the get, but we never do the security check now, with XOW, right? Can we just remove that whole chunk? And why is |document| an XOW here? It really shoudn't be. Really.
Flags: blocking1.9?
Comment 1•17 years ago
|
||
(In reply to comment #0) > And why is |document| an XOW here? It really shoudn't be. Really. The worry was that one could have a reference to a document, have another site do a document.open (document.open is allAccess) and then bypass access checks. Perhaps we should get rid of IsWrapperSameOrigin in favor of a notifying scheme, where a document or window notifies each of its wrappers when it changes principals.
Comment 2•17 years ago
|
||
(In reply to comment #0) > I frankly don't see the point of that code in the tail of > nsWindowSH::GetProperty; it was there to bail out early without doing a > security check on the get, but we never do the security check now, with XOW, > right? Can we just remove that whole chunk? Sorry, missed this when I first commented. I left that code in to preserve the NS_SUCCESS_I_DID_SOMETHING return value for fear of breaking XPCNWs or XOWs. Is it sufficient we can replace the GetWrappedNativeOfJSObject/QI with just a test against JSVAL_VOID?
Reporter | ||
Comment 3•17 years ago
|
||
No, that wouldn't do; that would get all sorts of random props off windows via XPCNativeWrapper, I think. It's worth changing the docs on that chunk of code to make it clear what it's doing. And to only run it when we're working with a wrapper, possibly... That also said, isn't forwarding to the inner in nsWindowSH::GetProperty broken for wrappers (including XOW)? Or am I missing something?
Comment 4•17 years ago
|
||
We can't (and shouldn't) forward for wrappers. The wrappers that call into this code with |obj| not being on the prototype chain of an actual wrapped native don't want ad-hoc properties. Actually, with that in mind, perhaps we don't have to worry so much about resolving random properties with a reduced-strength check there.
Reporter | ||
Comment 5•17 years ago
|
||
XOW doesn't want ad-hoc properties (after security checks)? Note also that in this case there's really no point to the security check that we're doing in the unwrapping process....
Comment 6•17 years ago
|
||
When the XOW is cross origin, then it behaves like an XPCNW. For same origin accesses, then we'll happily resolve any property that you want. The security check around unwrapping XOWs is to protect C++ APIs from having to security check every security sensitive argument. The idea was that an attempt to pass a XOW into an unexpected API should simply refuse to unwrap the XOW.
We can probably make window.document not be allAccess any more since microsoft has already made that change with IE7. It'd still be possible to call document.open() on your own documents though of course, but that should be same-origin access. Actually, if we did that maybe we could make it so that document.open() never changed the principal of the document, that would be nice for a number of reasons. For extra security we could add a check that makes sure that the subject principal subsumes the object principal in the .open implementation.
I filed bug 397828 on this.
Reporter | ||
Comment 9•17 years ago
|
||
I think we should do comment 7 and remove the XOW-ness here, yeah. That seems like the simplest way forward on this bug.
Depends on: 397828
Comment 10•17 years ago
|
||
We're hoping to land XOWs on the 1.8 branch, and we can't make that change on that branch so I'd rather fix the underlying issue (also, it'd be nice to speed up scripts that have a lot of |window.foo| uses).
Not sure if this is bad enough that it's a blocker, but we really do want this fixed so I would definitely approve a patch. Might even be a blocker if we can attribute enough slowdown to it. But in any case, lets try to get a patch for this.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Assignee: nobody → mrbkap
Comment 12•17 years ago
|
||
Boris, is the time in IsWrapperSameOrigin spent in the actual checking whether the two principals are equal or in getting the subject/object principals? The question really is, is it worth it to cache the principal on the XOW itself?
I would imagine that the two principals are going to be the same one in the vast majority of the cases. So if we could cache them we'd end up with just a pointer-compare in most cases.
Comment 14•17 years ago
|
||
Well, I do avoid calling anything if they compare equal (see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp&rev=1.15&root=/cvsroot&mark=267-270#267). I suspect the real problem is computing them. Caching the object principals requires a little work to deal with the underlying object changing principals suddenly but fairly straightforward. I really want to cache the subject principal too and argue that it's impossible for the same XOW to be used by scripts from two different origins, but that's harder to argue. If we could do that, though, then IsWrapperSameOrigin would be trivial and would fall off the map completely.
Reporter | ||
Comment 15•17 years ago
|
||
The time is almost entirely spent getting the principals. As you observed, the common case is a pointer compare once that's done.
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Updated•17 years ago
|
Priority: P3 → P2
Comment 17•17 years ago
|
||
Boris, what do the profiles look like now after jst's GetSecurityManager fix?
Reporter | ||
Comment 18•17 years ago
|
||
I won't be able to answer that very well until I'm back in Chicago in January....
Comment 19•17 years ago
|
||
(In reply to comment #17) > Boris, what do the profiles look like now after jst's GetSecurityManager fix? > Don't we still want to get XOW out of the way?
Assignee | ||
Comment 20•17 years ago
|
||
Blake, if we change document.load() to not permit cross origin loads even if the running script has elevated priveleges, could we simply stop using XOW's for same origin document objects? Or do you know of anything else that would prevent us from doing that now that window.document is no longer allAccess?
Comment 21•17 years ago
|
||
Yeah, I think that would let us avoid creating XOWs for documents at all. All other methods (other meaning "not document.load or document.open") create an entirely new document object, so by restricting document.load (and restricting access to document.open) we no longer have to worry about changing principals because the new principals *must* be same-origin.
We also have to change document.open to not use the principal of the caller if it's of a different origin. We might actually want to make it throw even if the caller is of a different origin. This might break a few extensions, but it seems like such extensions are on thin ice anyway, security wise.
Reporter | ||
Comment 23•16 years ago
|
||
> Boris, what do the profiles look like now after jst's GetSecurityManager fix?
On current trunk, IsWrapperSameOrigin is 5% of the total time. The main callers are:
XPCWrapper::Unwrap (called from XPCWrappedNative::GetWrappedNativeOfJSObject, mostly called from XPCCallContext::XPCCallContext, called from XPC_WN_CallMethod and XPC_WN_Helper_NewResolve).
XPC_XOW_GetOrSetProperty (called from js_NativeGet).
The time under IsWrapperSameOrigin breaks down as:
6844 IsWrapperSameOrigin(JSContext*, JSObject*)
3606 nsScriptSecurityManager::GetObjectPrincipal
1985 nsScriptSecurityManager::GetSubjectPrincipal
338 nsCOMPtr::~nsCOMPtr
217 nsGetterAddRefs::operator nsIPrincipal**()
Updated•16 years ago
|
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Comment 24•16 years ago
|
||
Moving to P1 - if we are going to get this in - must be before B3.
Priority: P2 → P1
Assignee | ||
Comment 25•16 years ago
|
||
This should basically fix this. This makes document.open() and (XML) document.load() not be callable if the result of the call would mean the origin of the document would change. This basically means that the tow methods are not callable from C++, and chrome and elevated privilege code gets no special treatment wrt these methods. If there are extensions out there that ever call document.open() (or write/writeln), or document.load() to load data from an origin other than that of the current document's origin, they're likely to break after this lands. If we don't take this change, we'd need to add all flavors of XML documents to the list in XPCWrapper.h as well.
Attachment #298631 -
Flags: superreview?(bzbarsky)
Attachment #298631 -
Flags: review?(jonas)
Reporter | ||
Comment 26•16 years ago
|
||
Comment on attachment 298631 [details] [diff] [review] Prevent a documents origin from ever changing. >+++ b/content/html/document/src/nsHTMLDocument.cpp >+ if (NS_FAILED(callerPrincipal->Subsumes(NodePrincipal(), &subsumes)) || >+ !subsumes || >+ NS_FAILED(NodePrincipal()->Subsumes(callerPrincipal, &subsumes)) || Why not just use Equals() here instead, the way the code before SetNewDocument() did? That seems clearer, and I can't think of reasons to do the above unless we change something radically about principals... >+++ b/content/xml/document/src/nsXMLDocument.cpp >@@ -289,27 +281,19 @@ nsXMLDocument::OnChannelRedirect(nsIChannel >+ rv = NodePrincipal()->GetURI(getter_AddRefs(codebase)); >+ NS_ENSURE_SUCCESS(rv, rv); What if NodePrincipal() is system? We should handle that case. >@@ -364,21 +348,7 @@ nsresult > nsXMLDocument::GetLoadGroup(nsILoadGroup **aLoadGroup) Why bother keeping this? It's unused after this patch, no? If we do still need it for some reason, the new implementation looks kind of wrong to me, since usually this document will have no useful document loadgroup. >@@ -427,6 +378,23 @@ nsXMLDocument::Load(const nsAString& aUrl, >+ // Get security manager, check to see if the current document is s/if/whether/ >+ // documents principal for this check so that we don't end up in a s/documents/document's/ >+ rv = secMan->CheckLoadURIWithPrincipal(NodePrincipal(), uri, This seems like the wrong (or at least an insufficient) check. For example, this will succeed any time uri is an HTTP URI. You want a CheckSameOriginURI check or something, no? >+ if (NS_FAILED(rv)) { >+ // We need to return success here so that JS will get a proper >+ // exception thrown later. Native calls should always result in >+ // CheckConnect() succeeding, but in case JS calls C++ which calls >+ // this code the exception might be lost. I'm not following this comment, especially since you're not calling CheckConnect. CheckLoadURI does not set a pending exception or anything. Do we have a testcase to make sure that creating a document and calling load() on it still works with this patch? If not, can we add one?
Comment 27•16 years ago
|
||
> If there are extensions out there that ever call document.open() (or
> write/writeln), or document.load() to load data from an origin other than that
> of the current document's origin, they're likely to break after this lands.
Will this change affect web sites or just extensions?
Comment 28•16 years ago
|
||
Because it is now impossible to get one's hand on a document from a different domain and document.load is currently restricted to same-origin for content code, this change should only affect extensions.
Assignee: mrbkap → jst
Reporter | ||
Updated•16 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 29•16 years ago
|
||
The only exception to that is content JS with elevated privileges (i.e. UniversalXPConnect or what not).
Assignee | ||
Comment 30•16 years ago
|
||
(In reply to comment #26) > >+++ b/content/xml/document/src/nsXMLDocument.cpp > >@@ -289,27 +281,19 @@ nsXMLDocument::OnChannelRedirect(nsIChannel > > >+ rv = NodePrincipal()->GetURI(getter_AddRefs(codebase)); > >+ NS_ENSURE_SUCCESS(rv, rv); > > What if NodePrincipal() is system? We should handle that case. It can't be, can it? chrome:// URIs can't redirect can they? And document with system principals now can't load anything that isn't also chrome. I'm attaching another patch that addresses the rest of your comments (AFAIK). Thanks for the feedback.
Assignee | ||
Comment 31•16 years ago
|
||
Attachment #298631 -
Attachment is obsolete: true
Attachment #298808 -
Flags: superreview?(bzbarsky)
Attachment #298808 -
Flags: review?(jonas)
Attachment #298631 -
Flags: superreview?(bzbarsky)
Attachment #298631 -
Flags: review?(jonas)
Comment on attachment 298808 [details] [diff] [review] Updated fix. >@@ -289,27 +281,19 @@ nsXMLDocument::OnChannelRedirect(nsIChannel *aOldChannel, > > nsIScriptSecurityManager *secMan = nsContentUtils::GetSecurityManager(); > >- if (mScriptContext && !mCrossSiteAccessEnabled) { >- nsCOMPtr<nsIJSContextStack> stack(do_GetService("@mozilla.org/js/xpc/ContextStack;1", & rv)); >- if (NS_FAILED(rv)) >- return rv; >- >- JSContext *cx = (JSContext *)mScriptContext->GetNativeContext(); >- if (!cx) >- return NS_ERROR_UNEXPECTED; >+ nsCOMPtr<nsIURI> codebase; >+ rv = NodePrincipal()->GetURI(getter_AddRefs(codebase)); >+ NS_ENSURE_SUCCESS(rv, rv); > >- stack->Push(cx); >+ nsCOMPtr<nsIURI> channelURI; >+ rv = aNewChannel->GetURI(getter_AddRefs(channelURI)); >+ NS_ENSURE_SUCCESS(rv, rv); I'd suggest you just use aOldChannel->GetURI and aNewChannel->GetURI. That's what we do elsewhere and should make us good even in the face of madly redirecting chrome channels :) >@@ -438,48 +405,20 @@ nsXMLDocument::Load(const nsAString& aUrl, PRBool *aReturn) > nsCOMPtr<nsIEventListenerManager> elm(mListenerManager); > mListenerManager = nsnull; > >- ResetToURI(uri, nsnull, principal); >- >- mListenerManager = elm; >- >- // Get security manager, check to see if we're allowed to load this URI >- nsCOMPtr<nsIScriptSecurityManager> secMan = >- do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv); >- if (NS_FAILED(rv)) { >- return rv; >- } >+ // When we are called from JS we can find the load group for the page, >+ // and add ourselves to it. This way any pending requests >+ // will be automatically aborted if the user leaves the page. > >- rv = secMan->CheckConnect(nsnull, uri, "XMLDocument", "load"); >- if (NS_FAILED(rv)) { >- // We need to return success here so that JS will get a proper >- // exception thrown later. Native calls should always result in >- // CheckConnect() succeeding, but in case JS calls C++ which calls >- // this code the exception might be lost. >- return NS_OK; Why remove the CheckConnect call? Basically it's there so that you can set policies to disable XMLDocument.load. I'm not a fan of that architecture, but we might as well keep it until moz2.
Attachment #298808 -
Flags: review?(jonas) → review+
Reporter | ||
Comment 33•16 years ago
|
||
> It can't be, can it? chrome:// URIs can't redirect can they? And document with
> system principals now can't load anything that isn't also chrome.
With the modified patch, yeah. It'll probably block remote chrome loads that return 3xx in a chrome doc... but that's just fine, imo.
Assignee | ||
Comment 34•16 years ago
|
||
Attachment #298808 -
Attachment is obsolete: true
Attachment #298836 -
Flags: superreview?(bzbarsky)
Attachment #298836 -
Flags: review+
Attachment #298808 -
Flags: superreview?(bzbarsky)
Reporter | ||
Comment 35•16 years ago
|
||
Comment on attachment 298836 [details] [diff] [review] Updated updated fix. Sure.
Attachment #298836 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 36•16 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 37•16 years ago
|
||
So this broke loading file: urls in extensions. If that is intended, could it be documented somewhere? And with some workarounds. Though, it is a bit strange to get security exception when executing chrome code. Error: uncaught exception: [Exception... "Security error" code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)" location: "javascript:%20var%20x%20=%20document.implementation.createDocument("text/xml","",null);%20x.async%20=%20false;%20x.load("file:///home/smaug/mozilla/tests/simple.xml"); Line: 1"]
Comment 38•16 years ago
|
||
> // We're called from chrome, check to make sure the URI we're > // about to load is also chrome. > PRBool isChrome = PR_FALSE; > if (NS_FAILED(uri->SchemeIs("chrome", &isChrome)) || !isChrome) { > return NS_ERROR_DOM_SECURITY_ERR; > } Huh? My reading of that comment implies to me that a chrome caller of document.load cannot load xml documents unless they are also chrome urls. That's not acceptable behaviour and makes this api practically useless from chrome. It also breaks xul templates with xml sources which use this api. I filed bug 415772 on this latter issue.
(In reply to comment #38) > Huh? My reading of that comment implies to me that a chrome caller of > document.load cannot load xml documents unless they are also chrome urls. > That's not acceptable behaviour and makes this api practically useless from > chrome. Basically that is the idea. You shouldn't be using this API. Use XMLHttpRequest instead.
Reporter | ||
Updated•16 years ago
|
Comment 40•16 years ago
|
||
> Basically that is the idea. You shouldn't be using this API. Use XMLHttpRequest > instead. OK, so is there a bug filed on calls to XMLHttpRequest crashing at http://lxr.mozilla.org/mozilla/source/content/base/src/nsCrossSiteListenerProxy.cpp#65 because there's no principal for native callers?
Comment 41•16 years ago
|
||
Bug 392322
Comment 42•16 years ago
|
||
Can someone summarize for me what needs to be documented here? There aren't any API changes (those are easy to spot, usually) that I can see, so I'm assuming it's a more esoteric behavior change that needs to be written up, and I don't want to screw it up -- so I'll ask for some help. :)
Updated•16 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•