Closed Bug 416534 Opened 12 years ago Closed 12 years ago

Clean up cross-site XHR security checks

Categories

(Core :: XML, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Patch to fix (obsolete) — Splinter Review
It'd be nice to have the security checks for cross site XHR more centralized. Including doing the necessary checks on redirects in the nsCrossSiteListenerProxy.

Attaching a patch that does the following:

1. Make sure to call CheckLoadURIWithPrincipal on all redirects
2. Make sure to do appropriate access-control checks even when redirected back
   to the original site. This is per spec.
3. Properly set Referer-Root to "null" when the referer doesn't have a host,
   such as for data uris. This is per spec.
3. Move more of the access-control logic into nsCrossSiteListenerProxy
4. Add the API nsIPrincipal::CheckMayLoad(uri) to avoid constantly having to
   deal with the system principal returning null for GetURI
5. Replace security checks in XSLT code with nsCrossSiteListenerProxy which
   means that they'll be done more properly using principals rather than uris.
Attachment #302291 - Flags: superreview?(peterv)
Attachment #302291 - Flags: review?(peterv)
Fixes compliance issues with the spec, so we should definitely do this before final release.
Blocks: xxx
Flags: blocking1.9+
Priority: -- → P2
You're replacing some calls to CheckSameOriginPrincipal with CheckMayLoad, but that doesn't look like it's equivalent?
It is intentionally different in that it allows chrome to load anything. It also ignores setting of document.domain. The new API is only meant to be used before you actually load something from the network, in which case both above effects are desired.

I should definitely comment on that in the .idl.

Peterv pointed out over irc that that might not be what we want for canvas. I'm not actually 100% sure what canvas is up to there, but I'll check with vlad. In any case I think CheckMayLoad is more restrictive in the general case since it ignores document.domain.
>+nsPrincipal::CheckMayLoad(nsIURI* aURI, PRBool aReport)
>+{
>+  nsresult rv = nsScriptSecurityManager::SecurityCompareURIs(mCodebase, aURI);
>+  if (NS_FAILED(rv) && aReport) {

SecurityCompareURIs returns PRBool, makes the mochitests fail.
Attached patch Fix tests (obsolete) — Splinter Review
Don't know if this is the right way to do it, but this tries to fix some minor logical issues in the previous patch, removes the requirement for the "Allow" response header and fixes the tests to match the spec.
Hmm.. How can you do POST tests without having OPTIONS support in the server? Shouldn't those always fail? In any case, I think we should fix the Allow header and test in a separate bug, this patch does enough as it is. Maybe bug 416958.

What were the other logical errors in the patch? I think I'd rather let peter do a full review before touching the patch given how big it is.
Not sure about this one, but

> nsCrossSiteListenerProxy::GetInterface(const nsIID & aIID, void **aResult) {
> ... return mOuterNotificationCallbacks->GetInterface(aIID, aResult);

mOuterNotificationCallbacks can be NULL





I talked with vlad about the canvas thing and it's fine to keep as is. Basically this is effectively loading the given uri, though it's doing so by accessing a DOM <img> with that uri inside it.
In addition to what Surya found in comment 4 and comment 7 I found 3 additional things:

The code to set Referer-Root was declaring |root| inside the if-statement, making Referer-Root always be set to empty.

The same code also used the loaded uri rather than the requesting uri to get the port.

These two fixes make the patch pass all of mochitest.

I also had redeclared rv in one of the functions giving compile errors on windows.

Let me know if you want a new patch.
(In reply to comment #6)
> How can you do POST tests without having OPTIONS support in the server?

I don't think this should be a problem; SJS support should make it possible to do such testing, I think:

http://developer.mozilla.org/en/docs/Mochitest#How_do_I_write_tests_that_check_header_values.2C_method_types.2C_etc._of_HTTP_requests.3F

The only cases I can think of where this wouldn't work is if the Request-URI in the first line of the request was "*" (which seems unlikely for this particular case) or if the particular hostname in the Request-URI must be checked to be a specific value (the server currently just strips it and uses the path portion because it doesn't have a way to know its identity).  The former seems unlikely, and the latter, while good to test, shouldn't be critical for checking functionality.

Still, if you need something, file a bug.
Comment on attachment 302291 [details] [diff] [review]
Patch to fix

>Index: caps/idl/nsIPrincipal.idl
>===================================================================

> 
>+    [noscript] void checkMayLoad(in nsIUri uri, in boolean report);

Document this.

>Index: caps/src/nsPrincipal.cpp
>===================================================================

>+nsPrincipal::CheckMayLoad(nsIURI* aURI, PRBool aReport)
>+{
>+  nsresult rv = nsScriptSecurityManager::SecurityCompareURIs(mCodebase, aURI);

SecurityCompareURIs returns PRBool.

>Index: caps/src/nsScriptSecurityManager.cpp
>===================================================================

>@@ -3254,21 +3256,20 @@ nsScriptSecurityManager::Observe(nsISupp

>+      ,mXPCDefaultGrantAll(PR_FALSE)

Add space after ,

>Index: content/base/public/nsISyncLoadDOMService.idl
>===================================================================

>+    nsIDOMDocument loadDocument(in nsIChannel aChannel,
>+                                in nsIPrincipal aLoaderURI);

s/aLoaderURI/aLoaderPrincipal/

>Index: content/base/src/nsContentUtils.cpp
>===================================================================

>   nsCOMPtr<nsIURI> loadingURI;
>   rv = aLoadingPrincipal->GetURI(getter_AddRefs(loadingURI));
>   NS_ENSURE_SUCCESS(rv, rv);
>-  return sSecurityManager->CheckSameOriginURI(loadingURI, aURIToLoad, PR_TRUE);
>+
>+  return aLoadingPrincipal->CheckMayLoad(aURIToLoad, PR_TRUE);

Remove loadingURI.

>Index: content/xslt/src/xslt/txMozillaStylesheetCompiler.cpp
>===================================================================

>@@ -87,28 +89,25 @@ getSpec(nsIChannel* aChannel, nsAString&

>-                         public nsIInterfaceRequestor

Why do you want to remove nsIInterfaceRequestor? Don't you need it and the code in GetInterface for nsIAuthPrompt?

>     NS_DECL_NSIINTERFACEREQUESTOR

If you really want to remove nsIInterfaceRequestor you should remove this and the GetInterface implementation too.

>@@ -543,41 +524,51 @@ txCompileObserver::startLoad(nsIURI* aUr

>-    channel->SetNotificationCallbacks(sink);

Again, won't this break the code that needs txStylesheetSink to return an nsIAuthPrompt?

I think at this point it'd be best to get a new patch so I can take another look.
Attachment #302291 - Flags: superreview?(peterv)
Attachment #302291 - Flags: superreview-
Attachment #302291 - Flags: review?(peterv)
Attachment #302291 - Flags: review-
Looking at the nsIAuthPrompt thing I actually think we *don't* want to provide an auth-prompt. The one living on the loadgroup seems to be more correct see

http://lxr.mozilla.org/mozilla/source/docshell/base/nsDocShell.cpp#9200

But it does seem like a good idea to leave it in for now and remove it in a separate patch.
Attached patch Patch v2Splinter Review
Fix those issues, update iids and sync to tip.
Assignee: nobody → jonas
Attachment #302291 - Attachment is obsolete: true
Attachment #302757 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #304390 - Flags: superreview?(peterv)
Attachment #304390 - Flags: review?(peterv)
Grmbl, I seem to have deleted my review comments. Just a quick note since the string freeze is tonight: don't you need to add an error message to caps.properties for CheckMayLoadError?
Oh, good point. Though looking at the messages that are there we could just as well use CheckSameOriginError. That currently says "Security Error: Content at %S may not load data from %S." which is exactly what i'd want for these new sites. Also, the function checks is loads are allowed by the same-origin policy.
Comment on attachment 304390 [details] [diff] [review]
Patch v2

>Index: caps/idl/nsIPrincipal.idl
>===================================================================

>+     * Checks weather this principal is allowed to load the network resource

whether

>+     * If the load is allowed this function does nothing. If the loed is not

load

>Index: caps/include/nsScriptSecurityManager.h
>===================================================================

>+    static PRBool SecurityCompareFileURIs(nsIURI* aSourceURI, nsIURI* aTargetURI);

Long line.

>Index: content/base/public/nsISyncLoadDOMService.idl
>===================================================================

>+    nsIDOMDocument loadDocument(in nsIChannel aChannel,
>+                                in nsIPrincipal aLoaderPrincipal);
> 
>     nsIDOMDocument loadDocumentAsXML(in nsIChannel aChannel,
>-                                     in nsIURI aLoaderURI);
>+                                     in nsIPrincipal aLoaderPrincipal);

>     nsIDOMDocument loadLocalDocument(in nsIChannel aChannel,
>-                                     in nsIURI aLoaderURI);
>+                                     in nsIPrincipal aLoaderPrincipal);

Or we could drop these methods since no one is using them anymore?

>Index: content/base/src/nsCrossSiteListenerProxy.cpp
>===================================================================


>+nsCrossSiteListenerProxy::GetInterface(const nsIID & aIID, void **aResult)
>+{
>+  if (aIID.Equals(NS_GET_IID(nsIChannelEventSink))) {
>+    *aResult = static_cast<nsIChannelEventSink*>(this);
>+    NS_ADDREF_THIS();

Just NS_ADDREF() around the assignment.

>Index: content/base/src/nsSyncLoadService.h
>===================================================================

>+     *                         security checka should be done.

check
Unless this is some sort of slang I don't know about :-P.

>Index: content/xml/document/src/nsXMLDocument.cpp
>===================================================================

>+  // using a document that content has a reference to and turn that into and

into a

>Index: content/xslt/src/xslt/txMozillaStylesheetCompiler.cpp
>===================================================================

>+NS_IMPL_ISUPPORTS5(txStylesheetSink,
>                    nsIXMLContentSink,
>                    nsIContentSink,
>                    nsIExpatSink,
>                    nsIStreamListener,
>-                   nsIRequestObserver,
>-                   nsIChannelEventSink,
>-                   nsIInterfaceRequestor)
>+                   nsIRequestObserver)

Don't remove nsIInterfaceRequestor.

nsXMLHttpRequest looks like it avoids using the nsCrossSiteListenerProxy for chrome loads or UniversalBrowserRead (see XML_HTTP_REQUEST_XSITEENABLED). Do we want this for syncloader and XSLT too?

Don't think I have much more, but want to have a final look tomorrow.
> >Index: content/base/public/nsISyncLoadDOMService.idl
> >===================================================================
> 
> >+    nsIDOMDocument loadDocument(in nsIChannel aChannel,
> >+                                in nsIPrincipal aLoaderPrincipal);
> > 
> >     nsIDOMDocument loadDocumentAsXML(in nsIChannel aChannel,
> >-                                     in nsIURI aLoaderURI);
> >+                                     in nsIPrincipal aLoaderPrincipal);
> 
> >     nsIDOMDocument loadLocalDocument(in nsIChannel aChannel,
> >-                                     in nsIURI aLoaderURI);
> >+                                     in nsIPrincipal aLoaderPrincipal);
> 
> Or we could drop these methods since no one is using them anymore?

I'd like to drop the whole interface, but I'm worried extensions are using it. I'd be fine either way.

> >Index: content/base/src/nsSyncLoadService.h
> >===================================================================
> 
> >+     *                         security checka should be done.
> 
> check
> Unless this is some sort of slang I don't know about :-P.

"checka dat out yo!" :)


> nsXMLHttpRequest looks like it avoids using the nsCrossSiteListenerProxy for
> chrome loads or UniversalBrowserRead (see XML_HTTP_REQUEST_XSITEENABLED). Do we
> want this for syncloader and XSLT too?

I sort of loath UniversalBrowserRead so I'd rather not waste effort on it, especially given that no-one's requested it. For chrome loads it shouldn't really make a difference I think. The mHasBeenCrossSite will never get set since CheckMayLoad will always succeed. Maybe we should always use it for XHR too just to be consistent?
> Don't remove nsIInterfaceRequestor.

Hmm.. which version did you review, that isn't removed in the latest attachment.
(In reply to comment #17)
> > Or we could drop these methods since no one is using them anymore?
> 
> I'd like to drop the whole interface, but I'm worried extensions are using it.
> I'd be fine either way.

Ok, let's keep it then.

> The mHasBeenCrossSite will never get set
> since CheckMayLoad will always succeed. Maybe we should always use it for XHR
> too just to be consistent?

Unless it affects performance much, I'd prefer consistency, yes.

(In reply to comment #18)
> Hmm.. which version did you review, that isn't removed in the latest
> attachment.

Attachment 304390 [details] [diff] definitely removes nsIInterfaceRequestor from the QI implementation for txStylesheetSink.
(In reply to comment #19)
> > The mHasBeenCrossSite will never get set
> > since CheckMayLoad will always succeed. Maybe we should always use it for
> > XHR too just to be consistent?
> 
> Unless it affects performance much, I'd prefer consistency, yes.

So we still don't want to install it when UniversalRead is enabled on the principal, unless we want nsPrincipal::CheckMayLoad check for that, but I don't think we do? I guess we could explicitly insert the proxy still just when we have a system principal, but that seems a bit odd...

> Attachment 304390 [details] [diff] definitely removes nsIInterfaceRequestor from the QI
> implementation for txStylesheetSink.

Doh! Put the implementation back, but forgot just the QIimpl.
Comment on attachment 304390 [details] [diff] [review]
Patch v2

r/sr=peterv with the issues that I mentioned fixed.
Attachment #304390 - Flags: superreview?(peterv)
Attachment #304390 - Flags: superreview+
Attachment #304390 - Flags: review?(peterv)
Attachment #304390 - Flags: review+
> >+nsCrossSiteListenerProxy::GetInterface(const nsIID & aIID, void **aResult)
> >+{
> >+  if (aIID.Equals(NS_GET_IID(nsIChannelEventSink))) {
> >+    *aResult = static_cast<nsIChannelEventSink*>(this);
> >+    NS_ADDREF_THIS();
> 
> Just NS_ADDREF() around the assignment.

Doesn't work since the assignment returns a void*.

Also didn't insert the crosssiteproxy listener in xhr unless needed in order to deal with UniversalBrowserRead
Did this cause the following mochitest failure?

*** 14351 INFO Running /tests/dom/tests/mochitest/bugs/test_bug317448.html...
*** 14352 INFO PASS | Must be interface requestor
*** 14353 ERROR FAIL | Error thrown during test: uncaught exception: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: http://localhost:8888/tests/dom/tests/mochitest/bugs/test_bug317448.html :: <TOP_LEVEL> :: line 32"  data: no] | got 0, expected 1 | /tests/dom/tests/mochitest/bugs/test_bug317448.html

That's one of the two mochitest failures in:
MacOSX Darwin 8.8.4 qm-xserve01 dep unit test on 2008/02/26 18:21:29  

I caused and fixed the other one.
I backed this out on suspicion of causing the mochitest failure.
Jonas relanded with the mochitest fix that he had in a different tree.
I get build error:

nsXFormsUtils.cpp: In static member function ‘static PRBool nsXFormsUtils::CheckSameOrigin(nsIDocument*, nsIURI*, nsXFormsUtils::ConnectionType)’:
nsXFormsUtils.cpp:1587: error: no matching function for call to ‘nsIPrincipal::CheckMayLoad(nsIURI*&)’
../../dist/include/caps/nsIPrincipal.h:206: note: candidates are: virtual nsresult nsIPrincipal::CheckMayLoad(nsIURI*, PRBool)
nsXFormsUtils.cpp:1599: error: ‘basePrincipal’ was not declared in this scope

Ah yes, there's a bunch of these

      rv = doc->NodePrincipal()->CheckMayLoad(uri);
      if (NS_FAILED(rv))

in NPOTB code.
Attached patch Bustage fixSplinter Review
I'll check in this bustage fix. The schema loader doesn't really need CheckMayLoad I think, it uses XMLHttpRequest, but I'm not going to spend time on that.
building breaks with 
nsXFormsUtils.cpp:1599: error: ‘basePrincipal’ was not declared in this scope
make[5]: *** [nsXFormsUtils.o] Fehler 1
make[5]: *** Warte auf noch nicht beendete Prozesse...
In file included from nsXFormsMDGEngine.h:52,
                 from nsXFormsModelElement.h:53,
                 from nsXFormsRepeatElement.cpp:61:
nsXFormsNodeState.h:92: warning: negative integer implicitly converted to unsigned type

checkout start: Mi 27. Feb 12:02:23 CET 2008 (3:02:23 PST)
This was checked in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
So how come this is noscript?  Is there a good reason?
Basically I just did what subsumes does as they are very similar.
Yeah, subsumes shouldn't be noscript either, imo.  The only noscript things here should be the ones that mutate the principal.
Depends on: 428847
You need to log in before you can comment on or make changes to this bug.