Closed
Bug 416534
Opened 18 years ago
Closed 17 years ago
Clean up cross-site XHR security checks
Categories
(Core :: XML, defect, P2)
Core
XML
Tracking
()
RESOLVED
FIXED
People
(Reporter: sicking, Assigned: sicking)
References
Details
Attachments
(2 files, 2 obsolete files)
74.62 KB,
patch
|
peterv
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
2.33 KB,
patch
|
Details | Diff | 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)
Assignee | ||
Comment 1•18 years ago
|
||
Fixes compliance issues with the spec, so we should definitely do this before final release.
Comment 2•18 years ago
|
||
You're replacing some calls to CheckSameOriginPrincipal with CheckMayLoad, but that doesn't look like it's equivalent?
Assignee | ||
Comment 3•18 years ago
|
||
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.
Comment 4•18 years ago
|
||
>+nsPrincipal::CheckMayLoad(nsIURI* aURI, PRBool aReport)
>+{
>+ nsresult rv = nsScriptSecurityManager::SecurityCompareURIs(mCodebase, aURI);
>+ if (NS_FAILED(rv) && aReport) {
SecurityCompareURIs returns PRBool, makes the mochitests fail.
Comment 5•18 years ago
|
||
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.
Assignee | ||
Comment 6•18 years ago
|
||
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.
Comment 7•18 years ago
|
||
Not sure about this one, but
> nsCrossSiteListenerProxy::GetInterface(const nsIID & aIID, void **aResult) {
> ... return mOuterNotificationCallbacks->GetInterface(aIID, aResult);
mOuterNotificationCallbacks can be NULL
Assignee | ||
Comment 8•18 years ago
|
||
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.
Assignee | ||
Comment 9•18 years ago
|
||
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.
Comment 10•18 years ago
|
||
(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 11•18 years ago
|
||
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-
Assignee | ||
Comment 12•18 years ago
|
||
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.
Assignee | ||
Comment 13•17 years ago
|
||
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)
Comment 14•17 years ago
|
||
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?
Assignee | ||
Comment 15•17 years ago
|
||
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 16•17 years ago
|
||
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.
Assignee | ||
Comment 17•17 years ago
|
||
> >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?
Assignee | ||
Comment 18•17 years ago
|
||
> Don't remove nsIInterfaceRequestor.
Hmm.. which version did you review, that isn't removed in the latest attachment.
Comment 19•17 years ago
|
||
(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.
Assignee | ||
Comment 20•17 years ago
|
||
(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 21•17 years ago
|
||
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+
Assignee | ||
Comment 22•17 years ago
|
||
> >+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.
Comment 24•17 years ago
|
||
I backed this out on suspicion of causing the mochitest failure.
Jonas relanded with the mochitest fix that he had in a different tree.
Comment 26•17 years ago
|
||
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
Comment 27•17 years ago
|
||
Ah yes, there's a bunch of these
rv = doc->NodePrincipal()->CheckMayLoad(uri);
if (NS_FAILED(rv))
in NPOTB code.
Comment 28•17 years ago
|
||
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.
Comment 29•17 years ago
|
||
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)
Assignee | ||
Comment 30•17 years ago
|
||
This was checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
![]() |
||
Comment 31•17 years ago
|
||
So how come this is noscript? Is there a good reason?
Assignee | ||
Comment 32•17 years ago
|
||
Basically I just did what subsumes does as they are very similar.
![]() |
||
Comment 33•17 years ago
|
||
Yeah, subsumes shouldn't be noscript either, imo. The only noscript things here should be the ones that mutate the principal.
You need to log in
before you can comment on or make changes to this bug.
Description
•