Copying over comment from bz from Bug 1303943 comment 11: > ...in a followup bug I think we can check whether the > cases in Reload() that end up doing the "triggeringPrincipal = nullptr, > principalToInherit = nullptr, referrer = mReferrerURI" ever get hit in > practice and if not whether we can just remove them. If we can, then looks > to me like we should be able to change LoadErrorPage to use a system > triggering principal explicitly, hoist the logic that creates a triggering > principal from referrer up to LoadURI (since it will be the only thing that > can pass an interesting referrer but a null triggering principal), do the > "create from referer" thing before our current computation of > principalToInherit, and if there is no referrer, explicitly set > triggeringPrincipal to system _before_ doing the principalToInherit > computation. > > Basically, try to trim down this forest of different paths and cases to be > smaller and saner... because what we have now is completely impossible to > reason about, which is bad for security code.

Seems to do what we want testing locally, let's see if TRY feels the same way: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42750dfa8a74b7164552825dad461e773847fe26

Boris, your approach almost works, there are only 2 things (as far as I can tell) that don't work correctly: 1) In LoadHistoryEntry() we might end up with a nullptr triggeringPrincipal and a nullptr principalToInherit. E.g. when loading 'about:config' from within LoadHistoryEntry() within test browser/components/sessionstore/test/browser_393716.js. I assume we also should fall back to use the referrer and worst case the systemPrincipal as triggeringPrincipal and principalToInherit within LoadHistoryEntry(), right? (That's what I did in the attached patch and it seems to work and mirrors old behavior). 2) Test sessionstore/test/browser_393716.js which tries to test that a refresh to data: URIs do not inherit the principal. I don't know what the best way to fix that issue is, any suggestions? Probably special case within ::Reload(), but not sure.

> E.g. when loading 'about:config' from within LoadHistoryEntry() within test browser/components/sessionstore/test/browser_393716.js This test is manually creating a broken session restore entry (one without principals) and then restoring from it via creating an shentry, right? I think we should either disallow loads from such entries (and fix the test accordingly, but would need to check whether session restore can create such entries in normal operation too) or explicitly set the triggering principal to system and principal to inherit to a nullprincipal if a load happens via LoadHistoryEntry and the entry doesn't have principals stored in it. > I assume we also should fall back to use the referrer Wouldn't help here; no referrer. In general, I think the referrer fallback is broken by design and we should not add new instances of it. But I guess falling back to referrer may be safer than falling back to system for the triggering principal... I could be convinced either way for the triggering principal. > and worst case the systemPrincipal as triggeringPrincipal and principalToInherit within LoadHistoryEntry() For triggering, fine. We should not use systemPrincipal for principalToInherit unless for some reason forced to by something; I see nothing that would force us to do it in this case. I would really prefer we used a nullprincipal for principal to inherit here. > Test sessionstore/test/browser_393716.js which tries to test that a refresh > to data: URIs do not inherit the principal. Is that the right test url? Which codepaths does whatever this test really is end up taking?

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3) > I would really prefer we used a nullprincipal for principal to inherit here. I agree actually (should have done that in the first place). Updated LoadHistoryEntry() to use the systemPrincipal as the triggeringPrincipal and a freshly created NullPrincipal for principalToInherit in case both principals are nullptrs when we start the load. Should work everywhere, definitely works for browser/components/sessionstore/test/browser_393716.js. > Is that the right test url? Which codepaths does whatever this test really > is end up taking? That was not the right test. The test that is still failing is docshell/test/test_bug475636.html. Although I don't know why. > TEST-UNEXPECTED-FAIL | docshell/test/test_bug475636.html | undefined assertion name - got "Able to access private: 42", expected "pass" In the old world we end up with the following principals when loading the data: URL for the test [1] before creating the new loadInfo in docshell: * triggeringPrincipal: http://mochi.test:8888/tests/docshell/test/test_bug475636.html * aPrincipalToInherit: nullptr In the new world we end up with: * triggeringPrincipal: http://mochi.test:8888/tests/docshell/test/test_bug475636.html * principalToInherit: http://mochi.test:8888/tests/docshell/test/test_bug475636.html which should technically be the same since internally we fall back to using the triggeringPrincipal in case the principalToInherit is a nullptr (or not explicitly overwritten the principalToInherit if null). The test does not trigger code within ::Reload, nor ::LoadHistoryEntry, the test basically just uses the normal Path of LoadURI -> Internalload(). At the moment I don't know what might cause that difference! If you have any suggestions what I could try to figure out what's going on, that would be appreciated otherwise I will continue to debug the problem tomorrow - thanks! [1] data:text/html,%3C%21DOCTYPE%20HTML%3E%3Cscript%3Etry%20%7B%20%20window.parent.postMessage%28%22Able%20to%20access%20private%3A%20%22%20+%20%20%20%20window.parent.private%2C%20%22*%22%29%3B%7Dcatch%20%28e%29%20%7B%20%20window.parent.postMessage%28%22pass%22%2C%20%22*%22%29%3B%7D%3C/script%3E

Boris, it was the inherit flag that differed for test docshell/test/test_bug475636.html (see comment 4). So, I suppose we could do the following: do not fall back to creating a triggeringPrincipal (using referrer or system) before we mess with the principalToInherit, but rather after. Otherwise, for example, we would never even enter that block [1] anymore. Also, I think if we do the triggeringPrincipal after that block we keep parity to our current implementation. The only thing we have to do now is to relax the assertion within InternalLoad() since we now mess with the triggeringPrincipal before we call InternalLoad(). When running docshell/test/test_bug475636.html we now call InternalLoad with * triggeringPrincipal: http://mochi.test:8888/tests/docshell/test/test_bug475636.html * principalToInherit: nullptr which would trigger the assertion. I suppose we could just add that LOAD_REFRESH special case to the assertion as I did in the patch? [1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1497

Boris, if you agree to my proposal and patch within Comment 5 then I was wondering if you could also review the patch - thanks!

Tanvi, I would really like to keep the assertion > MOZ_ASSERT(aContentPolicyType != nsIContentPolicy::TYPE_SUBDOCUMENT || > triggeringPrincipal, "Need a valid triggeringPrincipal"); and just move it to LoadURI. Problem is that the contentPolicyType is not available yet, e.g. simply replacing the first part of the assertion with |!IsFrame()| is not enough since there are more things to take into consideration when determining the contentPolicyType.

Sorry for the lag here. Every time I look at this I have to re-page it all in, so it takes an hour or so of contiguous free time every time. :( Per comment 0, we want to be doing the "set the triggering principal to system if there is no referrer" business _before_ we compute the principalToInherit, because we want it to end up getting sanitized in that situation. That was the part I really wanted to ensure happens. But maybe it's not needed, see below. For the test that ended up failing we do have a referrer, right? What is the actual code flow in InternalLoad that led to the triggering principal created from that referrer _not_ being inherited by the load? Did aPrincipalToInherit remain null all the way down to nsDocShell::DoURILoad? Reading all this really carefully again, if we land in nsDocShell::DoURILoad with null aTriggeringPrincipal and null aPrincipalToInherit, we will go ahead and create a triggeringPrincipal based on referrer (falling back to system if no referrer), but we will NOT set the "inherit" boolean to true, hence will not inherit that just-created thing into any loads. That's really good, actually. But none of this behavior is documented for DoURILoad, and the docs we have for internalLoad in nsIDocShell.idl not only don't document this, but incorrectly document that the triggeringPrincipal will get inherited, which is why I thought we needed to change something here... This is all horrible. :( It may still be a good idea to hoist the "create your dummy triggering principal" bit up higher, but if we do I agree that it should _certainly_ happen after whatever we do to determine the principalToInherit in LoadURI... Looking at what InternalLoad actually does with aPrincipalToInherit, it will pass it through unless it's null _and_ INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL is set, in which case it will try to inherit it from the current doc or parent doc. Plus the code that we added in bug 1303943 to set to nullprincipal, which is clearly based on incorrect premises about what having a null principalToInherit _really_ means and should just get removed. That will at least simplify _something_ about this mess. So I think we should take a deep breath, document what the arguments to DoURILoad mean, then based on that document what the arguments to InternalLoad really mean. Then remove that one chunk added in bug 1303943 because as far as I can tell it's a no-op. At that point the aPrincipalToInherit InternalLoad passes to DoURILoad will no longer depend on the value of aTriggeringPrincipal passed to InternalLoad, so if we want to we could hoist the creation-from-referrer up to LoadURI, as long as we put it where your last patch does: after we've determined what aPrincipalToInherit we plan to pass to InternalLoad. Or at least maybe we can. There are still at least two open questions: 1) Why does LoadErrorPage pass INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL? The fact that it does means that the behavior of InternalLoad as called from that function _does_ depend on whether aPrincipalToInherit is null or not. The change to pass it as system principal in your patch may not be right, therefore. 2) Can that !mOSHE && !mLSHE codepath in nsDocShell::Reload actually get hit? The hoisting of the "create from referrer" bit to LoadURI only really makes sense, in my opinion, if that codepath can't get hit, in which case we should remove it... I'm really sorry this stuff is so complicated and messed up. :( I'll try to keep it paged in in my head long enough for us to get this sorted out; certainly through tomorrow.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9) > Sorry for the lag here. Every time I look at this I have to re-page it all > in, so it takes an hour or so of contiguous free time every time. :( That's definitely a reason why we should try to simplify that part of the code. > For the test that ended up failing we do have a referrer, right? Sorry I was not precise the first time around. For the failing test within docshell/test/test_bug475636.html we do have a referrer, it's * http://mochi.test:8888/tests/docshell/test/test_bug475636.html > What is the actual code flow in InternalLoad that led to the triggering principal > created from that referrer _not_ being inherited by the load? In the old world aPrincipalToInherit was a nullptr and hence we didn't enter that code within LoadURI: if (aPrincipalToInherit) { inherit = nsContentUtils::ChannelShouldInheritPrincipal( aPrincipalToInherit, aURI, true, // aInheritForAboutBlank isSrcdoc); } see, https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10835 but in the new world (since we do the fallback of triggeringPrincipal *before* we determine the principalToInherit), the principalToInherit is already set by nsCOMPtr<nsIPrincipal> principalToInherit = triggeringPrincipal; here: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1476 > Did aPrincipalToInherit remain null all the way down to nsDocShell::DoURILoad? Yes exactly, aPrincipalToInherit remained a nullptr all the way to the loadInfo creation. > Reading all this really carefully again, if we land in nsDocShell::DoURILoad > with null aTriggeringPrincipal and null aPrincipalToInherit, we will go > ahead and create a triggeringPrincipal based on referrer (falling back to > system if no referrer), but we will NOT set the "inherit" boolean to true, > hence will not inherit that just-created thing into any loads. That's > really good, actually. But none of this behavior is documented for > DoURILoad, and the docs we have for internalLoad in nsIDocShell.idl not only > don't document this, but incorrectly document that the triggeringPrincipal > will get inherited, which is why I thought we needed to change something > here... This is all horrible. :( Ok, so this we certainly need to fix up. One step at a time though. Let's first check if all of our assumptions within this patch are correct now. In other words, let's wait to get TRY results. > It may still be a good idea to hoist the "create your dummy triggering > principal" bit up higher, but if we do I agree that it should _certainly_ > happen after whatever we do to determine the principalToInherit in LoadURI... Ok, this is what the current patch is doing. > Looking at what InternalLoad actually does with aPrincipalToInherit, it will > pass it through unless it's null _and_ INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL > is set, in which case it will try to inherit it from the current doc or > parent doc. Plus the code that we added in bug 1303943 to set to > nullprincipal, which is clearly based on incorrect premises about what > having a null principalToInherit _really_ means and should just get removed. > That will at least simplify _something_ about this mess. Ok, I reverted those changes within this patch that was before Bug 1303943 landed. > So I think we should take a deep breath, document what the arguments to > DoURILoad mean, then based on that document what the arguments to > InternalLoad really mean. Then remove that one chunk added in bug 1303943 > because as far as I can tell it's a no-op. At that point the > aPrincipalToInherit InternalLoad passes to DoURILoad will no longer depend > on the value of aTriggeringPrincipal passed to InternalLoad, so if we want > to we could hoist the creation-from-referrer up to LoadURI, as long as we > put it where your last patch does: after we've determined what > aPrincipalToInherit we plan to pass to InternalLoad. Or at least maybe we > can. Yes, I think I like that approach. Let's first determine the principalToInherit and then do the fallback for triggeringPrincipal within LoadURI. > There are still at least two open questions: > > 1) Why does LoadErrorPage pass INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL? The > fact that it does means that the behavior of InternalLoad as called from > that function _does_ depend on whether aPrincipalToInherit is null or not. > The change to pass it as system principal in your patch may not be right, > therefore. I can't tell to be honest. Predates my time and looking through blame I also couldn't find any evidence. What if we change the load flags to LOAD_FLAGS_NONE? Or alternatively set the triggeringPrincipal to System and the principalToInherit to a freshly created NullPrcinipal? > 2) Can that !mOSHE && !mLSHE codepath in nsDocShell::Reload actually get > hit? The hoisting of the "create from referrer" bit to LoadURI only really > makes sense, in my opinion, if that codepath can't get hit, in which case we > should remove it... Sorry, I can't completely follow, but I put an assertion that we never enter that branch and we take it from there once we know if that branch ever gets hit. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea7ebede0e9106b4d0ec2b03e8eb207ac8ecfb0c

> Predates my time and looking through blame I also couldn't find any evidence. OK. So looking at blame, the relevant changes seem to be these: 1) http://searchfox.org/mozilla-central/diff/0b952f22795ee344e4c5f56267124349395c00da/docshell/base/nsDocShell.cpp#3337 which has no associated bug but was just a "keep the behavior but fix the type being passed" change: PR_TRUE == 1 and INTERNAL_LOAD_FLAGS_INHERIT_OWNER == 1, but the callee expected flags, not a boolean. 2) The changes for bug 157004 which first started calling InternalLoad in LoadErrorPage. I can't tell what the PR_TRUE there was supposed to be; at the time InternalLoad definitely had that argument as a flag word, not a boolean. So looks to me like it was just a mistake all along. We should change this to LOAD_FLAGS_NONE and see whether anything breaks.

> We should change this to LOAD_FLAGS_NONE Er, INTERNAL_LOAD_FLAGS_NONE.

> Can that !mOSHE && !mLSHE codepath in nsDocShell::Reload actually get hit? I thought about this some more, and I believe that the general "no mOSHE and no mLSHE" can get hit as follows: 1) Start a load in the docshell. 2) Do a reload() call before the server response has been received. But the "bad" case that passes null principals also involves GetDocument() returning null, right? I'm not sure whether _that_ case is really reachable sanely. Specifically, GetDocument calls EnsureContentViewer(), which will create a document, unless we hit OOM or are in the middle of docshell destruction. So I think it would be reasonable to just change Reload to no-op if we have no mOSHE, no mLSHE, and GetDocument() returns null, so we don't have to worry about that case happening.

Boris, I incorporated your suggestions in this changeset. In particular: a) LoadErrorPage() uses a SystemPrincipal as the TriggeringPrincipal and uses INTERNAL_LOAD_FLAGS_NONE instead of INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL which seems that it has been wrong for quite some time. b) Reload() turns into a NO-OP if there is no mOSHE, no mLSHE and no doc. In case we end up in the (!mOSHE && !mLSHE) case, we use doc->NodePrincipal() as the triggeringPrincipal. Given the latest TRY run from comment 10, which *basically* already contained all of the changes, I suppose the only thing left is to update the documentation, right? https://treeherder.mozilla.org/#/jobs?repo=try&revision=baec0e41249b5e91812f10a1134840a924e8163b

Comment on attachment 8803716 [details] [diff] [review] bug_1308889_explicitly_pass_triggering_to_douriload.patch >+ // If there is no triggeringPrincipal, we should try to create one from Please very clearly document the ordering dependency with the principalToInherit bits above this and why the order is what it is. >@@ -5365,19 +5378,22 @@ nsDocShell::LoadErrorPage(nsIURI* aURI, >+ triggeringPrincipal, triggeringPrincipal, I think you should pass null for aPrincipalToInherit here. Note that it matters that much in practice, since the URI being loaded never inherits. I guess that means that the assert that aPrincipalToInherit is not null when aTriggeringPrincipal is not null at the top of InternalLoad should also just be scrapped: a null argument there means "do not inherit", which is a sane thing to want, right? I guess it doesn't when INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL is set, but we're exempting that case from the assert already anyway. >@@ -5410,52 +5426,55 @@ nsDocShell::Reload(uint32_t aReloadFlags >+ nsCOMPtr<nsIPrincipal> triggeringPrincipal = doc->NodePrincipal(); This doesn't need to be a strong ref. >@@ -12534,16 +12524,28 @@ nsDocShell::LoadHistoryEntry(nsISHEntry* This code needs a comment explaining why it's doing what it's doing. Still need the doc updates.

Boris, sorry for the lag of updates within this patch. I was out at a conference and now it also takes me hours to page that stuff back in :-( I think I updated and incorporated your suggestions and this patch should be ready for review. One thing I can't wrap my head around. In the new world we always should have a non-null triggeringPrincipal passed to InternalLoad(), right? Shouldn't we assert that we have a non-null triggeringPrincipal and also remove: MOZ_ASSERT(aTriggeringPrincipal || (!aPrincipalToInherit || aPrincipalToInherit->GetIsNullPrincipal())); I tried to add comments to the best of my docshell knowledge, but it's still possible that we are missing some parts that would also require documentation. In particular, are docs sufficient for internalLoad within nsIDocshell.idl? If not, I would appreciate your help with that.

Comment on attachment 8805907 [details] [diff] [review] bug_1308889_explicitly_pass_triggering_to_douriload.patch >@@ -1500,16 +1500,32 @@ nsDocShell::LoadURI(nsIURI* aURI, This change still needs the comments I asked for in comment 15. > In the new world we always should have a non-null triggeringPrincipal passed to InternalLoad(), right? Hmm. I think that's correct, yes. So yes, just assert it's not null. >@@ -12501,16 +12486,29 @@ nsDocShell::LoadHistoryEntry(nsISHEntry* Why not just pass a null pointer for principalToInherit here, since we're not passing INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL, so null means "don't inherit anything"? Put another way, I don't think you need that "set principalToInherit to a nsNullPrincipal" block. >@@ -127,45 +127,33 @@ interface nsIDocShell : nsIDocShellTreeI The docs here are NOT sufficient. What needs to be documented is: 1) aTriggeringPrincipal must not be null. 2) The behavior when aPrincipalToInherit when null is passed; see comment 9. The docs here do not correctly describe it! The handling of null aPrincipalToInherit in internalLoad is as follows, as far as I can tell: 1) If aFlags includes INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL, and aLoadType is not LOAD_NORMAL_EXTERNAL, and the URI would normally inherit a principal, then principalToInherit is set to the current document's principal, or parent document if there is not a current document. 2) If principalToInherit is still null (e.g. if some of the conditions of #1 were not satisfied), then no inheritance of any sort will happen: the load will just get a principal based on the URI being loaded. Now that I write this down... is it not the case that LoadURI is the _only_ place that passes INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL? Can we push that entire computation that's conditioned on that flag out of InternalLoad to LoadURI, and just have InternalLoad have simple semantics: If aPrincipalToInherit is non-null and the URL wants to inherit a principal, it will inherit it; if either of those is not the case, there will be no inheritance. Then we can kill INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL altogether. Followup probably OK for this change, if we want to reduce risk. The arguments to DoURILoad also still need documenting; see comment 9 towards the end. Luckily, the handling of aPrincipalToInherit here is pretty simple; it's exactly the handling I describe above for InternalLoad if we nix INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL. In particular, we could just have to DoURILoad docs point to those docs if we do that....

Boris, thanks for the continuous constructive feedback on this bug. I addressed your suggestions and updated the following: * Updated documentation within LoadURI() * Updated docs for DoURILoad() and InternalLoad() * Updated the assertion within InternalLoad() making sure that all calls to InternalLoad() pass a non-null triggeringPrincipal. * Removed the code block where we were creating a NullPrincipal within LoadHistoryEntry * I filed a follow up (Bug 1314234) that moves logic of INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL to LoadURI() and removes that flag. This bug is already complicated by itself and we should land this bug before the merge, because InternalLoad() triggers some assertions (e.g. Bug 1310645, or also Bug 1313456). Hopefully I a did not miss anything and we can land this bug soonish - thanks!

Comment on attachment 8806267 [details] [diff] [review] bug_1308889_explicitly_pass_triggering_to_douriload.patch >+++ b/docshell/base/nsIDocShell.idl >@@ -127,45 +127,37 @@ interface nsIDocShell : nsIDocShellTreeI > * @param aPrincipalToInherit - Principal to be inherited for that load. If > * passing null for this argument, then internally > * the triggeringPrincipal is also used for the >- * principalToInherit. No. That's not what happens. The comments for nsDocShell::DoURILoad have the same problem.

Comment on attachment 8806267 [details] [diff] [review] bug_1308889_explicitly_pass_triggering_to_douriload.patch >@@ -12502,16 +12485,24 @@ nsDocShell::LoadHistoryEntry(nsISHEntry* > if (isSrcdoc) { > aEntry->GetSrcdocData(srcdoc); > aEntry->GetBaseURI(getter_AddRefs(baseURI)); > flags |= INTERNAL_LOAD_FLAGS_IS_SRCDOC; > } else { > srcdoc = NullString(); > } > >+ // If there is no triggeringPrincipal we can fall back to using the >+ // SystemPrincipal as the triggeringPrincipal for loading the history >+ // entry, since the history entry can only end up in history if security >+ // checks passed in the initial loading phase. >+ if (!triggeringPrincipal) { >+ triggeringPrincipal = nsContentUtils::GetSystemPrincipal(); >+ } >+ Can you update this comment to add something like: Note that if new security checks were added since the history entry was stored, we may miss them here. Can we set triggeringPrincipal to codebasePrincipal for just this case? What could that break? >@@ -10822,34 +10821,18 @@ nsDocShell::DoURILoad(nsIURI* aURI, > // to load this resource. This docshell is scheduled for destruction > // already, so bail out here. > return NS_OK; > } > } > > // Getting the right triggeringPrincipal needs to be updated and is only > // ready for use once bug 1182569 landed. Until then, we cannot rely on >- // the triggeringPrincipal for TYPE_DOCUMENT loads. Please note that the >- // triggeringPrincipal falls back to the systemPrincipal below. >- nsCOMPtr<nsIPrincipal> triggeringPrincipal = aTriggeringPrincipal; >- >- // Make sure that we always get a non null triggeringPrincipal for >- // loads of type TYPE_SUBDOCUMENT. >- MOZ_ASSERT(aContentPolicyType != nsIContentPolicy::TYPE_SUBDOCUMENT || >- triggeringPrincipal, "Need a valid triggeringPrincipal"); >- We discussed how the triggeringPrincipal assert for TYPE_SUBDOCUMENT wouldn't work with these new code changes (since we wouldn't know if we fell back to system for TriggeringPrincipal). We discussed putting in a boolean member variable when we fallback and checking that here instead. Did you have a chance to do this? >- if (!triggeringPrincipal) { >- if (aReferrerURI) { >- rv = CreatePrincipalFromReferrer(aReferrerURI, >- getter_AddRefs(triggeringPrincipal)); >- NS_ENSURE_SUCCESS(rv, rv); >- } else { >- triggeringPrincipal = nsContentUtils::GetSystemPrincipal(); >- } >- } >+ // the triggeringPrincipal for TYPE_DOCUMENT loads. >+ MOZ_ASSERT(aTriggeringPrincipal, "Need a valid triggeringPrincipal"); > > bool isSandBoxed = mSandboxFlags & SANDBOXED_ORIGIN; > // only inherit if we have a aPrincipalToInherit > bool inherit = false; > > if (aPrincipalToInherit) { > inherit = nsContentUtils::ChannelShouldInheritPrincipal( > aPrincipalToInherit,

(In reply to Tanvi Vyas [:tanvi] from comment #21) > Can we set triggeringPrincipal to codebasePrincipal for just this case? > What could that break? At this point I don't know exactly what would break and unfortunately I don't have the cycles to investigate right now. Please file a follow up since I would really like to get this bug landed within this release cycle. > We discussed how the triggeringPrincipal assert for TYPE_SUBDOCUMENT > wouldn't work with these new code changes (since we wouldn't know if we fell > back to system for TriggeringPrincipal). We discussed putting in a boolean > member variable when we fallback and checking that here instead. Did you > have a chance to do this? Unfortunately we can't reliably distinguish between TYPE_DOCUMENT and TYPE_SUBDOCUMENT loads within LoadURI() because the distinction does not simply rely on isFrame(). I don't think polluting docshell with an additional boolean would help either. Please file a follow up and we can think of bringing in an assertion before we do the principal fallback within LoadURI() to make sure TYPE_SUBDOCUMENT loads *always* pass a non-null principal.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #20) > > * @param aPrincipalToInherit - Principal to be inherited for that load. If > > * passing null for this argument, then internally > > * the triggeringPrincipal is also used for the > >- * principalToInherit. > > No. That's not what happens. Boris, I agree that the wording is wrong, but conceptually the argument is true, right? If the principalToInherit is null, then the resulting channels' loadInfo returns the triggeringPrincipal when querying the principalToInherit. Or am I misinterpreting something?

[Tracking Requested - why for this release]: Bad security bug in newly introduced code; see below. > If the principalToInherit is null, then the resulting channels' loadInfo returns > the triggeringPrincipal when querying the principalToInherit. That's true in a narrow sense from the loadinfo's point of view, but deeply misleading in terms of the actual load behavior that docshell cares about, because when principalToInherit is null we do not set any of the load flags that would cause the PrincipalToInherit() in the loadinfo to actually be _used_. Specifically, if principalToInherit is null, then in DoURILoad "inherit" ends up false, and hence we never add SEC_FORCE_INHERIT_PRINCIPAL to the securityFlags. Docshell loads also never set any of the *_INHERITS flags at the moment. So as long as no one forces principal inheritance on the loadinfo via SetForceInheritPrincipalOverruleOwner the loadinfo's PrincipalToInherit will not be used in GetChannelResultPrincipal. So passing null for principalToInherit to InternalLoad/DoURILoad has _very_ different behavior from passing triggeringPrincipal for principalToInherit, while the comment makes it sound like the behavior would be the same the same. And now that I talk through this, I see we have horrible security bug that we have introduced due to the mistaken impresion this very comment produces! If nsDocShell::AddToSessionHistory is called with a non-null aChannel (read: "when we load a new page"), and that channel was initially created with a null principalToInherit, the session history entry will end up with a non-null principalToInherit (one pointer-identical to the triggering principal). If that session history entry is then loaded, and the URL is something that inherits principals, then it will now inherit the triggering principal, whereas it did not during the original load. This is extremely bad. So we need to either change loadinfo to keep returning a null principalToInherit if it was initialized with null (and change GetChannelResultPrincipal to use the triggering principal in the force-inherit cases when there is no principalToInherit) or we need to change AddToSessionHistory to check whatever combination of flags can cause GetChannelResultPrincipal to care about the principalToInherit and only get the principalToInherit in that case. I would much prefer the former approach, because it minimizes the risk of someone screwing things up and the codepaths diverging. Maybe what we really want is a method that returns the thing passed to the loadinfo at construction time and another method that returns what the other one does, or something. But either way, AddToSessionHistory needs to be able to recover values that are appropriate to be passed to InternalLoad, and right now it's not doing that, and that's bad. And either way, we need to fix the InternalLoad/DoURILoad comments to clearly spell out the behavior, so we stop confusing ourselves over this. If you want to spin off the AddToSessionHistory fix into a separate bug, one that we ensure gets landed in time for 52, that's fine with me. Whatever is easiest for you here... If you do that, please move the tracking nomination and "blocking bug 1297338" annotation there.

Tracking 52+ for the reasoning in Comment 24.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #24) > different behavior from passing triggeringPrincipal for principalToInherit, > while the comment makes it sound like the behavior would be the same the > same. That makes sense. I updated documentation for DoURILoad() as well as internalLoad() to now hopefully reflect that behavior correctly. > If that session history entry is then loaded, and the URL is > something that inherits principals, then it will now inherit the triggering > principal, whereas it did not during the original load. This is extremely > bad. Indeed, that is bad - good catch. > So we need to either change loadinfo to keep returning a null > principalToInherit if it was initialized with null (and change > GetChannelResultPrincipal to use the triggering principal in the > force-inherit cases when there is no principalToInherit) Returning the actual principalToInherit without falling back to the triggeringPrincipal sounds like the best way forward to me (at least for now). I updated GetChannelResultPrincipal accordingly. Further, I had to update BackgroundUtils as well as nsJSProtocollhandler to actually account for a *null* principalToInherit. I think there is nothing else that relies on the principalToInherit, at least I didn’t find anything when looking through the initial codes changes when introducing principalToInherit: https://hg.mozilla.org/mozilla-central/rev/dee3d68b0fc8 I think we should be able to land this bug in time before the merge next week, so let’s keep everything together within this bug.

Comment on attachment 8807297 [details] [diff] [review] bug_1308889_explicitly_pass_triggering_to_douriload.patch >+++ b/caps/nsScriptSecurityManager.cpp >@@ -372,29 +372,36 @@ nsScriptSecurityManager::GetChannelResul Ehat about the thidrd caller of PrincipalToInherit(), in the loadInfo->GetForceInheritPrincipalOverruleOwner() case? That needs to be fixed too, right? Also, the getter should be called GetPrincipalToInherit(), since it can now return null. >+++ b/docshell/base/nsDocShell.h >+ // Consider the following case: the system might initiate a load for >+ // 'about:blank', hence SystemPrincipal is passed for aTriggeringPrincipal. >+ // But the principal to be inherited for that load should be a NullPrincipal >+ // and not the SystemPrincipal. In that case, please explicitly pass that >+ // nullPrincipal as the principalToInherit. This comment is not making sense to me, now that we have clearly documented that null principalToInherit means "no inheritance". In fact, passign a nullprincipal will give the about:blank a nullprincipal, while passing null will give it a codebase principal for "about:blank". The latter is a lot more likely to be wanted in the about:blank case. Please either just remove those 6 lines of comment or change them to match reality better. >+++ b/docshell/base/nsIDocShell.idl > * @param aPrincipalToInherit - Principal to be inherited for that load. If >+ * passing null for this argument, then no >+ * inheritance of any sort will happen and the >+ * load will get a principal based on the URI >+ * being loaded. That last bit isn't true for this argument until bug 1314234 is fixed. Until that happens, passing null means this function will look at INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL, etc. Please just remove the entire second sentence in the bit I quoted here, because it's wrong. > If this argument is non-null >+ * then aPrincipalToInherit is computed as follows: No, this computation is what happens if the argument is _null_.

> Ehat about the thidrd caller of PrincipalToInherit() I meant "What about the third caller of PrincipalToInherit()?". This is clearly the wrong time of day for me to be typing. :(

Oh, and nsILoadInfo.idl's documentation of principalToInherit needs to be fixed too. For example, this bit: > For loads that are not TYPE_DOCUMENT or TYPE_SUBDOCUMENT that > * principal is always identical to the triggeringPrincipal. is just false; for non-document loads it will always be null. We need to document that it _can_ be null, and that if it's null then the triggeringPrincipal is what should get inherited if inheritance is happening.

Boris, thanks for your help, within this patch I updated: * Documentation for principalToInherit within nsILoadInfo * Fixed the missing case of principalToInherit for GetForceInheritPrincipalOverruleOwner() within GetChannelResultPrincipal * Further, I filed Bug 1315195 to replace PrincipalToInherit() with GetPrincipalToInherit(). Reason I filed a follow up is because we are facing the same problem for LoadingPrincipal() which can also return null. * Finally, I am still unsure about the documentation within nsIDocshell.h. I think I followed your suggestions, but I am also not entirely sure if that is what you were asking for. But now since we have updated that, we are facing failing tests, e.g. docshell/test/test_bug540462.html, where in the old world the loadinfo looks like: loadinfo { channelURI: wyciwyg://0/http://mochi.test:8888/tests/docshell/test/file_bug540462.html loadingPrincipal: nullptr triggeringPrincipal: http://mochi.test:8888/tests/docshell/test/file_bug540462.html principalToInherit: http://mochi.test:8888/tests/docshell/test/file_bug540462.html contentPolicyType: 6 securityMode: SEC_FORCE_INHERIT_PRINCIPAL, initalSecurityChecksDone: no enforceSecurity: no } vs in the new world the loadinfo looks like: doContentSecurityCheck { channelURI: wyciwyg://0/http://mochi.test:8888/tests/docshell/test/file_bug540462.html loadingPrincipal: nullptr triggeringPrincipal: http://mochi.test:8888/tests/docshell/test/file_bug540462.html principalToInherit: nullptr contentPolicyType: 6 securityMode: initalSecurityChecksDone: no enforceSecurity: no } Please note we end up with a *null* principalToInherit within AddToSessionHistory() which then causes us to not set the principalToInherit flag (SEC_FORCE_INHERIT_PRINCIPAL) and hence GetChannelResultPrincipal falls back to using the GetChannelURIPrincipal which then causes the following error: JavaScript error: wyciwyg://0/http://mochi.test:8888/tests/docshell/test/file_bug540462.html, line 1: Error: Permission denied to access property "documentWriteLoad" At this point I don't know exactly what we need to do, maybe you can propose a path forward.

Comment on attachment 8807675 [details] [diff] [review] bug_1308889_explicitly_pass_triggering_to_douriload.patch As discussed on IRC, the test failures are because wyciwyg channels get created outside docshell, but then can be loaded through session history. That means they need to have principalToInherit set on them at the place where they're created, now that we no longer default it to the triggeringPrincipal. Also as discussed on IRC, nsObjectLoadingContent will need a similar change, setting principalToInherit on the loadinfo only in the case when it's doing inheritance, because that can later become a document load. This also means the documenation for nsILoadInfo's principalToInherit is not _quite_ right because the nsObjectLoadingContent load may not be TYPE_DOCUMENT or TYPE_SUBDOCUMENT but will have a non-null principalToInherit. Let's gloss over that for now; see below. Comments on the patch itself: 1) It's not clear to me what "if the inherit flag" means in nsILoadInfo. There are several different "inherit" flags there.... Probably better to say something like "if a principal needs to be inherited". 2) The comments in nsIDocShell.idl look good, with one nit: replace "then aPrincipalToInherit is computed" with "then principalToInherit is computed". Comments on future direction: As we just discussed on IRC, this is all super-fragile. The fundamental problem is that session history needs to recover all the information it needs to properly redo the load. It needs to encode that information in (triggeringPrincipal, principalToInherit, uri, originalURI) and then we have to somehow rederive the original security flags in docshell code. That's all dandy if the security flags came from that docshell code itself, via InternalLoad(), but for the wyciwyg and nsObjectLoadingContent cases they don't. So we should have a followup to make this better, maybe along lines somewhat like this: 1) Store the actual security flags from the loadinfo in the shentry. This is a bit of a pain because we need to fix sessionstore to store them and whatnot, and make it all safe. This part is my main concern with this plan. 2) Propagate those security flags down to DoURILoad. 3) When security flags are passed to DoURILoad, don't try to guess at whether to inherit based on the other arguments; just use the given security flags. 4) Once all that is done, we can go back to having PrincipalToInherit() on loadinfo return the triggering principal as a fallback, because we will never inherit when we shouldn't. That will simplify the documentation for nsILoadInfo and we will be able to remove the code we're about to add for nsObjectLoadingContent/wyciwyg... Given item 1 there, maybe we can come up with a better solution? Anyway, followup for sure.

Boris, as discussed over IRC, I incorporated those suggestions also setting the PrincipalToInherit explicitly after channel creation within nsHTMLDocument as well as nsObjectLoadingContent. Further, I incorporated your nits about the documentation. Let's discuss next steps on Tuesday in our meeting - thanks!

Comment on attachment 8807695 [details] [diff] [review] bug_1308889_explicitly_pass_triggering_to_douriload.patch r=me

Boris, we forgot one thing. Since the principalToInherit within the loadInfo can be null now, we also have to implement the explicit fallback within amContentHandler.js. See the initial introduction: https://hg.mozilla.org/mozilla-central/rev/a3056e9ae41a I will fold that patch into the other one before landing.

> I will fold that patch into the other one before landing. Sounds like a plan. Good catch.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/57d473c3ca22 Try to explicitly pass aTriggeringPrincipal and aPrincipalToInherit to DoURILoad(). r=bz