Closed Bug 397791 Opened 17 years ago Closed 16 years ago

XOW used for same-origin "document" access

Categories

(Core :: XPConnect, defect, P1)

x86
Linux
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: jst)

References

Details

(Keywords: dev-doc-complete, perf)

Attachments

(1 file, 2 obsolete files)

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?
Severity: normal → major
Keywords: perf
Blocks: 375470
(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.
(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?
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?
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.
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....
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 think we should do comment 7 and remove the XOW-ness here, yeah.  That seems like the simplest way forward on this bug.
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]
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.
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.
The time is almost entirely spent getting the principals.  As you observed, the common case is a pointer compare once that's done.
Re-noming for perf reasons
Flags: blocking1.9- → blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: P3 → P2
Boris, what do the profiles look like now after jst's GetSecurityManager fix?
I won't be able to answer that very well until I'm back in Chicago in January....
(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?
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?
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.
> 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**()
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Moving to P1 - if we are going to get this in - must be before B3.
Priority: P2 → P1
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)
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?
> 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?
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
Keywords: dev-doc-needed
The only exception to that is content JS with elevated privileges (i.e. UniversalXPConnect or what not).
(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.
Attached patch Updated fix. (obsolete) — Splinter Review
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+
> 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.
Attachment #298808 - Attachment is obsolete: true
Attachment #298836 - Flags: superreview?(bzbarsky)
Attachment #298836 - Flags: review+
Attachment #298808 - Flags: superreview?(bzbarsky)
Comment on attachment 298836 [details] [diff] [review]
Updated updated fix.

Sure.
Attachment #298836 - Flags: superreview?(bzbarsky) → superreview+
Fix checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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"]
Blocks: 415772
> // 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.
No longer blocks: 415772
Depends on: 415772
> 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?
Depends on: 417378
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. :)
Depends on: 428653
Depends on: 505209
Depends on: 505212
You need to log in before you can comment on or make changes to this bug.