1085450 - Rename NS_NewChannelInternal with NS_NewChannelWithTriggeringPrincipal and NS_NewChannelWithLoadInfo

Status: RESOLVED WONTFIX

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

As Tanvi pointed out [1], we can rename/should NS_NewChannelInternal and either call NS_NewChannelWithTriggeringPrincipal or NS_NewChannelWithLoadInfo.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1083422#c7

Comment 1

[Tanvi Vyas[:tanvi]]

Add comments to explain when to use which method:
NS_NewChannel(that takes a node)
NS_NewChannel(that takes a principal)
NS_NewChannelWithTriggeringPrincipal(node and triggeringPrincipal)
NS_NewChannelWithLoadInfo(takes the loadinfo object itself)

Do we every call NS_NewChannel(principal) and pass something other than the systemPrincipal?  Generally speaking, this should only be used when passing system.  Add a comment explaining this.  And perhaps some examples of when that may not be the case.

Comment 2

[Tanvi Vyas[:tanvi]]

Attachment: remove_newchannelinternal-10-24-14.patch

Work in progress patch and a few notes:

* This has to be applied on top of bug 1083422.

* From bug 1083422, we have NS_NewChannelWithTriggeringPrincipal() that takes a loadingNode and a triggeringPrincipal.  We also have NS_NewChannelWithTriggeringPrincipal() that takes a loadingPrincipal and a triggeringPrincipal that is used for e10s.  In this bug we replace NS_NewChannelInternal() calls with NS_NewChannelWithTriggeringPrincipal. In one such case, this leads to ambigous function call - https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#304
In order to fix this, I have renamed NS_NewChannelWithTriggeringPrincipal() that takes a loadingPrincipal and a triggeringPrincipal to NS_NewChannelWithTriggeringAndLoadingPrincipal() (if you have an idea for a better name, I am open to changing this).

* Still need to add comments as described in comment 1.

* Need to update names of other Internal functions to account for the triggeringPrincipal (ex: NS_OpenURIInternal, NS_NewStreamLoaderInternal, etc).

* This patch compiles but crashes the browser because of a null check for the loadingNode.  I change NS_OpenURIInternal to call NS_NewChannelWithTriggeringPrincipal() instead of NS_NewChannelInternal() here https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#331.  NS_NewChannelWithTriggeringPrincipal() does a null check for loadingNode while NS_NewChannelInternal() does not.  On browser startup, this code is run and I guess it doesn't pass a node:
http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#1472
I need to look into this more.

Comment 3

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #1) 
> Do we every call NS_NewChannel(principal) and pass something other than the
> systemPrincipal?  Generally speaking, this should only be used when passing
> system.  Add a comment explaining this.  And perhaps some examples of when
> that may not be the case.

Looks like there are quite a few places where we call NS_NewChannel(principal) with a principal that is not system (and no node):
http://mxr.mozilla.org/mozilla-central/source/dom/base/EventSource.cpp#757
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsSyncLoadService.cpp#313
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#1757
http://mxr.mozilla.org/mozilla-central/source/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp#463
http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp#2375
http://mxr.mozilla.org/mozilla-central/source/dom/xul/XULDocument.cpp#2700
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUserFontSet.cpp#1185

Probably for e10s reasons:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/FTPChannelParent.cpp#147
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#239
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp#90

And with a nullprincipal:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsCSPContext.cpp#679
http://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#126
http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/nsExpatDriver.cpp#814

Comment 4

[Tanvi Vyas[:tanvi]]

NS_OpenURIPrincipal is pretty much always called with systemPrincipal.  Except for this case where it is passed in from parameters: 
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1544
But when you look at the callstack, you see that the two places that end up going through this code path also pass system.


NS_NewInputStreamChannel - For the loadingPrincipal, sometimes we use nullprincipal, sometimes system, sometimes something else.  Doesn't seem to be a general rule of thumb here.

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: remove_newchannelinternal-10-30-14.patch

Asking feedback from Christoph.  After that I'll split it up and flag individual reviewers.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8514555 [details] [diff] [review]
remove_newchannelinternal-10-30-14.patch

Review of attachment 8514555 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - I don't think you need to split the patches, it's only renaming of functions, you are not changing any functionality here. I guess Jonas can review that patch once Bug 1083422 has landed.

::: modules/libjar/nsJARChannel.cpp
@@ +880,3 @@
>              }
>              else {
> +              rv = NS_OpenURI(mDownloader,

Good catch, no need to call NS_OpenURIInternal here.

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: remove_newchannelinternal-10-30-14B.patch

Updated a comment.  Carrying over feedback+ from Christoph and r? to Jonas.  Jonas, there are a few comments in the patch with questions around how we should handle and NS_OpenURI call in layout/style/Loader.cpp.  I ran into a case where we don't have a requestingNode (call stack: http://pastebin.mozilla.org/7012233).

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Looks like there are quite a few places where we call
> NS_NewChannel(principal) with a principal that is not system (and no node):
> http://mxr.mozilla.org/mozilla-central/source/dom/base/EventSource.cpp#757

I can't think of good reasons why we'd call this without a document and with a principal other than a system principal. Can you provide a callstack?

> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsSyncLoadService.
> cpp#313

I think all callers of this code has a document available that it could pass instead of the loader principal.

> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.
> cpp#1757

Same as for EventSource.

> http://mxr.mozilla.org/mozilla-central/source/dom/xslt/xslt/
> txMozillaStylesheetCompiler.cpp#463

This is bug 1067517.

> http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.
> cpp#2375

You can probably simply use 'this' as the node.

> http://mxr.mozilla.org/mozilla-central/source/dom/xul/XULDocument.cpp#2700

You can probably simply use 'this' as the node.

> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUserFontSet.
> cpp#1185

I have to defer to John Daggett on this one. I can't think of any cases when we should be loading a font using a non-system principal, but not in the context of a document.

> And with a nullprincipal:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsCSPContext.cpp#679

When does this code get triggered?

> http://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.
> cpp#126

This shouldn't use a null principal. But it does make sense that we sometimes just have a worker, but no document, here.

> http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/
> nsExpatDriver.cpp#814

We should probably use a system principal here. But someone that knows this code better would need to confirm.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8514609 [details] [diff] [review]
remove_newchannelinternal-10-30-14B.patch

Review of attachment 8514609 [details] [diff] [review]:
-----------------------------------------------------------------

Do we really need all of these different function signatures in nsNetUtil.h? It seems like an aweful lot.

Couldn't we create a generic low-level one which returns an nsIChannel (NS_NewChannelInternal) and one which returns an nsInputStream (NS_OpenURIInternal), and then only create the neccessary other ones that we really want people calling? It seems like there's so many different ways of creating channels and streams that people will never find the right one to use and will instead end up calling the wrong one.

We should make the various channel implementations call NS_newChannelInternal and NS_OpenURIInternal.

::: layout/style/Loader.cpp
@@ +1470,5 @@
> +      // Note that we are calling NS_OpenURIWithTriggeringPrincipal() with both
> +      // a node and a principal.  This is because of a case where the node is
> +      // the document being styled and the triggering principal is the
> +      // stylesheet (perhaps from a different origin)  that is applying the
> +      // styles.

I don't think you need this comment. It's always the case when calling *WithTriggeringPrincipal.

@@ +1488,5 @@
> +      // Should the requestingPrincipal be system in that case?  That may not always be the case.
> +      // sicking - need your advice here.
> +      // This is also the first time we are calling an NS_*WithTriggeringAndLoadingPrincipal outside of NetUtil.h
> +      // That family of methods shoudl really be private to NetUti.h
> +      rv = NS_OpenURIWithTriggeringAndLoadingPrincipal(getter_AddRefs(stream),

You need to ask someone that knows this code better. Like dbaron or bz. It might well be that if mRequestingNode is null that we should just grab the right document instead.

There is nothing special about the system principal. I.e. whoever triggers a load by linking to a resource should be used as the triggering principal. No exceptions if that happens to be the system principal. And whatever document we're loading into should be used as the loading node/principal. No exceptions if that happens to be a system principal.

Just because the information isn't available immediately in the relevant function doesn't mean that we can get to it. It just means that we might need to modify callsites.

How to get the triggering principal or the loading node/principal is best asked the people that know the relevant code.

I.e. so rather than asking "we don't have a node/principal available, which one should we guess at", we should ask "we don't have a node/principal available, how can we make it available".

::: netwerk/base/public/nsNetUtil.h
@@ +406,3 @@
>  {
> +  //TANVI - this was originally a assertion on requestingPrincipal.  now that we have triggeringPrincipal, shoudl we chagne this to a check on requestingnode or triggeringPrincipal?
> +  NS_ASSERTION(aRequestingNode, "Can not create channel without a requesting Node!");

No. We we should still always have a requesting principal. No reason to relax requirements about knowing where we're loading something into.

@@ +436,5 @@
> +// Read() from it and blocking the UI thread is a bad idea. If you don't want
> +// to implement a full blown asynchronous consumer (via nsIStreamListener) look
> +// at nsIStreamLoader instead.
> +inline nsresult /* Private for nsNetUtil.h */
> +NS_OpenURIWithTriggeringAndLoadingPrincipal(nsIInputStream**       outStream,

Make the "Private for nsNetUtil.h" comment bigger and more clear, and move it to above the other comment. And mention which function people should call instead.

@@ +469,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  *outStream = stream;
> +  if (outChannel) {
> +    *outChannel = nullptr;
> +     channel.swap(*outChannel);

Nit: Replace these *two* lines with "channel.forget(outChannel);"

Comment 10

[Tanvi Vyas[:tanvi]]

NS_NewChannelWithTriggeringPrincipal was added in bug 1083422 and we decided to keep NS_NewChannelInternal.  Marking this bug as closed won't fix.