Closed
Bug 1063197
Opened 10 years ago
Closed 10 years ago
Storing LoadInfo on all channels created through NS_NewInputStreamChannel
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: ckerschb, Assigned: ckerschb)
References
(Blocks 1 open bug)
Details
Attachments
(12 files, 1 obsolete file)
(deleted),
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Similar to bug 1038756, we should append the loadInfo on all channels created through NS_NewInputStreamChannel.
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?
Assignee | ||
Comment 2•10 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8492800 -
Flags: review?(mcmanus)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8492801 -
Flags: review?(mcmanus)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8492802 -
Flags: review?(jst)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8492803 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8492804 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8492805 -
Flags: review?(roc)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8492806 -
Flags: review?(seth)
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8492808 -
Flags: review?(benjamin)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8492809 -
Flags: review?(gpascutto)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8492810 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 15•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8492801 -
Flags: review?(mcmanus) → review+
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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)
Assignee | ||
Comment 18•10 years ago
|
||
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 19•10 years ago
|
||
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 20•10 years ago
|
||
Comment on attachment 8492807 [details] [diff] [review]
8_callsites_in_parser.patch
Probably OK, yeah.
Attachment #8492807 -
Flags: review?(bzbarsky) → review+
Comment 21•10 years ago
|
||
Comment on attachment 8492810 [details] [diff] [review]
11_callsites_in_docshell.patch
r=me
Attachment #8492810 -
Flags: review?(bzbarsky) → review+
Comment 22•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8492802 -
Flags: review?(jst) → review+
Comment 23•10 years ago
|
||
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-
Assignee | ||
Comment 25•10 years ago
|
||
(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)
Attachment #8493877 -
Flags: review?(roc) → review+
Comment 26•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8492808 -
Flags: review?(benjamin) → review+
Updated•10 years ago
|
Attachment #8492809 -
Flags: review?(benjamin) → review?(mak77)
Updated•10 years ago
|
Attachment #8492809 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 27•10 years ago
|
||
Target Milestone: --- → mozilla36
Comment 28•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7e905781c715
https://hg.mozilla.org/mozilla-central/rev/a87d8dff37ca
https://hg.mozilla.org/mozilla-central/rev/7de43e0c3879
https://hg.mozilla.org/mozilla-central/rev/7921e44f3544
https://hg.mozilla.org/mozilla-central/rev/eabbbfc94865
https://hg.mozilla.org/mozilla-central/rev/dff13898998d
https://hg.mozilla.org/mozilla-central/rev/89f80a2a6431
https://hg.mozilla.org/mozilla-central/rev/547202c856b4
https://hg.mozilla.org/mozilla-central/rev/3de863f08848
https://hg.mozilla.org/mozilla-central/rev/9eeedb28b328
https://hg.mozilla.org/mozilla-central/rev/a5208bb3c5da
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 29•10 years ago
|
||
I didn't see responses to comment 19 or comment 23.
Flags: needinfo?(mozilla)
Assignee | ||
Comment 30•10 years ago
|
||
(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.
Assignee | ||
Comment 31•10 years ago
|
||
(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.
Description
•