Testcase generated from fuzzing a debug build of mozilla-central v20170222-7abeac2f2d66. Assertion failure: aTriggeringPrincipal (need a valid triggeringPrincipal to create a session history entry), at /home/worker/workspace/build/src/docshell/shistory/nsSHEntry.cpp:420 ASAN:DEADLYSIGNAL ================================================================= ==4045==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f0b445bab37 bp 0x7ffc92f99360 sp 0x7ffc92f99330 T0) #0 0x7f0b445bab36 in nsSHEntry::Create(nsIURI*, nsAString_internal const&, nsIInputStream*, nsILayoutHistoryState*, nsISupports*, nsACString_internal const&, nsIPrincipal*, nsIPrincipal*, nsID const&, bool) /home/worker/workspace/build/src/docshell/shistory/nsSHEntry.cpp:419:3 #1 0x7f0b44573fe7 in nsDocShell::AddToSessionHistory(nsIURI*, nsIChannel*, nsIPrincipal*, nsIPrincipal*, bool, nsISHEntry**) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:12421:3 #2 0x7f0b445810fb in nsDocShell::OnNewURI(nsIURI*, nsIChannel*, nsIPrincipal*, nsIPrincipal*, unsigned int, bool, bool, bool) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:11822:13 #3 0x7f0b44544de4 in nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, nsAString_internal const&, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, bool, nsIDocShell**, nsIRequest**) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:10426:7 #4 0x7f0b4453fbfd in nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:1564:10 #5 0x7f0b3f359755 in mozilla::dom::Location::SetURI(nsIURI*, bool) /home/worker/workspace/build/src/dom/base/Location.cpp:288:12 #6 0x7f0b3f35a0c9 in mozilla::dom::Location::SetHash(nsAString_internal const&) /home/worker/workspace/build/src/dom/base/Location.cpp:367:10 #7 0x7f0b3f945759 in mozilla::dom::Location::SetHash(nsAString_internal const&, nsIPrincipal&, mozilla::ErrorResult&) /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Location.h:209:14 #8 0x7f0b3f945587 in mozilla::dom::LocationBinding::set_hash(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Location*, JSJitSetterCallArgs) /home/worker/workspace/build/src/obj-firefox/dom/bindings/LocationBinding.cpp:703:3 #9 0x7f0b40b376cb in mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2919:8 #10 0x7f0b450d021a in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /home/worker/workspace/build/src/js/src/jscntxtinlines.h:281:15 #11 0x7f0b450cfc20 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:463:16 #12 0x7f0b450d0bae in InternalCall(JSContext*, js::AnyInvokeArgs const&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:508:12 #13 0x7f0b450d0dd1 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:527:10 #14 0x7f0b450d2310 in js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:654:12 #15 0x7f0b45b04cec in js::SetPropertyIgnoringNamedGetter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyDescriptor>, JS::ObjectOpResult&) /home/worker/workspace/build/src/js/src/proxy/BaseProxyHandler.cpp:245:10

Maybe Samael can take a look here.

Oh, Andrew McCreight noticed that Christoph touched this code recently. Maybe he has thoughts?

Could this be come grom bug 1307736? See also bug 1341589.

Most likely a dup of bug 1341589, but I will verify. Please note that it's not a problem in production code since we fall back to using the SystemPrincipal for history loads in case none is passed explicitly (See Bug 1337622). Nevertheless, we should fix that.

Smaug, I guess I need a little bit of guidance for fixing this bug. It seems there are several small issues that lead to a bigger issue. Let me summarize: 1) Location::SetURI() should provide a valid triggeringPrincipal on the loadInfo. I suppose we can query the correct triggeringPrincipal by using the same approach as we do within Location::CheckURL, right? 2) Within nsDocshell.cpp we currently call OnNewURI() with a null triggeringPrincipal because there is no mOSHE when running the attached testcase. In turn OnNewURI() internally calls AddToSessionHistory with a null triggeringPrincipal which causes the assertion to be triggered. I am not certain if we should fall back to using aTriggeringPrincipal in case we can't query one from mOSHE. What do you think? 3) When running the attached test we currently call ShouldAddToSessionHistory() with a url of 'about:blank#a'. The comment within ShouldAddToSessionHistory() mentions that we should not add about:blank to session history at all. I think that is a bug and we should ignore the ref when comparing the URL, right? All in all, I think we should do 1) and 3) but I am not sure if 2) is correct or not. What's your take on this?

1) yes I think that makes sense. 2) I think so 3) based on testing at least Gecko and Blink do put about:blank#foo to session history, so at least without telemetry data we can't remove support for it. Changes to session history handling are very regression risky.

Comment on attachment 8841560 [details] [diff] [review] bug_1341754_history_triggeringprincipal.patch So without some telemetry data about about:blank#foo usage, I don't think we can change ShouldAddToSessionHistory. (FWIW, there is also specIgnoringRef which might be useful if we end up changing that after all) Nit, coding style is } else {

(In reply to Olli Pettay [:smaug] from comment #6) > 3) based on testing at least Gecko and Blink do put about:blank#foo to > session history, so at least without telemetry data we can't remove support > for it. Changes to session history handling are very regression risky. Ok, thanks. I think I don't even wanna go that route and we just keep those parts untouched, they are actually not *required* to fix that bug. What's important is (1) and (2) which fixes the problem. The only other things is an assertion within nsGlobalWindow.cpp which makes sure the url is about:blank and gets triggered by about:blank#foo. I fixed the issue inline, alternatively we could update NS_IsAboutBlank() [1] to ignore the ref. Let me know if you would prefer that! [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#2052

Very simple testcase which triggers the assertion within nsSHEntry but works with the patch applied. It's actually not super sophisticated test, but I suppose for what we want to test here it's good enough.

Comment on attachment 8841925 [details] [diff] [review] bug_1341754_test_triggeringprincipal_location_seturi.patch Could you change this a bit. Or, add another test. The other test shouldn't test the implicit about:blank but actually wait for load event for an iframe (with src="about:blank") to fire, then asynchronously change iframe location's hash to #bar and check that it is now the current .href and then history.back() and check that .href is now "about:blank". This other test just happens to test quite different case than the implicitly created about:blank which you're using. With that, r+

Comment on attachment 8841924 [details] [diff] [review] bug_1341754_triggeringprincipal_location_seturi.patch > /* This is a anchor traversal with in the same page. > * call OnNewURI() so that, this traversal will be > * recorded in session and global history. > */ >- nsCOMPtr<nsIPrincipal> triggeringPrincipal, principalToInherit; >+ nsCOMPtr<nsIPrincipal> newURItriggeringPrincipal, newURIprincipalToInherit; capital letter after URI in both cases. > if (mOSHE) { >- mOSHE->GetTriggeringPrincipal(getter_AddRefs(triggeringPrincipal)); >- mOSHE->GetPrincipalToInherit(getter_AddRefs(principalToInherit)); >+ mOSHE->GetTriggeringPrincipal(getter_AddRefs(newURItriggeringPrincipal)); >+ mOSHE->GetPrincipalToInherit(getter_AddRefs(newURIprincipalToInherit)); >+ } else { >+ newURItriggeringPrincipal = aTriggeringPrincipal; >+ newURIprincipalToInherit = principalToInherit; > } > // Pass true for aCloneSHChildren, since we're not > // changing documents here, so all of our subframes are > // still relevant to the new session history entry. > // > // It also makes OnNewURI(...) set LOCATION_CHANGE_SAME_DOCUMENT > // flag on firing onLocationChange(...). > // Anyway, aCloneSHChildren param is simply reflecting > // doShortCircuitedLoad in this scope. >- OnNewURI(aURI, nullptr, triggeringPrincipal, principalToInherit, >+ OnNewURI(aURI, nullptr, newURItriggeringPrincipal, newURIprincipalToInherit, > mLoadType, true, true, true); > > // Get the incumbent script's browsing context to set as source. > nsCOMPtr<nsPIDOMWindowInner> sourceWindow = > do_QueryInterface(mozilla::dom::GetIncumbentGlobal()); >+ nsCOMPtr<nsIPrincipal> triggeringPrincipal; > if (sourceWindow) { > loadInfo->SetSourceDocShell(sourceWindow->GetDocShell()); >+ triggeringPrincipal = sourceWindow->GetDoc()->NodePrincipal(); GetExtantDoc(), but you need to null check GetExtantDoc() (as you should null check GetDoc()) > } >+ else { And the coding style is still } else { ;) But perhaps change it to if (!triggeringPrincipal) { so that if GetExtantDoc() for some weird reason returns null, we still get some reasonable principal >+ // No document; determine triggeringPrincipal by quering the >+ // subjectPrincipal, wich is the principal of the current JS which Hmm, but why do we need the code in Location::SetURI? CheckURL should set the triggeringPrincipal already. It calls loadInfo->SetTriggeringPrincipal(triggeringPrincipal); What am I missing here?

(In reply to Olli Pettay [:smaug] (review request overload, closing the queue for a day > The other test shouldn't test the implicit about:blank but actually wait for > load event for an iframe (with src="about:blank") to fire, then > asynchronously change iframe location's hash to #bar and check that it is > now the current .href and then history.back() and check that .href is now > "about:blank". I added the test you requested in addition, but location.hash does not trigger a new frame load, hence we also can't call history.back(). I simulated that behavior by 'manually' setting .src = "about:blank". Carrying over r+.

(In reply to Olli Pettay [:smaug] (review request overload, closing the queue for a day or two) > Hmm, but why do we need the code in Location::SetURI? CheckURL should set > the triggeringPrincipal already. It calls > loadInfo->SetTriggeringPrincipal(triggeringPrincipal); > What am I missing here? Good catch. You are nothing missing, you are right. That was simply my programming mistake because I started debugging and first added the changes within ::SetURI() and updated docShell afterwards not realizing that the changes within ::SetURI() are superfluous. [Setting ni? but actually requesting r? - whenever you get a chance, no rush].

Comment on attachment 8842522 [details] [diff] [review] bug_1341754_triggeringprincipal_location_seturi.patch I started to think this still some more. Do we actually want to use aTriggeringPrincipal, or principalToInherit. What if it is cross-domain code which triggers this anchor traversal, then we end up having x-domain principal as triggering principal, which is probably fine, but also principalToInherit from x-domain. Then when later loading the shentry, we get different principal to inherit than what the original document had. So, could we get the principalToInherit from the current document? (from its channel or even its nodePrincipal) Or convince me that I'm very wrong here and ask review again :)

Bug 1307736 landed within 53, hence requesting tracking.

(In reply to Olli Pettay [:smaug] (high review load) from comment #14) > Do we actually want to use aTriggeringPrincipal, or principalToInherit. > What if it is cross-domain code which triggers this anchor traversal, then > we end up having x-domain principal as triggering principal, which is > probably fine, but also principalToInherit from x-domain. Then when later > loading the shentry, we get different principal to inherit than what the > original document had. Smaug, let me try to summarize/clarify: Within Location::SetURI() (well actually within CheckURL, but theoretically within ::SetURI) we query the current document's NodePrincipal and set that as the triggeringPrincipal on the docShellLoadInfo and then we call ::LoadURI(). Within ::LoadURI() we then set the principalToInherit *equal* to the triggeringPrincipal. Then we call ::InternalLoad() where the triggeringPrincipal as well as the principalToInherit are identical. So, we want to use the triggeringPrincipal for performing security checks but use the principalToInherit for inheriting the security context. Which is what we do at the moment. And in fact, we should get the same principalToInherit as the original document had. > So, could we get the principalToInherit from the current document? (from its > channel or even its nodePrincipal) That is in fact what's happening at the moment; the principalToInherit is the document's nodePrincipal. I suppose you got confused by the manipulation we do within ::LoadURI() [1], but in fact we don't maniuplate the principalToInherit in that case - the triggeringPrincipal and the principalToInherit are the same. I am re-requesting r? unless I did not interpret your concern precisely, in which case I think you have to explain one more time. [1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1455

When we're doing in-document-navigation, we usually take the principals from mOSHE, so SHEntries for the same document end up having same principals, inheriting from the initial document load. But now with about:blank the document load may be triggered principal Foo, yet latter in-document-navigations get principal Bar in SHEntry, and then when later loading from session history, the document is loaded using principal Bar.

Smaug, as agreed on IRC we should use doc->NodePrincipal() as the principalToInherit in case there is no mOSHE. We further discussed whether we should use the triggeringPrincipal from mOSHE in case mOSHE is non null. I investigated that a little bit. Originally every mOSHE had an owner which then got renamed to triggeringPrincipal, but that was also before we converted docshell to asyncOpen and also before we introduced the concept of a principalToInherit. In the new world the principaltoInherit reflects more accurately the former owner of a channel, hence I would imagine we could update the triggeringPrincipal in that case. Anyway, since this bug needs to be uplifted I suggest we keep that patch rather simple. I filed [1] to investigate that issue more closely. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1344819

Comment on attachment 8844101 [details] [diff] [review] bug_1341754_triggeringprincipal_location_seturi.patch And we really need some test here. This is tricky stuff, so better to have tests to ensure we don't regress something accidentally. Test should try out whether cross domain js can touch about:blank when going back or something.

I extended the tests to exercise the old and the new codepath, having a regular about:blank test as well as same origin and cross origin tests.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e17cf732fbe6 Provide a valid triggeringPrincipal when calling SetURI in Location. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/b20481a22b2d Test SetURI in Location passes triggeringPrincipal. r=smaug

Comment on attachment 8844101 [details] [diff] [review] bug_1341754_triggeringprincipal_location_seturi.patch Approval Request Comment Similar to Bug 1341589, this bug does not cause immediate end user facing problems because within Bug 1337622 we added code to revert to the old behavior before Bug 1307736 landed. Nevertheless, uplifting this bug would have history entries to have the desired/accurate triggeringPrincipal. The assertion we hit here is only in debug builds. In release builds it works fine using reverted semantics. [Feature/Bug causing the regression]: Bug 1307736 - Assert history loads pass a valid triggeringPrincipal for docshell loads [User impact if declined]: See Approval request [Is this code covered by automated tests?]: Yes, an automated test landed on mc with this bug. [Has the fix been verified in Nightly?]: I haven't tested manually - but we have an automated test. [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: --- [Is the change risky?]: No, not risky. [Why is the change risky/not risky?]: We are only setting an explicit triggeringPrincipal to history entries instead of using a built-in fallback mechanism. [String changes made/needed]: No

Comment on attachment 8844409 [details] [diff] [review] bug_1341754_test_triggeringprincipal_location_seturi.patch Approval Request Comment See comment 22.

https://hg.mozilla.org/mozilla-central/rev/e17cf732fbe6 https://hg.mozilla.org/mozilla-central/rev/b20481a22b2d

Comment on attachment 8844101 [details] [diff] [review] bug_1341754_triggeringprincipal_location_seturi.patch Fix an assertion failure and include tests. Aurora54+.

https://hg.mozilla.org/releases/mozilla-aurora/rev/2723f822d105 https://hg.mozilla.org/releases/mozilla-aurora/rev/dae4ddf7b27a

Comment on attachment 8844101 [details] [diff] [review] bug_1341754_triggeringprincipal_location_seturi.patch History/session store fix for debug builds. Let's uplift this for beta 2.

Comment on attachment 8844409 [details] [diff] [review] bug_1341754_test_triggeringprincipal_location_seturi.patch Thanks for the new tests.

https://hg.mozilla.org/releases/mozilla-beta/rev/bfd6d752f4d4 https://hg.mozilla.org/releases/mozilla-beta/rev/37fb7f53b4ac

Setting qe-verify- based on Christoph's assessment on manual testing needs (see Comment 22) and the fact that this fix has automated coverage.