Right now we hang a principal off channels. We should enable hanging an object that contains more information about the loading context. In particular, we will want a principal in there, and a CSP (not identical, in the system principal case!) and maybe other info Henri wanted? General plan: 1) Make things that get the channel's owner try to QI to this other object and if that fails QI to principal. 2) Make docshell set up this object on its loads. 3) Have docshell always set a principal in the object, with a boolean for whether it should be inherited.

Is this something that we want on channels? As opposed to introducing an object tracking the whole load life cycle across redirects (see bug 779959 comment 22)?

The idea was to do something like bug 779959 comment 22. In particular, definitely passing it across redirects.

(In reply to Boris Zbarsky [:bz] from comment #0) > Right now we hang a principal off channels. We should enable hanging an > object that contains more information about the loading context. How would be this different than nsILoadContext? We should consider how this interacts with e10s, and in particular how this would interact with other work that would try to move CSP enforcement, mixed content enforcement, etc. into the parent process in the e10s case.

> How would be this different than nsILoadContext? That's a good question. Conceptually it might now be. In practice right now nsILoadContext is shared across a bunch of channels (and in particular, is usually just gotten off the loadgroup, not the channel directly). We could change that, of course, but then we should make nsILoadContext live as a property on the channel, not off of notification callbacks... > We should consider how this interacts with e10s, and in particular how this would > interact with other work that would try to move CSP enforcement, mixed content > enforcement, etc. into the parent process in the e10s case. So what I was going to do was just throw in the CSP and principal of the document that started the load into a single object. Designing something that can represent this information (effectively something that identifies the originating document to the parent process) is a lot harder... and still spoofable if the content process gets compromised. :( Given this last, I'd like to understand what the actual goals would be for moving CSP enforcement to the parent, apart from making redirects easier to handle.

(In reply to Boris Zbarsky [:bz] from comment #4) > > How would be this different than nsILoadContext? > > That's a good question. > > Conceptually it might now be. In practice right now nsILoadContext is > shared across a bunch of channels (and in particular, is usually just gotten > off the loadgroup, not the channel directly). > > We could change that, of course, but then we should make nsILoadContext live > as a property on the channel, not off of notification callbacks... I think making the nsILoadContext a property of the channel is a good idea. nsHttpChannel already kind of treats it as such; see GetLoadContextInfo and NS_GetAppInfo, which need to be modified to handle e10s beyond the scope of B2G app processes. > So what I was going to do was just throw in the CSP and principal of the > document that started the load into a single object. I suggest we try to make the nsILoadContext do this, if we can. The only issue would be if frames need different contexts and whether reusing nsILoadContext would cause trouble there. > Designing something > that can represent this information (effectively something that identifies > the originating document to the parent process) is a lot harder... and still > spoofable if the content process gets compromised. :( In e10s, on the parent side of things we synthesize a fake nsILoadContext when we receive a message from the child. > Given this last, I'd like to understand what the actual goals would be for > moving CSP enforcement to the parent, apart from making redirects easier to > handle. To be clear, enforcing CSP in the parent process is not an *immediate* need, but something that should be eventually done. Let's say your CSP says "only load subresources from my site." Then, if/when we have multiple sandboxed child processes, enforcing this kind of CSP adds another layer of sandboxing by preventing any attempts to exfiltrate data. Obviously, there are some aspects of CSP that we won't be able to effectively enforce, but I suspect that for many kinds of websites, such as banks, such enforcement will be useful and effectivewhen implemented. You are right that if the child maintains this object and sends it to the parent, then the child could spoof whatever information it wants. That is why we need the parent process to have an accurate and secure way to know, at least, the top-level principal, CSP, mixed content blocking state, etc. independent of what the child tells it. (The child could still send navigation events to the parent to change the top-level principal, but the parent process might elect, for example, to load the navigation destination document in a separate process or whatever.)

> I think making the nsILoadContext a property of the channel is a good idea. OK. Let's say we did that... > I suggest we try to make the nsILoadContext do this, if we can. As long as you're OK with it usually returning null for both at first, sure. > The only issue would be if frames need different contexts I don't know what you mean. Every load needs a different context for purposes of this bug. > In e10s, on the parent side of things we synthesize a fake nsILoadContext when we > receive a message from the child. Sure. But how do you plan to send over the principal and CSP from the child... in a way that's at all trustworthy? > the top-level principal, CSP The <meta> version of CSP makes that last _really_ hard.

The other issue with using nsILoadContext here is that currently the nsILoadContext for a load can in fact change when it moves across loadgroups (e.g. the save as case) but if we hang a shim nsILoadContext off the channel directly that won't be able to change at that point. Maybe we don't care and it's OK to keep pointing to the old docshell for parts of the loadcontext data in that case. The other question is: are we OK with having an nsILoadContext (or whatever interface we add if we add one) attribute on nsIChannel, or will we need to make it nsISupports?

> are we OK with having an nsILoadContext (or whatever interface we add if we add one) > attribute on nsIChannel Specifically, we cannot use the current "get it off the notification callbacks" approach for the object I need to add here, because the notification callbacks are not usefully per-channel in the case of document loads (because the docshell is the notification callbacks) and I don't want to deal with the very likely compat fallout from changing what the notification callbacks are on document channels just to make this minor change. This is blocking several things at this point, so I'd really like people to decide what they want to do here. My personal opinion is that we can just introduce a new interface (it can be nsILoadContext if preferred, but would be simpler if it doesn't have all that baggage) that docshell could stash in the owner field on the channel. Consumers would QI to that interface before QIing to nsIPrincipal, thus allowing us to do the new functionality while preserving compat with existing code that sets the owner to an nsIPrincipal.

(In reply to Boris Zbarsky [:bz] from comment #6) > > The only issue would be if frames need different contexts > > I don't know what you mean. Every load needs a different context for > purposes of this bug. I see. I was expecting that the CSP and the principle you were wanting to stash there *would* be shared across multiple loads. > > In e10s, on the parent side of things we synthesize a fake nsILoadContext > > when we receive a message from the child. > > Sure. But how do you plan to send over the principal and CSP from the > child... in a way that's at all trustworthy? The parent process needs to keep track of what principal and CSP is in effect for the document, and when it synthesizes its nsILoadContexts it needs to use this parent-controlled information, not information sent over from the child. (Same for security indicator state.) > > the top-level principal, CSP > > The <meta> version of CSP makes that last _really_ hard. No, the rules for <meta> in CSP 1.1 make what I'm suggesting work. If you have a CSP from an HTTP header then the <meta> CSP is ignored. That means that in e10s we can have an "augmentCSPWithAdditionalRestrictions" message that the child sends to the parent to always strengthen, never loosen, parent-enforced CSP.

(In reply to Boris Zbarsky [:bz] from comment #8) > This is blocking several things at this point, so I'd really like people to > decide what they want to do here. > > My personal opinion is that we can just introduce a new interface (it can be > nsILoadContext if preferred, but would be simpler if it doesn't have all > that baggage) that docshell could stash in the owner field on the channel. > Consumers would QI to that interface before QIing to nsIPrincipal, thus > allowing us to do the new functionality while preserving compat with > existing code that sets the owner to an nsIPrincipal. I am fine with this, since it is blocking urgent work. It would be great if you could keep the things I mentioned in mind when working on this, but I am not (and wasn't) intending for the consideration of all those things to result in extra blocking work at this point in time. More, I just want to avoid us designing ourselves into a corner.

Looking at the bugs this bug blocks, this "document settings object" looks a lot like an nsIContentPolicy to me. "Document settings object" implies that this would be the object that holds the information that we will use to compute whether or not to do a load; nsIContentPolicy is an object that holds such state and the computation.

> I was expecting that the CSP and the principle you were wanting to stash there *would* be > shared across multiple loads. Ah, very much no. I mean, they will get propagated to the document that results from the load, and might get used for multiple loads from there but each document load needs its own CSP/principal pair. > The parent process needs to keep track of what principal and CSP is in effect for the > document Toplevel document, I guess. That seems fine, sure... > the rules for <meta> in CSP 1.1 make what I'm suggesting work. My point was that you can't actually rely on the parent process to enforce <meta> CSP because the child can just totally lie about the <meta> bits. Which is no worse than doing it in the child process, of course. > It would be great if you could keep the things I mentioned in mind when working on this I can do that, sure. > this "document settings object" looks a lot like an nsIContentPolicy to me Er... an nsIContentPolicy is a policy implementation of some sort; these are generally services. I'm producing data to be consumed by such a policy: the principal and CSP of the thing that started the load. The information _can_ be used to decide what to do with the document load itself (as in bug 418354), but can also be used to decide what to do with subresources of the document once the document has loaded (bug 923902). I'm totally up for better naming here, of course. "load context" would in fact be nice if it were not taken.... For the record, here is what I plan to store in the object at the moment, for document loads: 1) The principal responsible for starting the load of the document. 2) A boolean indicating whether this document should inherit the principal from item #1. 3) The CSP for the document responsible for starting the load of this document. Redirects would copy over the object to the new channel, but force the #2 boolean to false in the process. Henri then wants to add a readyState value to that object (see bug 779959 comment 22) which would track the current state of the load.

(In reply to Boris Zbarsky [:bz] from comment #12) > > the rules for <meta> in CSP 1.1 make what I'm suggesting work. > > My point was that you can't actually rely on the parent process to enforce > <meta> CSP because the child can just totally lie about the <meta> bits. > Which is no worse than doing it in the child process, of course. Yes, of course. My point is that the parent can enforce CSP from HTTP headers in a secure way, even if it can't enforce CPS from <meta>. This is probably the most unfortunate aspect of adding <meta> support to CSP, long-term. At least the <meta> tag could not be used by the child to circumvent provided-by-HTTP CSP, and that's why it is still useful to (eventually) enforce CSP in the parent. > 1) The principal responsible for starting the load of the document. > 2) A boolean indicating whether this document should inherit the principal > from item #1. > 3) The CSP for the document responsible for starting the load of this > document. > > Redirects would copy over the object to the new channel, but force the #2 > boolean to false in the process. > > Henri then wants to add a readyState value to that object (see bug 779959 > comment 22) which would track the current state of the load. It seems like the mixed content blocking enabled/disabled state might need to be there too. Also, maybe some stuff from nsILoadContext should be moved there. It seems like whatever problems we're trying to solve with this rafactoring also likely apply to mixed content blocking and private browsing mode tracking, at least. (I'm not so familiar with the other attributes of nsILoadContext, though they seem like they are mostly about storing data to decide what to load and what to block.)

> It seems like the mixed content blocking enabled/disabled state might need to be there > too. That doesn't seem unreasonable, at first glance. Where is that stored right now? > private browsing mode tracking This is done on a per-docshell level (per-window, even), not per-load, right?

(In reply to Boris Zbarsky [:bz] from comment #14) > > It seems like the mixed content blocking enabled/disabled state > > might need to be there too. > > That doesn't seem unreasonable, at first glance. Where is that stored right > now? This setting is fixed per top-level-document/docshell. > > private browsing mode tracking > > This is done on a per-docshell level (per-window, even), not per-load, right? OK, based just on this I understand what you're trying to do better. Yes, private browsing mode is per-top-level-docshell or even per-window. CSP is per-DocShell, not per-load, isn't it? I would think it has to be. I can understand why the principal is per-load, but not why CSP is per-load.

> I can understand why the principal is per-load, but not why CSP is per-load. The problem I'm trying to solve with this bug initially is this: <!-- I'm a document with CSP --> <iframe src="about:blank"></iframe> The iframe should have the document's CSP applied. Right now that works by storing the CSP in the principal of the parent document and then having the iframe inherit that principal (via the .owner on the channel) as part of its load. That gives the iframe the same principal, and hence same CSP, as the parent document. But now if I load a different URI in that same iframe (so same docshell) it will get another principal (maybe the same, maybe not), which will have its own CSP. All good so far, but for bug 923902 we want to decouple the principal from the CSP: we want all chrome documents to have the system principal, but want them to be able to have different CSPs. And at the same time, I would like the CSP to continue inheriting into about:blank or data: subframes of those chrome documents, both because that's how the spec says it should work and because that's the sane thing to do from a security viewpoint. What that means is that I need to pass both a principal and a CSP down to the subframe load, and I need to do this again and again for every load in that subframe. That's why the CSP-to-be-inherited on the channel ends up being per-load. This is not to be confused with deciding whether to do a load or not based on CSP, which is indeed per-document (not per-docshell, since a new document loaded in the same docshell will have different CSP).

(In reply to Boris Zbarsky [:bz] from comment #16) > What that means is that I need to pass both a principal and a CSP down to > the subframe load, and I need to do this again and again for every load in > that subframe. That's why the CSP-to-be-inherited on the channel ends up > being per-load. This is not to be confused with deciding whether to do a > load or not based on CSP, which is indeed per-document (not per-docshell, > since a new document loaded in the same docshell will have different CSP). I see. I think I was lead astray by the association of bug 418354 with this bug. AFAICT, bug 418354 could have been (can be) implemented in a perfectly fine way without this, now that I better understand what this is for. (Maybe it is still better to base the solution for bug 418354 on this framework, but it isn't required by any means, AFAICT.)

Sorry I'm late to the party here: > are we OK with having an nsILoadContext (or whatever interface we add > if we add one) attribute on nsIChannel, or will we need to make it nsISupports? Doesn't need to be nsISupports. At this point in the game we're fine with Necko knowing about at least a limited subset of higher-up IDLs (basically anything that's needed, but we try to keep the list short). Necko channels already know about nsILoadContext in particular (we use them to get AppId/InBrowser/IsPrivateBrowsing all over the place). > maybe some stuff from nsILoadContext should be moved there That would be fine. We use nsILoadContext because that's where appId/etc wound up, but there's a bunch of fields in the IDL that we can't support in the parent for remote channels (window objects) so we wind up throwing if you try to access them (see /docshell/base/LoadContext.cpp, which is the stub object we create on the parent). We'd also be fine moving from getting nsILoadContext (or whatever) from needing to be via the callbacks object--I don't *think* we ever get them via the loadGroup, for instance, so setting them via some channel attribute or whatever ought to work fine.

Actually we might get the LoadContext on the child from the LoadGroup (i.e. channel's own callbacks don't provide it, but loadGroup's do). I don't know that setup well. But we definitely don't do that on the parent for remote channels--we still don't even know on the parent what loadGroup a remote channel belongs to on the child (and hoping to keep it that way :)

For now just preserve the old behavior of storing the principal in the session history.

We're going to want to check whether a channel has an existing owner before sticking a new one on there, but some channels currently assert if you just ask them whether they have an owner.

Comment on attachment 8384652 [details] [diff] [review] part 6. Make HTTP redirects propagate along the load owner. Review of attachment 8384652 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +1790,5 @@ > + if (loadOwner) { > + nsCOMPtr<nsISupports> newOwner; > + newChannel->GetOwner(getter_AddRefs(newOwner)); > + if (!newOwner) { > + // Make sure we don't inherit principals across redirects, no matter what. why exactly? are you just copying the current behavior (w/o this patch) where we just don't pass the owner to the target (new) channel?

> why exactly? Because if http://foo.com makes a request to http://bar.com and the latter redirects to data:text/html,stuff, we do not want the resulting document to have the origin of foo.com. We could maybe make a case for it having the origin of bar.com, but in practice it's safest to just give it a nonce origin. And yes, that's the current behavior, but the current behavior, which is very much not accidental.

Comment on attachment 8384643 [details] [diff] [review] part 1. Introduce an nsILoadOwner interface that can be used to propagate information like the loading principal and whether it should be inherited along with the load. s Review of attachment 8384643 [details] [diff] [review]: ----------------------------------------------------------------- Looking at these patches, I think I'm probably not the right reviewer here. I could be, with a bit of background digging into the Necko stuff, but I've got too much on my plate to do a good job of that right now. Bouncing to smaug. I'm really sorry it took 3 days for me to come to this conclusion. :-( ::: docshell/base/nsILoadOwner.idl @@ +19,5 @@ > + */ > + readonly attribute nsIPrincipal loadingPrincipal; > + > + /** > + * A C++-friendly version of loadingPrincipal. Can't we just do this inline with %{C++ in the header? That seems preferable IMO.

> Can't we just do this inline with %{C++ in the header? That seems preferable IMO. I guess we could add a %{C++ member to the interface itself... I'm not sure how ugly we want to make the IDL. ;)

(In reply to Boris Zbarsky [:bz] from comment #34) > > Can't we just do this inline with %{C++ in the header? That seems preferable IMO. > > I guess we could add a %{C++ member to the interface itself... I'm not sure > how ugly we want to make the IDL. ;) That's what I did for nsIPrincipal. IMO it's better to do it that way, because the helper doesn't exist in multiple places, and it's simpler to fix if/when we land support for non-primitive [infallible] attributes.

Comment on attachment 8384644 [details] [diff] [review] part 2. Teach the security manager about nsILoadOwner. > if (owner) { >+ nsCOMPtr<nsILoadOwner> loadOwner(do_QueryInterface(owner)); >+ if (loadOwner && loadOwner->GetInheritPrincipal()) { >+ NS_IF_ADDREF(*aPrincipal = loadOwner->LoadingPrincipal()); Why we need _IF? The documentation for nsILoadOwner says it is never null. Just MOZ_ASSERT that *aPrincipal is non-null.

Comment on attachment 8384648 [details] [diff] [review] part 4. Create an implementation of nsILoadOwner. ># HG changeset patch ># User Boris Zbarsky <bzbarsky@mit.edu> ># Date 1393865603 18000 ># Mon Mar 03 11:53:23 2014 -0500 ># Node ID 98c1b5d7daa75de9988bc13bb821d712b2f65127 ># Parent 2fe22f415635c8594ccddcd1437208f5a7f69bf4 >Bug 965413 part 4. Create an implementation of nsILoadOwner. r=bholley > >diff --git a/docshell/base/LoadOwner.cpp b/docshell/base/LoadOwner.cpp >new file mode 100644 >--- /dev/null >+++ b/docshell/base/LoadOwner.cpp >@@ -0,0 +1,52 @@ >+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* vim: set sw=2 ts=8 et tw=80 : */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+#include "mozilla/LoadOwner.h" >+ >+#include "mozilla/Assertions.h" >+#include "nsISupportsImpl.h" >+#include "nsISupportsUtils.h" >+ >+namespace mozilla { >+ >+LoadOwner::LoadOwner(nsIPrincipal* aPrincipal, >+ bool aInheritPrincipal) >+ : mPrincipal(aPrincipal) >+ , mInheritPrincipal(aInheritPrincipal) >+{ >+ MOZ_ASSERT(aPrincipal); >+} >+ >+NS_IMPL_ISUPPORTS1(LoadOwner, nsILoadOwner) >+ >+NS_IMETHODIMP >+LoadOwner::GetLoadingPrincipal(nsIPrincipal** aPrincipal) >+{ >+ NS_ADDREF(*aPrincipal = mPrincipal); >+ return NS_OK; >+} >+ >+nsIPrincipal* >+LoadOwner::LoadingPrincipal() >+{ >+ return mPrincipal; >+} >+ >+NS_IMETHODIMP >+LoadOwner::SetInheritPrincipal(bool aInheritPrincipal) >+{ >+ mInheritPrincipal = aInheritPrincipal; >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+LoadOwner::GetInheritPrincipal(bool* aInheritPrincipal) >+{ >+ *aInheritPrincipal = mInheritPrincipal; >+ return NS_OK; >+} >+ >+} // namespace mozilla >diff --git a/docshell/base/LoadOwner.h b/docshell/base/LoadOwner.h >new file mode 100644 >--- /dev/null >+++ b/docshell/base/LoadOwner.h >@@ -0,0 +1,36 @@ >+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* vim: set sw=2 ts=8 et tw=80 : */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+#ifndef mozilla_LoadOwner_h >+#define mozilla_LoadOwner_h >+ >+#include "nsIPrincipal.h" >+#include "nsILoadOwner.h" >+ >+namespace mozilla { >+ >+/** >+ * Class that provides an nsILoadOwner implementation. >+ */ >+class LoadOwner MOZ_FINAL : public nsILoadOwner >+{ >+public: >+ NS_DECL_ISUPPORTS >+ NS_DECL_NSILOADOWNER >+ >+ // aPrincipal MUST NOT BE NULL. >+ LoadOwner(nsIPrincipal* aPrincipal, >+ bool aInheritPrincipal); >+ >+private: >+ nsCOMPtr<nsIPrincipal> mPrincipal; >+ bool mInheritPrincipal; >+}; >+ >+} // namespace mozilla >+ >+#endif // mozilla_LoadOwner_h >+ >diff --git a/docshell/base/moz.build b/docshell/base/moz.build >--- a/docshell/base/moz.build >+++ b/docshell/base/moz.build >@@ -41,20 +41,22 @@ EXPORTS += [ > 'nsIScrollObserver.h', > 'nsIWebShellServices.h', > 'SerializedLoadContext.h', > ] > > EXPORTS.mozilla += [ > 'IHistory.h', > 'LoadContext.h', >+ 'LoadOwner.h', > ] > > UNIFIED_SOURCES += [ > 'LoadContext.cpp', >+ 'LoadOwner.cpp', > 'nsAboutRedirector.cpp', > 'nsDefaultURIFixup.cpp', > 'nsDocShellEditorData.cpp', > 'nsDocShellEnumerator.cpp', > 'nsDocShellLoadInfo.cpp', > 'nsDocShellTransferableHooks.cpp', > 'nsDownloadHistory.cpp', > 'nsDSURIContentListener.cpp',

Comment on attachment 8384648 [details] [diff] [review] part 4. Create an implementation of nsILoadOwner. >+NS_IMETHODIMP >+LoadOwner::SetInheritPrincipal(bool aInheritPrincipal) >+{ >+ mInheritPrincipal = aInheritPrincipal; >+ return NS_OK; >+} Actually, is it ever ok to override false mInheritPrincipal with true? Perhaps something like MOZ_ASSERT(mInheritPrincipal || !aInheritPrincipal); // Assert hard in debug builds NS_ENSURE_TRUE(mInheritPrincipal || !aInheritPrincipal, NS_ERROR_INVALID_ARG); mInheritPrincipal = aInheritPrincipal; return NS_OK; With that, r=me (or explain why it is ok to override)

Comment on attachment 8384647 [details] [diff] [review] part 3. Teach the docshell code that sets up session history entries about nsILoadOwner. Somewhat odd. nsISHEntry.owner is principal if we're inheriting principal, otherwise nsILoadOwner. Maybe other patches explain why this behavior.

Comment on attachment 8384651 [details] [diff] [review] part 5. Make it safe to GetOwner() and SetOwner() on channels without asserting. I guess this is ok.

(In reply to Olli Pettay [:smaug] from comment #38) > Perhaps something like > MOZ_ASSERT(mInheritPrincipal || !aInheritPrincipal); // Assert hard in debug > builds > NS_ENSURE_TRUE(mInheritPrincipal || !aInheritPrincipal, > NS_ERROR_INVALID_ARG); > mInheritPrincipal = aInheritPrincipal; > return NS_OK; > > With that, r=me > (or explain why it is ok to override) Or change the interface so that there is void disableInheritingPrincipal(); and [infallible] readonly attribute boolean inheritPrincipal;

Comment on attachment 8384654 [details] [diff] [review] part 8. Use a LoadOwner for loading documents in various places where we force a particular principal. Hmm, this is somewhat error prone. What if someone doesn't pass LoadOwner but keeps using principal. We'd get inconsistent behavior.

Comment on attachment 8384655 [details] [diff] [review] part 9. Use a LoadOwner as needed for loading stylesheets. (Assuming we want to keep the error prone SetOwner handling)

Comment on attachment 8384657 [details] [diff] [review] part 10. Use a LoadOwner in SetUpChannelOwner. > /** >- * Set the given principal as the owner of the given channel, if >- * needed. aURI must be the URI of aChannel. aPrincipal may be >- * null. If aSetUpForAboutBlank is true, then about:blank will get >- * the principal set up on it. If aForceOwner is true, the owner >- * will be set on the channel, even if the principal can be determined >- * from the channel. >- * The return value is whether the principal was set up as the owner >- * of the channel. >+ * Set the given principal as the principal on the nsILoadOwner of the given >+ * channel, and tell the channel to inherit it if needed. aURI must be the >+ * URI of aChannel. aPrincipal may be null. If aInheritForAboutBlank is >+ * true, then about:blank will be told to inherit the principal. If >+ * aForceInherit is true, the channel will be told to inherit the principal >+ * no matter what. This comment is a bit vague what happens if aPrincipal is null. Since in that case we don't actually create loadOwner at all. But I'll need to continue looking at this patch tomorrow. In general I'd really prefer if nsIChannel.owner wasn't nsISupports. Can it not be nsILoadOwner?

> Why we need _IF? The documentation for nsILoadOwner says it is never null. Indeed, habit. Will remove the _IF bit. > Actually, is it ever ok to override false mInheritPrincipal with true? Turns out, SetInheritPrincipal is totally unused in the patches as they ended up, so I just made it a readonly attribute for now. > nsISHEntry.owner is principal if we're inheriting principal, otherwise nsILoadOwner That's just a mistake; good catch! It should be like so: if (loadOwner) { // For now keep storing just the principal in the SHEntry, which // means only storing it when it should be inherited. if (loadOwner->GetInheritPrincipal()) { owner = loadOwner->LoadingPrincipal(); } else { owner = nullptr; } } Does that make more sense? > What if someone doesn't pass LoadOwner but keeps using principal. Then for now everything still works. That's why all the consumers check for both loadowner and principal. This is not a hypothetical situation; some cases (chrome channels, about: channels) are still using a principal as owner even after these patches. Not to mention whatever extensions are doing. > This comment is a bit vague what happens if aPrincipal is null Rephrased like so: * Set the given principal as the principal on the nsILoadOwner of the given * channel, and tell the channel to inherit it if needed. aPrincipal may be * null, in which case this method is a no-op. * * If aPrincipal is not null, aURI must be the URI of aChannel. If * aInheritForAboutBlank is true, then about:blank will be told to inherit the * principal. If aForceInherit is true, the channel will be told to inherit * the principal no matter what, as long as the principal is not null. * * The return value is whether the channel was told to inherit the principal > Can it not be nsILoadOwner? Not without breaking compat, sadly. I do think we want to go there eventually, but it involves fixing more of our internal code, auditing addons, doing a deprecation period, and all that jazz. I'm happy to at least get that rolling in some followup bugs filed... Do you have opinions on Bobby's suggestion to add inline C++ bits to the IDL?

I'd prefer to add as little C++ to idl as possible. That keeps idl files easier to read. The getter isn't exactly perf-critical.

Comment on attachment 8384647 [details] [diff] [review] part 3. Teach the docshell code that sets up session history entries about nsILoadOwner. Ok, new patch coming for this then.

Comment on attachment 8384654 [details] [diff] [review] part 8. Use a LoadOwner for loading documents in various places where we force a particular principal. Do we need to handle WebSocket somehow? I guess no, since it is rather special.

Comment on attachment 8384657 [details] [diff] [review] part 10. Use a LoadOwner in SetUpChannelOwner. r=me with the comment fixed. (Code moves made the patch a bit hard to read.)

I just talked to sicking. His feedback: 1) Rename to nsILoadInfo. 2) Add a new property on channels (.loadInfo?) instead of storing in .owner. 3) Make redirects pass along the loadInfo unchanged. 4) Add a boolean to channels that says whether they inherit the principal. Make it writable so the loader can say whether to inherit or not. 5) Stop setting .owner when opening channels. 5) Fix GetChannelPrincipal to check for sandboxing first, then .owner, then the inheritance stuff. Jason, does that sound reasonable to you on the necko end?

Er, I guess Patrick is the necko owner now...

(In reply to Boris Zbarsky [:bz] from comment #52) > I just talked to sicking. His feedback: > > 1) Rename to nsILoadInfo. > 2) Add a new property on channels (.loadInfo?) instead of storing in .owner. This may break some addons, no? So should we remove .owner? And just have attribute nsILoadInfo .loadInfo > 3) Make redirects pass along the loadInfo unchanged. Hmm, really? Why exactly. That is somewhat surprising > 4) Add a boolean to channels that says whether they inherit the principal. But principal lives where? in loadInfo? Then it shouldn't be channel which says whether principal from loadInfo is inherited. Or would we still use .owner as principal? (if so, could we rename it to .principal or .loadinPrincipal?) > Make it > writable so the loader can say whether to inherit or not. > 5) Stop setting .owner when opening channels. But do what? Set .loadInfo? > 5) Fix GetChannelPrincipal to check for sandboxing first, then .owner, then > the inheritance > stuff. Feels even more complicated than just using .owner either for principal or for LoadOwner/LoadInfo

> This may break some addons, no? You mean if addons implement channels? If addons just _set_ .owner that will continue to work for now. Long-term I would live to remove .owner, yes. > Hmm, really? Why exactly. We need some state passed along with no changes, basically. Like "who started this load?". > But principal lives where? in loadInfo? The principal that started the load, yes. > Then it shouldn't be channel which says whether principal from loadInfo is inherited. Why not? Generally speaking, data: and javascript: channels would say "yes" unless otherwise overridden; others would say no... > But do what? Set .loadInfo? Yes. Note that some Gecko code (in channel implementations themselves) would continue to set .owner. Again, long-term we want to get rid of that.

Comment on attachment 8384654 [details] [diff] [review] part 8. Use a LoadOwner for loading documents in various places where we force a particular principal. Review of attachment 8384654 [details] [diff] [review]: ----------------------------------------------------------------- Ollie: I'm not sure if we need this for websockets or not. Websockets get bootstrapped as HTTP channels, so if we need the LoadOwner for the HTTP part, then we need to give it to websockets (and have websockets give it to the HTTP channel they use for bootstrapping). That said, I'm not actually sure what we're using LoadOwner for. There's a bunch of references to channel principal in nsHttpChannel and HttpChannelParent, but it doesn't look like any of the patches in this bug change the basic GetPrincipal function: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#6157 So I don't feel like I know enough to give good feedback here.

The patches in this bug change what securityManager->GetChannelPrincipal() returns, hence what GetPrincipal() returns.

Looking at the code some more, I see that we don't need to change nsHttpChannel::GetPrincipal because it gets it from the securityManager, which is changed by this patch. Patrick or Honza probably knows more than than me, but it looks like the principal is used for SSL options that I suspect we'll need when bootstrapping websockets over HTTPS? The rest of comment 52 sounds ok, to the extend that I understand this stuff (which is not as much as I'd like). Hopefully honza or Patrick can chime in here.

I guess in my mind a channel would have some kind of (mutable) .loadState which collects various stuff during the load. First it would have originatingPrincipal or some such and then a flag for inheritance etc. In a way, something close to PerformanceTiming (although that is about timing).

I'm good with comments 52/55 wrt websockets, the websocket channel (which is not a nsIChannel at all for reasons that escape me at the moment) doesn't seem to know of an owner at all, much less the new and improved load info.. given that the ws channel instantiates a http[s] channel for bootstrapping itself, this seems like an oversight.. it should have that info and should be passing it to the new channel. perhaps that's a different bug.

FYI: IIRC websockets are not nsIChannels mostly because that IDL assumes a single request/response model that maps poorly (fields like .contentLength don't make sense). It might be worth shoehorning it to support nsIChannel anyway at some point, but we haven't gotten to that point yet.

So I think we're good here for the plan in comment 52. If we don't have cycles in this bug for supporting websockets getting the LoadInfo and passing it to the bootstrap HTTP channel, please open a bug for that when this closes.

Comment on attachment 8392294 [details] [diff] [review] Part 1 updated to review comments Review of attachment 8392294 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling request here. My recommendation is to stick a "may inherit principal" flag and a "loads in sandbox" flag on nsILoadInfo. Neither of these facts change for the lifetime of a request, even through redirects. Then make nsIScriptSecurityManager.GetChannelPrincipal look at the URI_INHERITS_SECURITY_CONTEXT flag of the loaded URI, the "may inherit principal" flag, and check if we've done redirects, to determine if we should actually inherit the principal or not. Likewise it would check the "loads in sandbox" flag and if it's set return a null principal. Not fully sure if that will make everything work as it should. It'd be great to hear if it wouldn't as it affects the security-hooks proposal i'm working on.

OK. I've updated my patches for this and will post them. The renaming to LoadInfo meant I had to add some webrtc changes as a new part 1, and adding a new channel property becomes a new part 3. So what used to be part 1 is now part 2, and all the other parts got shifted over by 2.

For now just preserve the old behavior of storing the principal in the session history.

Try runs covering this stuff at https://tbpl.mozilla.org/?tree=Try&rev=35ba2ea7abd4 and https://tbpl.mozilla.org/?tree=Try&rev=743e45470c84 (the latter with a tweak to fix the e10s stuff).

Comment on attachment 8450810 [details] [diff] [review] part 10. Use a LoadInfo for loading documents in various places where we force a particular principal Review of attachment 8450810 [details] [diff] [review]: ----------------------------------------------------------------- true, false everywhere isn't particularly readable

Indeed. An enum for the values would be nicer.

> Indeed. An enum for the values would be nicer. I'll add enums for both of those arguments. Do you want me to repost the patches with that change, or just assume I get it right?

Since I got to it before you reviewed, updated patches coming up.

Comment on attachment 8450801 [details] [diff] [review] part 3. Add a .loadInfo property to channels Review of attachment 8450801 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabParent.cpp @@ +2193,5 @@ > + } > + NS_IMETHOD SetLoadInfo(nsILoadInfo* aLoadInfo) > + { > + mLoadInfo = aLoadInfo; > + return NS_OK; ws

Comment on attachment 8450803 [details] [diff] [review] part 5. Teach the docshell code that sets up session history entries about nsILoadInfo >+ if (!owner) { >+ nsCOMPtr<nsILoadInfo> loadInfo; >+ aChannel->GetLoadInfo(getter_AddRefs(loadInfo)); >+ if (loadInfo) { >+ // For now keep storing just the principal in the SHEntry. >+ if (loadInfo->GetLoadingSandboxed()) { >+ owner = do_CreateInstance(NS_NULLPRINCIPAL_CONTRACTID, &rv); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ return rv; >+ } >+ } >+ >+ if (loadInfo->GetMayInheritPrincipal()) { Missing else before if GetChannelPrincipal does quite a bit more and we'd just do GetOwner call again we do above. Forcing security manager to create a non-null principal here in some cases, possibly theoretical cases, feels wrong. (GetChannelPrincipal should be called GetOrCreateChannelPrincipal)

Comment on attachment 8451209 [details] [diff] [review] Interdiff from old part 4 to new part 6 >+ enum InheritType { { goes to its own line >+ enum SandboxType { ditto

Comment on attachment 8450808 [details] [diff] [review] part 8. Make HTTP redirects propagate along the load info Ok, so this is rather major change to the previous version of the patch, but now the important thing is to check the creators of loadinfo

Comment on attachment 8450814 [details] [diff] [review] part 13. Stop propagating null principal owners across redirects in nsHttpChannel, since we now handle that via loadInfo I don't yet understand the reason for this....

Comment on attachment 8451211 [details] [diff] [review] part 10. Use a LoadInfo for loading documents in various places where we force a particular principal >- channel->SetOwner(principal); >+ nsCOMPtr<nsILoadInfo> loadInfo = >+ new LoadInfo(principal, LoadInfo::eInheritPrincipal, >+ LoadInfo::eNotSandboxed); You have a tab here >+++ b/netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp >@@ -593,19 +593,19 @@ GetTabChild(nsIChannel* aChannel) > /* void asyncOpen (in nsIStreamListener aListener, in nsISupports aContext); */ > NS_IMETHODIMP > WyciwygChannelChild::AsyncOpen(nsIStreamListener *aListener, nsISupports *aContext) > { > LOG(("WyciwygChannelChild::AsyncOpen [this=%p]\n", this)); > > // The only places creating wyciwyg: channels should be > // HTMLDocument::OpenCommon and session history. Both should be setting an >- // owner. >- NS_PRECONDITION(mOwner, "Must have a principal"); >- NS_ENSURE_STATE(mOwner); >+ // owner or loadinfo. >+ NS_PRECONDITION(mOwner || mLoadInfo, "Must have a principal"); >+ NS_ENSURE_STATE(mOwner || mLoadInfo); It is not yet clear to me why we wouldn't have mLoadInfo here, but perhaps I'll figure it out.

Oh, I see. We store only owner in sh.

> I don't yet understand the reason for this.... It's basically dead code. It was added to support propagating the "you are sandboxed" state across HTTP redirects, but with the patches in this bug that state is already propagated via the loadinfo, and we're no longer setting a nullprincipal as the channel owner. > It is not yet clear to me why we wouldn't have mLoadInfo here We might have it. That's why I changed the precondition to "loadinfo or owner" instead of just "owner". Will fix the bits from comment 92, comment 93, comment 94, and remainder of comment 97.

Comment on attachment 8450814 [details] [diff] [review] part 13. Stop propagating null principal owners across redirects in nsHttpChannel, since we now handle that via loadInfo Ok, that is what I expected.

Comment on attachment 8451214 [details] [diff] [review] part 12. Use a LoadInfo in SetUpChannelOwner >+ * Set the given principal as the principal on the nsILoadOwner of the given nsILoadOwner? You mean nsILoadInfo >+ * channel, and tell the channel to inherit it if needed. aPrincipal may be >+ * null, in which case this method is a no-op. >+ * >+ * If aPrincipal is not null, aURI must be the URI of aChannel. If >+ * aInheritForAboutBlank is true, then about:blank will be told to inherit the >+ * principal. If aForceInherit is true, the channel will be told to inherit >+ * the principal no matter what, as long as the principal is not null. aIsSandBoxed should be documented too. > static bool SetUpChannelOwner(nsIPrincipal* aLoadingPrincipal, > nsIChannel* aChannel, > nsIURI* aURI, >- bool aSetUpForAboutBlank, >- bool aForceOwner = false); >+ bool aInheritForAboutBlank, >+ bool aIsSandboxed, >+ bool aForceInherit); It might be good to use enum here for bool params, but either way.

> nsILoadOwner? You mean nsILoadInfo Yes, good catch. > aIsSandBoxed should be documented too. Will do. > It might be good to use enum here for bool params, but either way. I think I'm ok leaving this as is for now.

Boris, it seems that this bug tries to solve similar problems to what we are trying to solve in the BetterNeckoSecurityHooks (aka RevampSecurityHooks) project initiated by Jonas [5]. As a first step for this project we are changing the API of channels so that channels can only be created by passing the requesting context (usually nsINode), or if not available, the requesting principal (note, if we hand the node, we can easily query the principal from that node), so that *no* channel can be created without a requesting principal. In addition, we are setting the contentPolicyType at construction time of a channel. Both, the principal as well as the contentPolicyType are frozen after construction so we can perform security checks based on that information throughout the lifetime of a channel, even after redirects. On top we are moving security checks into AsyncOpen() (and also regular Open()), so that ContentPolicies are called whenever a channel is about to be openend. Please note that this is only the first step of the that project. @MCB: The Node/Principal and the ContentPolicyType is sufficient to handle redirects. We already have written some tests that verify that MCB redirects can be handled correctly with our approach. @CSP: For CSP, the principal does not provide all the information needed to query the CSP. See http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#162 where we query the CSP from the node. Hence, we store the node on the channel as well. Please note, that our approach allows us to eliminate/remove the nsIChannelPolicy, which is currently attached to channels. Ultimately we are going to create a loadInfo-object that hangs of a channel in the end which holds the principal/node/contentPolicyType, etc. Most probably this object is going to be created within NS_NewChannel in nsNetUtil.h before we really create the channel through the ioService. So in case we want to extend that loadInfo, there is no need to change the *.idl of the ioService again. Please find some links underneath, especially relevant is [3], the sourcecode we have written so far. I was wondering how we can better coordinate the two projects to get the most synergy effects out of it. Let me know what you think! [1] Tracking Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1006868 [2] Attach Principal/Node/ContentPolicyType to Channel: https://bugzilla.mozilla.org/show_bug.cgi?id=1006881 [3] SourceCode: https://github.com/ckerschb/gecko-dev/compare/revampGeckoSecHook [4] wiki: https://wiki.mozilla.org/Security/Features/Revamp_Security_Hooks [5] Jonas proposal: https://etherpad.mozilla.org/BetterNeckoSecurityHooks

It sounds like you should be able to put your things in the loadinfo object this bug is adding, right? So apart from possible code conflicts I don't think there's a problem here... I was explicitly asked to get this in sooner rather than later (months ago :( ) because it's blocking other work that people really want to get done. If we can unblock them by landing bug 1006881 instead, that would work too, of course. but it's not clear to me that that bug is close to landing.

(In reply to Boris Zbarsky [:bz] from comment #104) > It sounds like you should be able to put your things in the loadinfo object > this bug is adding, right? So apart from possible code conflicts I don't > think there's a problem here... > > I was explicitly asked to get this in sooner rather than later (months ago > :( ) because it's blocking other work that people really want to get done. I know, I was one of the people asking for it :-). The only reason why I like Jonas approach better is that no channel can be created without passing a principal/contentPolicyType. Month ago, I was trying a similar approach to yours, adding setRequestPrincipal(), setRequestNode(), and setRequestContentType() on the channel [1] to handle MCB redirects correctly. The criticism I got was mostly that that approach is too error prone. If I interpret your patches correctly, we are trying to do a similar thing but instead of setting the three things explicitly we bundle them in a loadInfo object. What happens if we forget to set the loadInfo on a new Channel? With Jonas approach that could never happen. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=418354 > If we can unblock them by landing bug 1006881 instead, that would work too, > of course. but it's not clear to me that that bug is close to landing. We are currently talking to Jorge about addon compatibilty and as a first step would like to land the NewChannel-API, nothing else. That should be ready for reviews fairly soon. Is there anything else in this bug that I am missing?

> The only reason why I like Jonas approach better I don't think the two things are mutually exclusive. As in, we can add the new create-a-channel methods and have them create loadinfo instances. Or is the objection to adding the nsIChannel API? We can undo that change once we have a better way to get the loadinfo to the channel, right? The other parts of this bug are still needed no matter how the loadinfo ends up on the channel. > Is there anything else in this bug that I am missing? No. Just the ability to bundle up three bits of data (loading principal, whether you're sandboxed, and whether to inherit the loading principal) and put that bundle on channels, plus changing code to actually do that.

(In reply to Boris Zbarsky [:bz] from comment #106) > > The only reason why I like Jonas approach better > > I don't think the two things are mutually exclusive. As in, we can add the > new create-a-channel methods and have them create loadinfo instances. > > Or is the objection to adding the nsIChannel API? We can undo that change > once we have a better way to get the loadinfo to the channel, right? The > other parts of this bug are still needed no matter how the loadinfo ends up > on the channel. That sounds reasonable to, lets do that. > > > Is there anything else in this bug that I am missing? > > No. Just the ability to bundle up three bits of data (loading principal, > whether you're sandboxed, and whether to inherit the loading principal) and > put that bundle on channels, plus changing code to actually do that. Ok, that's great. Thanks for clarification!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #107) > (In reply to Boris Zbarsky [:bz] from comment #106) > > > The only reason why I like Jonas approach better > > > > I don't think the two things are mutually exclusive. As in, we can add the > > new create-a-channel methods and have them create loadinfo instances. > > > > Or is the objection to adding the nsIChannel API? Just to be concise, the good thing about your approach is that it's not going to break any addons that implement their own channels. Our approach will break addons by really changing the way how channels are created. So, using/landing your implementation soon we can handle e.g. redirects for MCB. At the same time we can work on a precise plan so that addons are affects and forced to update the code only once.

Right, for this bug an explicit goal was to not break addon compat so we didn't need to block on deprecation periods, etc. I agree that the long-term plan should involve a compat break here.

Comment on attachment 8450800 [details] [diff] [review] part 2. Introduce an nsILoadInfo interface that can be used to propagate information like the loading principal and whether it should be inherited along with the load. s Review of attachment 8450800 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsILoadInfo.idl @@ +27,5 @@ > + > + /** > + * If mayInheritPrincipal is true, the data coming from the channel > + * should use loadingPrincipal for its principal if it wants to > + * inherit the loading principal. Add a comment that this is ignored if loadingSandboxed is set. Also add a comment that this flag causes the principal to get inherited no matter what scheme is loaded from, including when loading from http: @@ +29,5 @@ > + * If mayInheritPrincipal is true, the data coming from the channel > + * should use loadingPrincipal for its principal if it wants to > + * inherit the loading principal. > + */ > + [infallible] readonly attribute boolean mayInheritPrincipal; Can we call this forceInheritPrincipal? Since it forces the principal to be inherited no matter what protocol is being loaded. I think *possibly* we will eventually want to let data: use a separate "allowIneritPrincipal" (or some such) flag which doesn't inherit into chrome: and/or doesn't inherit if there's been redirects (addons will be able to redirect any request, including ones to data:)

Talked to Jonas: I renamed to forceInheritPrincipal, changed the LoadInfo ctor so that forceInheritPrincipal is never true of loadingSandboxed is true, and documented that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d3daff592e https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5b5826f7e0 https://hg.mozilla.org/integration/mozilla-inbound/rev/6ca74bed32d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/de82396bc56e https://hg.mozilla.org/integration/mozilla-inbound/rev/970e43b1fab9 https://hg.mozilla.org/integration/mozilla-inbound/rev/c6791b67b381 https://hg.mozilla.org/integration/mozilla-inbound/rev/b53f72363f2c https://hg.mozilla.org/integration/mozilla-inbound/rev/16d73a72ac80 https://hg.mozilla.org/integration/mozilla-inbound/rev/41cd9b721602 https://hg.mozilla.org/integration/mozilla-inbound/rev/3542e1112df0 https://hg.mozilla.org/integration/mozilla-inbound/rev/b4e466a6f388 https://hg.mozilla.org/integration/mozilla-inbound/rev/999d34774ff6 https://hg.mozilla.org/integration/mozilla-inbound/rev/99b306c69791

https://hg.mozilla.org/mozilla-central/rev/f6d3daff592e https://hg.mozilla.org/mozilla-central/rev/cc5b5826f7e0 https://hg.mozilla.org/mozilla-central/rev/6ca74bed32d8 https://hg.mozilla.org/mozilla-central/rev/de82396bc56e https://hg.mozilla.org/mozilla-central/rev/970e43b1fab9 https://hg.mozilla.org/mozilla-central/rev/c6791b67b381 https://hg.mozilla.org/mozilla-central/rev/b53f72363f2c https://hg.mozilla.org/mozilla-central/rev/16d73a72ac80 https://hg.mozilla.org/mozilla-central/rev/41cd9b721602 https://hg.mozilla.org/mozilla-central/rev/3542e1112df0 https://hg.mozilla.org/mozilla-central/rev/b4e466a6f388 https://hg.mozilla.org/mozilla-central/rev/999d34774ff6 https://hg.mozilla.org/mozilla-central/rev/99b306c69791

bz: I don't see any mention of nsILoadInfo in the websockets code. Was that an oversight? (WebsocketChannels aren't nsIChannels, mostly because they map poorly onto the single request/response model that nsIChannel assumes).

and... we already covered this in comment 62--sounds like we want a followup?

Yes, please!