Closed Bug 1063197 Opened 5 years ago Closed 5 years ago

Storing LoadInfo on all channels created through NS_NewInputStreamChannel

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

Attachments

(12 files, 1 obsolete file)

9.73 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
7.17 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
3.58 KB, patch
jst
: review+
Details | Diff | Splinter Review
4.02 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.98 KB, patch
karlt
: review+
Details | Diff | Splinter Review
5.19 KB, patch
seth
: review+
Details | Diff | Splinter Review
2.25 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.02 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.83 KB, patch
mak
: review+
Details | Diff | Splinter Review
4.41 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.00 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.70 KB, patch
Details | Diff | Splinter Review
Similar to bug 1038756, we should append the loadInfo on all channels created through NS_NewInputStreamChannel.
Blocks: 1006881
FWIW, I'm not entirely sure that this is needed. Once some code creates an input-stream-channel it already has access to the data in the form of an inputstream. I.e. that we already should have done any needed security checks by the time that we get to code using input-stream-channel.

I thought that input-stream-channel was basically something we used internally in order to simplify some multithreading work (by letting the input-stream-channel take care of reading from the stream in a non-blocking way).

But maybe I'm wrong about that?
(In reply to Jonas Sicking (:sicking) from comment #1)
> FWIW, I'm not entirely sure that this is needed. Once some code creates an
> input-stream-channel it already has access to the data in the form of an
> inputstream. I.e. that we already should have done any needed security
> checks by the time that we get to code using input-stream-channel.

Well, the reason I would like to add the loadInfo here as well is that *all* channels should have a loadInfo attached sooner or later. We could easily accomplish this goal by adding the loadinfo in nsnetUtil.h without having to change any *.idl files, keeping addon compatability.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Depends on: 1038756
Summary for all reviewers:
Channels know very little about who initiated the request for the data they are fetching.  But there are use cases where we need some loading information during the lifetime of the channel.  Hence, bug 965413 added a LoadInfo object that hangs off channels. This bug is to populate that LoadInfo object at channel creation time (via NS_NewInputStreamChannel).  In this code, we pass a loadingPrincipal and/or a loadingNode to NS_NewInputStreamChannel which then creates the loadinfo object using the passed arguments.

Please take a careful look and see if we have identified the correct loading node and/or loading principal.  We are not looking for the node and principal corresponding to the data that the channel is fetching, but the node and principal of the entity that is loading that data.  These values are crucial to security checking code that is currently being reimplemented.  If we pass the wrong values, we may end up allowing resource loads that should be blocked by the browser.  If we use systemPrincipal, that will mean that all loads will be permitted. So please check that the load is coming from the system and that any needed security checks have already been done.
Attachment #8492800 - Flags: review?(mcmanus)
Attachment #8492801 - Flags: review?(mcmanus)
Attachment #8492803 - Flags: review?(bzbarsky)
Attachment #8492804 - Flags: review?(ehsan.akhgari)
Attached patch 6_callsites_in_gfx.patch (obsolete) — Splinter Review
Attachment #8492805 - Flags: review?(roc)
Attachment #8492808 - Flags: review?(benjamin)
Attachment #8492809 - Flags: review?(gpascutto)
Attachment #8492810 - Flags: review?(bzbarsky)
Comment on attachment 8492807 [details] [diff] [review]
8_callsites_in_parser.patch

mrbkap is on vacation, Boris can you review that patch as well?
Attachment #8492807 - Flags: review?(bzbarsky)
Attachment #8492801 - Flags: review?(mcmanus) → review+
Comment on attachment 8492800 [details] [diff] [review]
1_nsnetutil_changes.patch

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

::: netwerk/base/public/nsNetUtil.h
@@ +614,5 @@
>      return NS_GetDefaultPort(scheme.get());
>  }
>  
>  inline nsresult
> +NS_NewInputStreamChannelInternal(nsIChannel**        outChannel,

thanks for cleaning up this function along with appending your new logic
Attachment #8492800 - Flags: review?(mcmanus) → review+
Comment on attachment 8492809 [details] [diff] [review]
10_callsites_in_toolkit.patch

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

I'm not a peer for this module.
Attachment #8492809 - Flags: review?(gpascutto)
Comment on attachment 8492809 [details] [diff] [review]
10_callsites_in_toolkit.patch

(In reply to Gian-Carlo Pascutto [:gcp] from comment #17)
> Comment on attachment 8492809 [details] [diff] [review]
> I'm not a peer for this module.

Maybe Benjamin you can take a look at this one too. Once reviewed, I'll update the reviewer in the comment.
Attachment #8492809 - Flags: review?(benjamin)
Comment on attachment 8492803 [details] [diff] [review]
4_callsites_in_dom.patch

That JSON code is one sick puppy.  :(

>+++ b/dom/jsurl/nsJSProtocolHandler.cpp

Please document that the actual values passed in here don't matter, because they will get overridden by whatever SetLoadInfo sets on the JS channel?

And given that, can we do something cheaper than constructing a UUID here?  For example, can we pass nullptr for the principal argument to NS_NewInputStreamChannel or something?  If not, doing what you do here is probably OK for now, assuming the performance isn't too terrible; please do write a testcase that loads a bunch of javascript: things with and without this patch and see whether the difference is detectable.

In an ideal world we'd just have the right arguments to pass here, but those don't get passed to nsIProtocolHandler::NewChannel.  Perhaps they should be...

r=me with the above addressed.
Attachment #8492803 - Flags: review?(bzbarsky) → review+
Comment on attachment 8492807 [details] [diff] [review]
8_callsites_in_parser.patch

Probably OK, yeah.
Attachment #8492807 - Flags: review?(bzbarsky) → review+
Comment on attachment 8492810 [details] [diff] [review]
11_callsites_in_docshell.patch

r=me
Attachment #8492810 - Flags: review?(bzbarsky) → review+
Comment on attachment 8492804 [details] [diff] [review]
5_callsites_in_extensions.patch

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

I'm sorry, I don't know this code.  Forwarding to Karl.
Attachment #8492804 - Flags: review?(ehsan.akhgari) → review?(karlt)
Attachment #8492802 - Flags: review?(jst) → review+
Comment on attachment 8492804 [details] [diff] [review]
5_callsites_in_extensions.patch

I don't know about SEC_NORMAL and TYPE_OTHER.

I trust neither of these affect the behavior of URI_DANGEROUS_TO_LOAD?
Attachment #8492804 - Flags: review?(karlt) → review+
Comment on attachment 8492805 [details] [diff] [review]
6_callsites_in_gfx.patch

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

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +372,5 @@
> +    rv = NS_NewInputStreamChannel(getter_AddRefs(channel),
> +                                  uri,
> +                                  nullptr, //aStream
> +                                  principal,
> +                                  nsILoadInfo::SEC_NORMAL,

Why did this change from SEC_FORCE_INHERIT_PRINCIPAL?
Attachment #8492805 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)

> > +                                  nsILoadInfo::SEC_NORMAL,
> Why did this change from SEC_FORCE_INHERIT_PRINCIPAL?

Don't know what happened here, obviously it should remain SEC_FORCE_INHERIT_PRINCIPAL.
Attachment #8492805 - Attachment is obsolete: true
Attachment #8493877 - Flags: review?(roc)
Comment on attachment 8492806 [details] [diff] [review]
7_callsites_in_image.patch

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

Looks good!
Attachment #8492806 - Flags: review?(seth) → review+
Attachment #8492808 - Flags: review?(benjamin) → review+
Attachment #8492809 - Flags: review?(benjamin) → review?(mak77)
Attachment #8492809 - Flags: review?(mak77) → review+
Depends on: 1084481
I didn't see responses to comment 19 or comment 23.
Flags: needinfo?(mozilla)
(In reply to Boris Zbarsky [:bz] from comment #19)
> And given that, can we do something cheaper than constructing a UUID here
> For example, can we pass nullptr for the principal argument to
> NS_NewInputStreamChannel or something?

Unfortunately we can't, because the LoadInfo constructor needs a requesting principal.

> If not, doing what you do here is
> probably OK for now, assuming the performance isn't too terrible; please do
> write a testcase that loads a bunch of javascript: things with and without
> this patch and see whether the difference is detectable.

I tested a bunch of things, using timing mechanisms in C++ and also in JS. One thing I did (amongst others) is the attached testcase. Running it with, and without the patches applied. It's a mochitest, which I run on an optimized build using nice -n -20 to minimize operating scheduler effects. I performed 10 repetitions each to get some kind of stable comparison. The measured time in both cases was between 50 and 52ms. I think it's really hard to measure the performance impact here. At least we see that it's not causing a measurable performance decrease.
(In reply to Karl Tomlinson (:karlt) from comment #23)
> Comment on attachment 8492804 [details] [diff] [review]
> 5_callsites_in_extensions.patch
> 
> I don't know about SEC_NORMAL and TYPE_OTHER.
> 
> I trust neither of these affect the behavior of URI_DANGEROUS_TO_LOAD?

Sorry for leaving you in limbo for such a long time. Currently there are no consumers of loadInfo (besides CSP and MCB redirect handling). To answer your question, neither of those flags (nor the loadinfo in general) affect the behavior of URI_DANGEROUS_TO_LOAD.
Flags: needinfo?(mozilla)
You need to log in before you can comment on or make changes to this bug.