Closed Bug 1297338 Opened 3 years ago Closed 3 years ago

Introduce the concept of principalToInherit to loadinfo

Categories

(Core :: DOM: Security, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(5 files, 15 obsolete files)

14.57 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
6.99 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
46.92 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
11.61 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
3.10 KB, patch
mossop
: review+
Details | Diff | Splinter Review
As a spinoff of [1], we should not nullify the triggeringPrincipal if it's the systemPrincipal for docshell loads. If we assign |triggeringPrincipal = nullptr] here [2] then we create a principal from the referrer here [3] which is wrong.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1182569#c21
[2] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1480
[3] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10778
Assignee: nobody → ckerschb
Blocks: 1182569
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [domsecurity-active]
One of the test that illustrates this behavior is docshell/test/chrome/test_bug789773.xul where the triggeringPrincipal is  chrome://mochitests/content/chrome/docshell/test/chrome/test_bug789773.xul instead of the SystemPrincipal.
Boris, assuming I interpreted your suggestion within [1] correctly (see attached patch) I am running into the following assertion [2] (see stacktrace underneath) when running test_bug789773.xul. The value of newWindowPrincipal initially [3] is SystemPrincipal which is then overwritten and set to nullptr |newWindowPrincipal = nullptr;|. The mDoc->NodePrincipal() is SystemPrincipal which then triggers the assertion.

Any suggestions on how to handle and proceed? Happy to provide more debug information if needed. Thanks!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1182569#c21
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2178
[3] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2159


[11713] ###!!! ASSERTION: Unexpected original document: 'uri && NS_IsAboutBlank(uri) && NS_IsAboutBlank(mDoc->GetDocumentURI())', file /home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp, line 2184
#01: nsGlobalWindow::SetInitialPrincipalToSubject() (/home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:2182 (discriminator 1))
#02: nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsIArray*, float*, mozIDOMWindowProxy**) (/home/ckerschb/moz/mc/embedding/components/windowwatcher/nsWindowWatcher.cpp:1137 (discriminator 1))
#03: nsWindowWatcher::OpenWindow2(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsISupports*, float, unsigned char, mozIDOMWindowProxy**) (/home/ckerschb/moz/mc/embedding/components/windowwatcher/nsWindowWatcher.cpp:461 (discriminator 3))
#04: nsGlobalWindow::OpenInternal(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, bool, bool, bool, bool, bool, nsIArray*, nsISupports*, nsPIDOMWindowOuter**) (/home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:11819)
#05: nsGlobalWindow::OpenJS(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsPIDOMWindowOuter**) (/home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:8088)
#06: nsGlobalWindow::OpenOuter(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) (/home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:8046)
#07: nsGlobalWindow::Open(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) (/home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:8055 (discriminator 1))
#08: open (/home/ckerschb/moz/mc-obj-ff-dbg/dom/bindings/WindowBinding.cpp:2193)
#09: genericMethod (/home/ckerschb/moz/mc-obj-ff-dbg/dom/bindings/WindowBinding.cpp:14683)
#10: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/ckerschb/moz/mc/js/src/jscntxtinlines.h:235)
#11: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:453)
#12: InternalCall (/home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:499)
#13: js::CallFromStack(JSContext*, JS::CallArgs const&) (/home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:505)
#14: Interpret (/home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:2881)
#15: js::RunScript(JSContext*, js::RunState&) (/home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:399)
#16: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) (/home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:679)
#17: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (/home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:712)
#18: ExecuteScript (/home/ckerschb/moz/mc/js/src/jsapi.cpp:4330)
#19: JS::CloneAndExecuteScript(JSContext*, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) (/home/ckerschb/moz/mc/js/src/jsapi.cpp:4394)
#20: mozilla::dom::XULDocument::ExecuteScript(nsXULPrototypeScript*) (/home/ckerschb/moz/mc/dom/xul/XULDocument.cpp:3530)
#21: mozilla::dom::XULDocument::ResumeWalk() (/home/ckerschb/moz/mc/dom/xul/XULDocument.cpp:2850)
#22: mozilla::dom::XULDocument::OnScriptCompileComplete(JSScript*, nsresult) (/home/ckerschb/moz/mc/dom/xul/XULDocument.cpp:3463)
#23: mozilla::dom::XULDocument::OnStreamComplete(nsIStreamLoader*, nsISupports*, nsresult, unsigned int, unsigned char const*) (/home/ckerschb/moz/mc/dom/xul/XULDocument.cpp:3378)
#24: mozilla::net::nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (/home/ckerschb/moz/mc/netwerk/base/nsStreamLoader.cpp:106)
#25: nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (/home/ckerschb/moz/mc/netwerk/base/nsBaseChannel.cpp:827)
#26: nsInputStreamPump::OnStateStop() (/home/ckerschb/moz/mc/netwerk/base/nsInputStreamPump.cpp:715)
#27: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (/home/ckerschb/moz/mc/netwerk/base/nsInputStreamPump.cpp:433)
#28: nsInputStreamReadyEvent::Run() (/home/ckerschb/moz/mc/xpcom/io/nsStreamUtils.cpp:97 (discriminator 1))
#29: nsThread::ProcessNextEvent(bool, bool*) (/home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:1058 (discriminator 1))
#30: NS_ProcessNextEvent(nsIThread*, bool) (/home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:290)
#31: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:96)
#32: MessageLoop::RunInternal() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:233)
#33: MessageLoop::RunHandler() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:226)
#34: MessageLoop::Run() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:205)
#35: nsBaseAppShell::Run() (/home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:158)
#36: nsAppStartup::Run() (/home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:284)
#37: XREMain::XRE_mainRun() (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4302)
#38: XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4429)
#39: XRE_main (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4520)
#40: do_main (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:259)
#41: main (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:392)
#42: __libc_start_main (/build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:321)
#43: _start (/home/ckerschb/moz/mc-obj-ff-dbg/dist/bin/firefox)
#44: ??? (???:???)
Flags: needinfo?(bzbarsky)
Well, we shouldn't be creating the initial about:blank as system.  That's what the assertion is asserting.  So whatever code is doing that in this case (presumably a call to CreateAboutBlankContentViewer from somewhere) needs to get fixed.  And the right fix is to have it use the "principal to be inherited", not the triggeringPrincipal.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #3)
> Well, we shouldn't be creating the initial about:blank as system.  That's
> what the assertion is asserting.  So whatever code is doing that in this
> case (presumably a call to CreateAboutBlankContentViewer from somewhere)
> needs to get fixed.  And the right fix is to have it use the "principal to
> be inherited", not the triggeringPrincipal.

Boris, I don't fully understand what you mean by your suggestion. Let me try to summarize, for test test_bug789773.xul the uri about:mozilla is now loaded using a systemprincipal as the triggeringPrincipal.

The about:blank load which triggered the assertion here [1] initiates from dom/base/nsFrameLoader.cpp. I don't know what the right fix is, but presumably we should only leave the triggeringPrincipal a nullptr and let docshell assing a systemPrincipal as the triggeringPrincipal.

I don't understand what you mean by >> And the right fix is to have it use the "principal to
> be inherited", not the triggeringPrincipal. << ?

Thanks for your help and clarification!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2182
Attachment #8783915 - Attachment is obsolete: true
Attachment #8785932 - Flags: feedback?(bzbarsky)
Per our conversation just now, I think we should do the following:

1)  Check whether on the relevant testcase there is a CreateAboutBlankContentViewer call that happens with the system principal with the docshell change but with a non-system principal without it.

2)  If that is not the case, then check whether there is a call to nsWindowWatcher::ReadyOpenedDocShellItem that has "doc" with a system principal with the docshell change but not otherwise.  Chances are this is the case we're actually hitting, given our conversation just now and what you said about the load which triggered the assertion originating in nsFrameLoader.

3)  In either case, add another principal member, called "principalToInherit" or something, to loadinfo.  This will be the same as triggeringPrincipal unless explicitly set otherwise.  Use the principalToInherit member in nsScriptSecurityManager::GetChannelResultPrincipal on the *_INHERITS codepath.

4)  In nsDocShell::LoadURI, instead of nulling out triggeringPrincipal, do something like this:

  nsCOMPtr<nsIPrincipal> principalToInherit = triggeringPrincipal;

and then if IsSystemPrincipal(principalToInherit) null out principalToInherit and set inheritPrincipal to true.  Pass both triggeringPrincipal and principalToInherit to internalLoad.  Then in internalLoad, where we check the INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL flag, condition that on !principalToInherit instead of !triggeringPrincipal and reset principalToInherit to GetInheritedPrincipal, the way triggeringPrincipal gets reset right now.

The basic idea is that docshell wants the "principal used for security checks" and the "principal inherited by loads" to be different principals, but right now in loadinfo they're the same thing: triggeringPrincipal...

Now that I write all this down, I'm actually not sure which of those two concepts triggeringPrincipal should really represent.  I was assuming that we'd use triggeringPrincipal for security checks and add the new "principalToInherit" for the "inherited by loads" concept.  And if triggeringPrincipal were only used for those two things, that would be ok.  But if we have other things using triggeringPrincipal, which of the two different meanings would they want to see?

Arguably a smaller change from current behavior is to have two principals in loadinfo: triggeringPrincipal and securityCheckPrincipal or something.  securityCheckPrincipal defaults to triggeringPrincipal if not set otherwise.  Docshell would keep doing what it does not for triggeringPrincipal, but use the original value of triggeringPrincipal, before it munged it, as the securityCheckPrincipal.  This approach has the benefit that anyone examining triggeringPrincipal right now that's not the security check code doesn't have its behavior changed.  If we do the approach I described above, with changing the value of triggeringPrincipal for docshell loads, we need to audit all existing triggeringPrincipal consumers to see which value they should be using...
What I can answer so far:

(In reply to Boris Zbarsky [:bz] from comment #5)
> 1)  Check whether on the relevant testcase there is a
> CreateAboutBlankContentViewer call that happens with the system principal
> with the docshell change but with a non-system principal without it.

When running test_bug789773.xul there is is a total of 7 calls to CreateAboutBlankContentViewer() which are completely identical with and without the docshell changes applied where sometimes the principal is a SystemPrincipal and sometimes a nullptr;

> 2)  If that is not the case, then check whether there is a call to
> nsWindowWatcher::ReadyOpenedDocShellItem that has "doc" with a system
> principal with the docshell change but not otherwise.  Chances are this is
> the case we're actually hitting, given our conversation just now and what
> you said about the load which triggered the assertion originating in
> nsFrameLoader.

Without the docshell changes applied the doc->NodePrincipal() within nsWindowWatcher::ReadyOpenedDocShellItem [1] is about:blank but with the docshell changes applied the principal is the SystemPrincipal.

[1] https://dxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#2186
Summary: Don't modify triggeringPrincipal if SystemPrincipal for docshell loads → Introduce the concept of principalToInherit to loadinfo
(In reply to Boris Zbarsky [:bz] from comment #5)
> 3)  In either case, add another principal member, called
> "principalToInherit" or something, to loadinfo.  This will be the same as
> triggeringPrincipal unless explicitly set otherwise.  Use the
> principalToInherit member in
> nsScriptSecurityManager::GetChannelResultPrincipal on the *_INHERITS
> codepath.

That part is fairly straight forward.
Attachment #8785932 - Attachment is obsolete: true
Attachment #8785932 - Flags: feedback?(bzbarsky)
Attachment #8786060 - Flags: review?(bzbarsky)
> 4)  In nsDocShell::LoadURI, instead of nulling out triggeringPrincipal, do
> something like this:
> 
>   nsCOMPtr<nsIPrincipal> principalToInherit = triggeringPrincipal;
> 
> and then if IsSystemPrincipal(principalToInherit) null out
> principalToInherit and set inheritPrincipal to true.  Pass both
> triggeringPrincipal and principalToInherit to internalLoad.  Then in
> internalLoad, where we check the INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL flag,
> condition that on !principalToInherit instead of !triggeringPrincipal and
> reset principalToInherit to GetInheritedPrincipal, the way
> triggeringPrincipal gets reset right now.

That part still doesn't fully make sense to me. In particular, don't we have to do something with the principalToInherit here:
https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10770

But if we do, then what to we set the triggeringPrincipal to? Confuses me, maybe you have a clearer picture of that Boris.
Attachment #8786061 - Flags: feedback?(bzbarsky)
I'd really like us to make a call on the "Arguably a smaller change from current behavior" option before we start changing code, fwiw.

> In particular, don't we have to do something with the principalToInherit here

Yes, probably.  But only in the case when principalToInherit is not null, which will correspond to the current "if (triggeringPrincipal)" branch.  And on that branch we don't currently modify triggeringPrincipal, and still would not do so.
(In reply to Boris Zbarsky [:bz] from comment #9)
> I'd really like us to make a call on the "Arguably a smaller change from
> current behavior" option before we start changing code, fwiw.

Boris, what does that mean exactly? Can you propose a way forward with this bug? It seems crucial to get this bug fixed, especially with regards to converting docshell to use asyncOpen2() (Bug 1182569).

I am happy to provide more debug information or whatever it takes to get this bug moving. Thanks!
Flags: needinfo?(bzbarsky)
> Can you propose a way forward with this bug?

I've proposed two ways forward.  I don't have the information at my fingertips that would allow me to choose between them; that depends on how triggeringPrincipal is being used in our codebase and whether we have the time and inclination to audit all the uses.  I don't know the former off the top of my head, and I don't know the latter at all.  I expect you know the former better than I do, and you certainly know the latter way better than I do, at least for yourself...
Flags: needinfo?(bzbarsky)
Attachment #8786061 - Attachment is obsolete: true
Attachment #8786061 - Flags: feedback?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #5)
> Now that I write all this down, I'm actually not sure which of those two
> concepts triggeringPrincipal should really represent.  I was assuming that
> we'd use triggeringPrincipal for security checks and add the new
> "principalToInherit" for the "inherited by loads" concept.  And if
> triggeringPrincipal were only used for those two things, that would be ok. 
> But if we have other things using triggeringPrincipal, which of the two
> different meanings would they want to see?
> 
> Arguably a smaller change from current behavior is to have two principals in
> loadinfo: triggeringPrincipal and securityCheckPrincipal or something. 
> securityCheckPrincipal defaults to triggeringPrincipal if not set otherwise.
> Docshell would keep doing what it does not for triggeringPrincipal, but use
> the original value of triggeringPrincipal, before it munged it, as the
> securityCheckPrincipal.  This approach has the benefit that anyone examining
> triggeringPrincipal right now that's not the security check code doesn't
> have its behavior changed.  If we do the approach I described above, with
> changing the value of triggeringPrincipal for docshell loads, we need to
> audit all existing triggeringPrincipal consumers to see which value they
> should be using...

Boris, I think I like the idea of keeping the triggeringPrincipal as is, which is the principal that initiated (triggered) the load and which we use for security checks (in almost all cases that principal should be identical to the referrer). And I also think it's better to introduce the concept of principalToInherit to docshell, scriptsecuritymanager and loadinfo instead of introducing a securityCheckPrincipal.

I tried to audit usages of TriggeringPrincipal [1] and mostly we only use if to security checks, besides:
a) AddToSessionHistory where we want to force the principal to be inherited, and
b) nsJSProtocolHandler where we also want to force the principal to be inherited.
c) and obviously GetChannelResultPrincipal()
I suppose those are the only places that need to be updated.

I also attached all of the patches I wrote so far which might influence our decision in the end.

Here is the TRY run which might also help us find some overseen edge cases:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c79656bed69d

What do you think, does that sound reasonable? I am happy to provide whatever information you are looking for to help us make a decision, just let me know. Thanks!

[1] https://dxr.mozilla.org/mozilla-central/search?q=TriggeringPrincipal()&redirect=false
Flags: needinfo?(bzbarsky)
Keeping triggeringPrincipal as is and just adding principalToInherit is fine given we've examined the consumers.

The consumer in WyciwygChannelChild::Init is not necessarily used for security checks, or at least might need to ship the "principal to inherit" over as well.

Similarly, LoadInfoToLoadInfoArgs might need to do something with the principalToInherit, right?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #15)
> The consumer in WyciwygChannelChild::Init is not necessarily used for
> security checks, or at least might need to ship the "principal to inherit"
> over as well.

That is absolutely correct, we should enhance SendInit within that new argument.
 
> Similarly, LoadInfoToLoadInfoArgs might need to do something with the
> principalToInherit, right?

Yes, that is already incorporated within bug_1297338_extend_loadinfo_with_principaltoinherit.patch which should also do all the serialization for the loadinfo.
Comment on attachment 8786060 [details] [diff] [review]
bug_1297338_extend_loadinfo_with_principaltoinherit.patch

This generally looks good.  I think you need to propagate the principal to inherit in WyciwygChannelChild::Init and WyciwygChannelParent::RecvInit.

>+   * is set. For non docshell loads that principal is always identical to

I think I would prefer s/non docshell loads/loads that are not TYPE_DOCUMENT or TYPE_SUBDOCUMENT/.  There's nothing about loadinfo that indicates whether a load is a "docshell load"...

r=me with the comment nit fixed and the principal to inherit propagated in WyciwygChannel.
Attachment #8786060 - Flags: review?(bzbarsky) → review+
Please let me know when you're ready for review on the "add_principaltoinherit_within_docshell" patch.  The "do_not_set_triggeringprincipal_within_frameloader_if_about_blank" patch should not be needed at all, I would think.
(In reply to Boris Zbarsky [:bz] from comment #17)
> I think I would prefer s/non docshell loads/loads that are not TYPE_DOCUMENT
> or TYPE_SUBDOCUMENT/.  There's nothing about loadinfo that indicates whether
> a load is a "docshell load"...

Updated. Carrying over r+ from bz.
Attachment #8786060 - Attachment is obsolete: true
Attachment #8787627 - Flags: review+
Boris, here are the requested changes for the WyciwygChannel.
Attachment #8787628 - Flags: review?(bzbarsky)
Comment on attachment 8787201 [details] [diff] [review]
bug_1297338_do_not_set_triggeringprincipal_within_frameloader_if_about_blank.patch

(In reply to Boris Zbarsky [:bz] from comment #18)
>  The "do_not_set_triggeringprincipal_within_frameloader_if_about_blank" patch
> should not be needed at all, I would think.

I think we would need to change that, no? Otherwise we end up with exactly the same as before. Namely we would enter that branch and set
  principalToInherit = nullptr;
so that later within the loadInfo constructor the current triggeringPrincipal would be set as the principalToInherit and the assertion within nsGlobalWindow would still get triggeret, but maybe I am just missing something within the docshell patch.
Attachment #8787201 - Flags: review?(bzbarsky)
Comment on attachment 8787628 [details] [diff] [review]
bug_1297338_extend_wyciwygchannel_with_principaltoinherit.patch

r=me
Attachment #8787628 - Flags: review?(bzbarsky) → review+
Comment on attachment 8787200 [details] [diff] [review]
bug_1297338_add_principaltoinherit_within_docshell.patch

Boris, I think I am still missing something, we are failing a lot of tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c79656bed69d&selectedJob=26764666

One of them for example is: test_child_navigation_by_location.html [1] where I get the error:
> Permission denied to access property "assign"

I haven't debugged that closely yet. But since something is completely off I was wondering if you could provide some feedback on the docshell patch before I do some more debugging. Thanks!


[1] https://dxr.mozilla.org/mozilla-central/source/docshell/test/iframesandbox/test_child_navigation_by_location.html#48
Attachment #8787200 - Flags: feedback?(bzbarsky)
> Namely we would enter that branch and set
>  principalToInherit = nullptr;

That's fine.  And then we would end up creating a codebase principal for the principal to inherit, no?  I mean, that's what we do right now for the case of null triggeringPrincipal (and what caused the issues in bug 1182569).  Yes, that means that in the docshell code we need to ensure we have a principalToInherit in the cases when we currently ensure we have a triggeringPrincipal.  That's the whole point here: those games with nulling out and then creating as codebase or inheriting from current doc are needed to ensure proper behavior for the _inheritance_ case, so they should be performed on the principalToInherit, not the triggeringPrincipal.
Comment on attachment 8787201 [details] [diff] [review]
bug_1297338_do_not_set_triggeringprincipal_within_frameloader_if_about_blank.patch

This is wrong; see comemnt 24.
Attachment #8787201 - Flags: review?(bzbarsky) → review-
Comment on attachment 8787200 [details] [diff] [review]
bug_1297338_add_principaltoinherit_within_docshell.patch

> I was wondering if you could provide some feedback on the docshell patch before I do some more debugging.

Some things that jump out at me:

1)  In reload(), passing null for principal to inherit is probably wrong.  Null isn't the same as "use the triggering principal", after all.  You want to pass at least "principal" but more likely the session history entry needs to store both principals to do this correctly.

2)  This generally applies to all calls to InternalLoad: passing null principal to inherit while passing a non-null triggering principal is buggy.

3)  In DoURILoad, you should always have a non-null aPrincipalToInherit, unless aTriggeringPrincipal is also null.  If both are null, they should probably get set to the same thing.

4)  In AddToSessionHistory, assigning a principalToInherit to triggeringPrincipal seems odd.  Again, chances are the SHEntry needs to store both.
Attachment #8787200 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8787201 [details] [diff] [review]
bug_1297338_do_not_set_triggeringprincipal_within_frameloader_if_about_blank.patch

That patch is indeed not needed anymore.
Attachment #8787201 - Attachment is obsolete: true
Boris, I extended nsISHEntry.idl with a attribute principalToInherit to extend that separation between triggeringPrincipal and principalToInherit. All of the local testing seems to work. Since Monday is a holiday in the US, I would like to ask for review before the weekend even though I don't have a full TRY run yet. A few things where I am not entirely sure and I would like to draw your attention to:

(a) ::Reload(), ::OnNewURI(), ::OnLinkClickSync()  uses the same principal as TriggeringPrincipal and PrincipalToInherit, I would imagine that's ok, right?
(b) In nsDocShell::AddToSessionHistory() I think we can on solely on the loadInfo and skip the part where we query the owner of the channel, right?
(c) I searched the codebase where we manually set the TriggeringPrincipal on the nsSHEntry because I thought we have to manually set the principalToInherit in those cases as well, but I couldn't find any, so I suppose that part should be ok too.

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4241cb496f8e
Attachment #8787200 - Attachment is obsolete: true
Attachment #8787734 - Flags: review?(bzbarsky)
Comment on attachment 8787734 [details] [diff] [review]
bug_1297338_add_principaltoinherit_within_docshell.patch

>+++ b/caps/nsScriptSecurityManager.cpp
>         if (forceInterit) {

While you're here, rename this to "forceInherit"?

>+            if (nsContentUtils::ChannelShouldInheritPrincipal(principalToInherit,
>                                                                uri,

And fix the indent here and on the next two lines?

>+++ b/docshell/base/nsDocShell.cpp
>@@ -1471,25 +1471,25 @@ nsDocShell::LoadURI(nsIURI* aURI,
>+    } else if (nsContentUtils::IsExpandedPrincipal(principalToInherit)) {

You want to change the principalToInherit here too, right?  Or at least not reset the triggeringPrincipal?  But probably both.

Below this, this part:

  if (!triggeringPrincipal && !inheritPrincipal && !principalIsExplicit) {
    // See if there's system or chrome JS code running
    inheritPrincipal = nsContentUtils::LegacyIsCallerChromeOrNativeCode();
  }

Needs to test !principalToInherit instead of !triggeringPrincipal, right?

> nsDocShell::InternalLoad(nsIURI* aURI,

So what's our invariant for this function?  When is aTriggeringPrincipal allowed to be null?  When is aPrincipalToInherit allowed to be null?

Seems to me that both might be null, the former will never be null unless the latter also is, but the latter might be null while the former is non-null (and in fact creating that situation is what the code in LoadURI is all about).  Does that match your understanding?  Might be worth documenting...

Which principal is the right one to pass to OnNewURI in the short-circuited load case and why?  Or both (see below)?  Needs documentation if we only pass one explaining the choice.

> nsDocShell::DoURILoad(nsIURI* aURI,
>  // only inherit if we have a triggeringPrincipal

This comment needs fixing.

>+  } else if (!principalToInherit && aReferrerURI) {

Do we need this bit?  In practice, principalToInherit is only used if "inherit" is true, right?  And that only gets set if principalToInherit is non-null here...

I guess it doesn't hurt to have this, but it's more or less a no-op as far as I can tell.

>+  if (aPrincipalToInherit) {
>+    loadInfo->SetPrincipalToInherit(principalToInherit);

But given this, why are we bothering with the principalToInherit things above if we only set it when aPrincipalToInherit is non-null?  This needs to either test "principalToInherit" or we should remove the dead code above.

>@@ -11604,17 +11628,18 @@ nsDocShell::OnNewURI(nsIURI* aURI, nsICh
       (void)AddToSessionHistory(aURI, aChannel, aTriggeringPrincipal,
-                                aCloneSHChildren, getter_AddRefs(mLSHE));
+                                aTriggeringPrincipal, aCloneSHChildren,

This doesn't look right.  It will create a session history entry with a different principalToInherit than our original load had.  Seems to me like we should pass both the triggering principal and the principalToInherit into OnNewURI.

>@@ -12171,54 +12198,53 @@ nsDocShell::AddToSessionHistory(nsIURI* 
>-    aChannel->GetOwner(getter_AddRefs(owner));

No, this code can't go away while we're still setting channel owners somewhere...  Please leave it here.  

>+++ b/docshell/base/nsIDocShell.idl

This needs much more extensive documentation of the interaction between aTriggeringPrincipal and aPrincipalToInherit and when they're allowed to be null.

>+++ b/docshell/shistory/nsISHEntry.idl
>+     * is set. For loads that are not TYPE_DOCUMENT or TYPE_SUBDOCUMENT

Such loads do not produce history entries, fwiw.

There's one important thing still missing here, given the "null principal to inherit means inherit the triggering principal" setup: Session restore needs to save/restore the principal to inherit from history entries.  And doing that will need to be done very carefully to not break migration back and forward.  Specifically:

1)  When restoring across a migration forward (read: Firefox got updated to this new code), the saved thing only has a triggering principal.  We want to use that for principal to inherit.  But what to use for triggering principal?  The old code didn't do security checks on these loads, but the new code will do them and the loads could well fail...  It's probably OK to use the one saved principal as triggering principal too, but would need to be done a bit carefully.  This can maybe be mitigated somewhat by shipping the two-principal setup in session restore while we _keep_ not doing security checks for a few releases and then switching on the security checks, but I expect you don't want to wait a few releases to do AsyncOpen2 in docshell.

2)  When restoring across a migration backward (read: User downgraded Firefox for some reason) the saved thing has both principals, but the reading code will only look for a triggering principal.  That principal will then be used to inherit as needed.  So while we can have a totally sensible new-world setup of "triggering == system", "principal to inherit" == "http://something", URI = "data:whatever", restoring that to the old world as "triggering == system", URI="data:whatever" would be a security bug.

What that means is that to avoid security bugs during backmigrations the new code must put the "principal to inherit" (if not null) into the "triggeringPrincipal" thing in session restore.  That's somewhat convenient, since that's where a forward-migration would expect to find the "principal to inherit" anyway.  The actual triggeringPrincipal would have to go into a slot with a different name, and get read from there.

Yes, this makes the naming suck, after you just adjusted it in bug 1286472.  The problem is that the SHEntry code never cared about the "triggering" aspects of the triggeringPrincipal, just its inheritance aspects.  :(  I'm open to other suggestions here; the above is just what I came up with after thinking about this for a few minutes.

Now to answer your questions:

>(a) ::Reload(), ::OnNewURI(), ::OnLinkClickSync()  uses the same principal as
>TriggeringPrincipal and PrincipalToInherit, I would imagine that's ok, right?

It's OK for Reload (because this is on the "no session history entry" codepath) and OnLinkClickSync (because at that point there is only one principal involved to start with).  IT's not OK for OnNewURI, per my comments above.

>(b) In nsDocShell::AddToSessionHistory() I think we can on solely on the
>loadInfo and skip the part where we query the owner of the channel, right?

As I said above, I don't think so.  We still set channel owners in various places in our code, most notably in nsChromeProtocolHandler::NewChannel2.

> (c) I searched the codebase where we manually set the TriggeringPrincipal on
the nsSHEntry

We do it in session restore code...  See http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/mobile/android/components/SessionStore.js#1298 and http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/browser/components/sessionstore/SessionHistory.jsm#399
Attachment #8787734 - Flags: review?(bzbarsky) → review-
Boris, thanks for the detailed feedback. I added more details and answers below. In general, this patch is ready for review (TRY also looks good now), but we need one more patch (once this is r+ed) to cover the following parts which I haven’t incorporated yet:
* add more detailed documentation
* Changes within SessionHistory.jsm for deserializeEntry().

(In reply to Boris Zbarsky [:bz] from comment #29)
> >+++ b/caps/nsScriptSecurityManager.cpp

Incorporated your nits and nsScriptSecurityManager should be ready to go by now.

> >+++ b/docshell/base/nsDocShell.cpp
> > nsDocShell::InternalLoad(nsIURI* aURI,
> So what's our invariant for this function?  When is aTriggeringPrincipal
> allowed to be null?  When is aPrincipalToInherit allowed to be null?

I suppose it can be either way, both null, only aPrincipalToInherit null or also only aTriggeringPrincipal might be null given that code:

	if (aLoadFlags & LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL) {
	  principalToInherit = nsNullPrincipal::CreateWithInheritedAttributes(this);
	  inheritPrincipal = false;
  	}

> Which principal is the right one to pass to OnNewURI in the short-circuited
> load case and why?  Or both (see below)?

I extend OnNewURI() with an argument: aPrincipalToInherit so we have those two principals separated.

> >+  } else if (!principalToInherit && aReferrerURI) {
> 
> Do we need this bit?  In practice, principalToInherit is only used if
> "inherit" is true, right?  And that only gets set if principalToInherit is
> non-null here...

Both these parts got updated within this patch:
(a) triggeringPrincipal: If we don’t explicitly received aTriggeringPrincipal, then we create one from the referrer if aReferrerURI is non null, and otherwise we fall back to the SystemPrincipal.
(b) principalToInherit: As you said, that part was basically a no-op. I think it’s better to just use the fallback within the loadInfo in that case where principalToInherit falls back to the triggeringPrincipal.

> >@@ -11604,17 +11628,18 @@ nsDocShell::OnNewURI(nsIURI* aURI, nsICh
>        (void)AddToSessionHistory(aURI, aChannel, aTriggeringPrincipal,
> -                                aCloneSHChildren, getter_AddRefs(mLSHE));
> +                                aTriggeringPrincipal, aCloneSHChildren,
> 
> This doesn't look right.  It will create a session history entry with a
> different principalToInherit than our original load had.  Seems to me like
> we should pass both the triggering principal and the principalToInherit into
> OnNewURI.

As already mentioned, OnNewURI() now takes a separate argument: aPrincipalToInherit.

While we are discussing that, the code within nsDocShell::AddToSessionHistory() is really complicated and I am not sure if I am using the right fallbacks. E.g. even in the old code, wouldn’t it make more sense to use the argument aTriggeringPrincpial instead of querying the owner from the channel? As I said, I am not sure if I am doing the right thing within that method.

> >+++ b/docshell/base/nsIDocShell.idl
> 
> This needs much more extensive documentation of the interaction between
> aTriggeringPrincipal and aPrincipalToInherit and when they're allowed to be
> null.

Well, I agree more documentation is useful here, but to be honest, I don’t precisely know when they are allowed to be null.

> >+++ b/docshell/shistory/nsISHEntry.idl

So, I suppose serializeEntry() is pretty straight forward. I think all we have to do is to update serializeTriggeringPrincipal() to the more generic serializePrincipal().

> 1)  When restoring across a migration forward
> 2)  When restoring across a migration backward

I haven’t incorporated that change yet. The main problem I have: how can I distinguish between the two cases (1) and (2) and also the regular case, where we don’t have any migration happening?

And you are right: I don’t necessarily want to wait a few cycles before converting docshell to rely on AsyncOpen2().
Attachment #8787734 - Attachment is obsolete: true
Attachment #8788428 - Flags: review?(bzbarsky)
Do you mind posting an interdiff from the last thing I reviewed?
Flags: needinfo?(ckerschb)
Comment on attachment 8787734 [details] [diff] [review]
bug_1297338_add_principaltoinherit_within_docshell.patch

Marking the r- patch as non-obsolete since I will post an interdiff to review for Boris.
Attachment #8787734 - Attachment is obsolete: false
Flags: needinfo?(ckerschb)
Comment on attachment 8788428 [details] [diff] [review]
bug_1297338_add_principaltoinherit_within_docshell.patch

This becomes obsolete.
Attachment #8788428 - Attachment is obsolete: true
Attachment #8788428 - Flags: review?(bzbarsky)
This patch shows an interdiff and should provide everything needed codewise. Documentation is still missing, maybe you can provide some help with that, in particular:

* within nsISHEntry.idl, for |attribute nsIPrincipal principalToInherit;|.
* documenttation within nsDocShell::InternalLoad(nsIURI* aURI,...), in particular: what's the invariant? When is what principal allowed to be null?
Attachment #8788886 - Flags: review?(bzbarsky)
Boris, as explained within Comment 33, the change within SessionHistory.jsm is still complicated. Is there any way we can distinguish between:

a) A regular restore,
b)  When restoring across a migration forward
c)  When restoring across a migration backward
Attachment #8788887 - Flags: feedback?(bzbarsky)
> the code within nsDocShell::AddToSessionHistory() is really complicated
> and I am not sure if I am using the right fallbacks.
...
> wouldn’t it make more sense to use the argument aTriggeringPrincpial
> instead of querying the owner from the channel

AddToSessionHIstory has:

  NS_PRECONDITION(!aChannel || !aTriggeringPrincipal, "Shouldn't have both set");

so it's one or the other but not both.

In the aChannel != nullptr branch, it gets the owner off the channel, and if that fails tries the loadinfo.  Otherwise, it's already set triggeringPrincipal to aTriggeringPrincipal.
> Well, I agree more documentation is useful here, but to be honest, I don’t precisely know when they are allowed to be null.

OK.  Well, with your last patch here, when are they null?  Let's start there and then we can try to figure out whether those case are all correct and if so how to summarize the requirements in documentation...
Flags: needinfo?(ckerschb)
Comment on attachment 8788886 [details] [diff] [review]
bug_1297338_add_principaltoinherit_within_docshell_interdiff.patch

Thank you for the interdiff.

OnNewURI should probably assert that aTriggeringPrincipal is non-null if and only if aPrincipalToInherit is non-null.

r=me with that assert added and once we sort out the comments in nsIDocShell.idl
Attachment #8788886 - Flags: review?(bzbarsky) → review+
> how can I distinguish between the two cases (1) and (2) and also the regular case

I don't think you really can, but check with the people who own that code?
Comment on attachment 8788887 [details] [diff] [review]
bug_1297338_add_principaltoinherit_to_sessionhistory.patch

You need to fix mobile/android/components/SessionStore.js too.  But we should figure out the shape of the fix first.

>+    // (c) Migration backward: The entry has a triggeringPrincipal
>+    //     as well as a principalToInherit. Use the principalToInherit
>+    //     as the triggeringPrincipal.

No, see, this is the annoying part.  In the migration backward case whatever entry is written by your changed serialization code needs to work with the already existing deserialization code in the older version.  That code will look for a thing called entry.triggeringPrincipal_b64 and then use that as the principal to inherit.

Which is why comment 29 said:

  the new code must put the "principal to inherit" (if not null) into the
  "triggeringPrincipal" thing in session restore.

as part of my proposed solution to this problem.  But I'm open to other ones, of course, assuming they address the problems I'm seeing.  Which you should reconfirm with the owners of this code are real problems, btw.
Attachment #8788887 - Flags: feedback?(bzbarsky) → feedback-
Qfolding the two docshell patches (inital patch + interdiff) together into on docshell patch. Carrying over r+ from bz.

Please note, that we are still missing appropriate documentation, will be provide that in another patch once we figured all invariants.
Attachment #8787734 - Attachment is obsolete: true
Attachment #8788886 - Attachment is obsolete: true
Flags: needinfo?(ckerschb)
Attachment #8789357 - Flags: review+
Gijs, within this bug we are adding a new attribute 'principalToInherit' to nsISHEntry.idl which makes migrations backward and forward within SessionHistory.jsm tricky because what's currently the triggeringPrincipal will be the 'principalToInherit' in the new world (see comment 29 the part starting with 'There's one important thing still missing here...' for more details).

Question is: Do you happen to know if there is a possibility to distinguish between a backward/forward migration within SessionHistory.jsm?

(See also first attempt for writing that code within bug_1297338_add_principaltoinherit_to_sessionhistory.patch)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #43)
> Gijs, within this bug we are adding a new attribute 'principalToInherit' to
> nsISHEntry.idl which makes migrations backward and forward within
> SessionHistory.jsm tricky because what's currently the triggeringPrincipal
> will be the 'principalToInherit' in the new world (see comment 29 the part
> starting with 'There's one important thing still missing here...' for more
> details).
> 
> Question is: Do you happen to know if there is a possibility to distinguish
> between a backward/forward migration within SessionHistory.jsm?
> 
> (See also first attempt for writing that code within
> bug_1297338_add_principaltoinherit_to_sessionhistory.patch)

302 mikedeboer who has been looking at sessionstore more than me.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mdeboer)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #43)
> Gijs, within this bug we are adding a new attribute 'principalToInherit' to
> nsISHEntry.idl which makes migrations backward and forward within
> SessionHistory.jsm tricky because what's currently the triggeringPrincipal
> will be the 'principalToInherit' in the new world (see comment 29 the part
> starting with 'There's one important thing still missing here...' for more
> details).
> 
> Question is: Do you happen to know if there is a possibility to distinguish
> between a backward/forward migration within SessionHistory.jsm?
> 
> (See also first attempt for writing that code within
> bug_1297338_add_principaltoinherit_to_sessionhistory.patch)

The only thing that would work is to deprecate 'triggeringPrincipal' and write migration code in SessionHistory.jsm to set 'principalToInherit' to 'triggeringPrincipal' if that's available and `delete` the property from the deserialized object.
In this case we ensure we won't see 'triggeringPrincipal' ever again in subsequent sessionstore.js flushes and are ready to use 'principalToInherit'.
But perhaps I misunderstood the semantics you're looking to implement here, so please correct me if I'm wrong!
Flags: needinfo?(mdeboer) → needinfo?(ckerschb)
(In reply to Mike de Boer [:mikedeboer] from comment #45)
> The only thing that would work is to deprecate 'triggeringPrincipal' and
> write migration code in SessionHistory.jsm to set 'principalToInherit' to
> 'triggeringPrincipal' if that's available and `delete` the property from the
> deserialized object.
> In this case we ensure we won't see 'triggeringPrincipal' ever again in
> subsequent sessionstore.js flushes and are ready to use 'principalToInherit'.
> But perhaps I misunderstood the semantics you're looking to implement here,
> so please correct me if I'm wrong!

Thanks Mike for the reply!

The problem is that so far we only had a triggeringPrincipal in the old world. In the new world however we have two principals, a principalToInherit and a triggeringPrincipal. To complicate things further, what used to be the triggeringPrincipal is now the principalToInherit. Ideally what we would want is:

a) Regular sessions restore
   - The entry has a principalToInherit and a triggeringPrincipal.
   - Set the principalToInherit back to principalToInherit and the triggeringPrincipal back to triggeringPrincipal.

b) When restoring across a migration forward
   - The entry has a triggeringPrincipal (but no principalToInherit)
   - What used to be the triggeringPrincipal now needs to be set to principalToInherit. Potentially it's fine to also use that triggeringPrincipal from the entry as the actual triggeringPrincipal.

c) When restoring across a migration backward
   - The entry has a principalToInherit and a triggeringPrincipal.
   - But here is the annoying part. Basically the new principalToInherit is what used to be the triggeringPrincipal. In order to remain compatible, we would have to set principalToInherit to the slot of 'triggeringPrincipal' because the old code does not know about the principalToInherit.

If we would be able to somehow distinguish between a backward and a forward migration, then my thinking was that we could write 3 different codepaths for the serialization.

Does that make sense? Do you think there might be a way so we can distinguish those 3 paths? Or rather no?
Flags: needinfo?(ckerschb) → needinfo?(mdeboer)
Blocks: 1301666
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #46)
> Thanks Mike for the reply!
> 
> The problem is that so far we only had a triggeringPrincipal in the old
> world. In the new world however we have two principals, a principalToInherit
> and a triggeringPrincipal. To complicate things further, what used to be the
> triggeringPrincipal is now the principalToInherit. Ideally what we would
> want is:

Ouch, quite the mess we've gotten ourselves into!

> a) Regular sessions restore
>    - The entry has a principalToInherit and a triggeringPrincipal.
>    - Set the principalToInherit back to principalToInherit and the
> triggeringPrincipal back to triggeringPrincipal.

For these cases it's quite common to simply rename the property that changes semantics, either by appending '2' or changing its name entirely.
IOW: I suggest making it 'principalThatTriggered' or something other so that you be smarter and take c) into account.

Updated;
a) Regular sessions restore
  - The entry has a principalToInherit and a principalThatTriggered.
  - Set the principalToInherit back to principalToInherit and the principalThatTriggered back to principalThatTriggered. We make sure that principalToInherit keeps being mirrored to the old, deprecated triggeringPrincial, otherwise c) will break.

> b) When restoring across a migration forward
>    - The entry has a triggeringPrincipal (but no principalToInherit)
>    - What used to be the triggeringPrincipal now needs to be set to
> principalToInherit. Potentially it's fine to also use that
> triggeringPrincipal from the entry as the actual triggeringPrincipal.

Updated;
b) When restoring across a migration forward
  - The entry has a triggeringPrincipal (but no principalToInherit)
  - What used to be triggeringPrincipal now needs to be set to principalToInherit. It _has_ to be fine to also use that triggeringPrincipal from the entry as principalThatTriggered, since we'll still be saving 'triggeringPrincipal' to disk. If not, c) will break.

> c) When restoring across a migration backward
>    - The entry has a principalToInherit and a triggeringPrincipal.
>    - But here is the annoying part. Basically the new principalToInherit is
> what used to be the triggeringPrincipal. In order to remain compatible, we
> would have to set principalToInherit to the slot of 'triggeringPrincipal'
> because the old code does not know about the principalToInherit.

Updated;
c) When restoring across a migration backward
  - The entry has a principalToInherit, a principalThatTriggered and a triggeringPrincipal.
  - Since the new code made sure that principalToInherit kept being mirrored to triggeringPrincipal, we're still good.
  - It's quite important to file a bug to remove 'triggeringPrincipal' from the codebase entirely after a few versions, say Fx 55. Otherwise we'll keep having bloat around forever.

Thoughts?
Flags: needinfo?(mdeboer)
Boris, Mike, so basicially the idea is to store the principalToInherit as triggeringPrincipal_b64 and also as principalToInherit_b64 and store the actual triggeringPrincipal as tmpTriggeringPrincipalTillFF55_b64.

Whenever we deserialize we perform the following action:
* use the triggeringPrincipal_b64 as triggeringPrincipal and also as principalToInherit, and if
* tmpTriggeringPrincipalTillFF55_b64 is non null then we use that more accurate information of principals from  tmpTriggeringPrincipalTillFF55_b64 and principalToInherit_b64 as triggeringPrincipal and principalToInherit.

Does that make sense, or am I still missing something? If you are fine with that then I would also incorporate that change into mobile/android/components/SessionStore.js.
Attachment #8788887 - Attachment is obsolete: true
Attachment #8789892 - Flags: feedback?(mdeboer)
Comment on attachment 8789892 [details] [diff] [review]
bug_1297338_add_principaltoinherit_to_sessionhistory.patch

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

I think you forgot to f?bz about this, but perhaps it's good I looked at it first.
You're going to bang heads with 1294866, because I think that'll land sooner. Just a heads-up ;) It should make the JS code here simpler.

::: docshell/shistory/nsISHEntry.idl
@@ +223,5 @@
> +     * Hands off, that's not the principal you want. You want
> +     * to query the principalToInherit or the triggeringPrincipal.
> +     * Only used for session restore and to beremoved within Bug 1301666.
> +     */
> +    attribute nsIPrincipal tmpTriggeringPrincipalTillFF55;

I think it's not handy to put version numbers in here... I think a name like `deprecatedTriggeringPrincipal`, whilst keeping the comment above, would communicate your intent as clearly.
Attachment #8789892 - Flags: feedback?(mdeboer) → feedback+
Boris, within the latest TRY run [1], the test browser_bug553455.js was still failing, in particular the subtest test_urlbar() [2]. It took me quite some time to figure out what's going on, but eventually I narrowed it down to the code within AddonManager.jsm [3], which was introduced within Bug 1042699.

In the old world the security check was performed on the following arguments:
> aInstallingPrincipal      moz-nullprincipal:{6f285133-84a4-47c8-8b33-060a94b72eab}
> aBrowser.contentPrincipal moz-nullprincipal:{c9c509e8-86cf-43de-aa3a-429b17b6f1cf}

In the new world, where we introduced a principalToInherit, we got the following arguments (before this patch was applied):
> aInstallingPrincipal      SystemPrincipal
> aBrowser.contentPrincipal moz-nullprincipal:{e2d2b41f-c806-47ed-b862-5aab87cd7ede}

which caused the test to time out, because security checks did not fail. I am not entierly sure if the security checks within [3] are correct and we just have to use the principalToInherit (which is was this test is doing), or if we have to update the security checks within [3].

Potentially we have to ask dveditz or mossop, who have introduced the security checks within Bug 1042699.


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=28ac810e482a
[2] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug553455.js#790
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#2188
(In reply to Mike de Boer [:mikedeboer] from comment #49)
> You're going to bang heads with 1294866, because I think that'll land
> sooner. Just a heads-up ;) It should make the JS code here simpler.

Yeah, I am aware of that bug, thanks for the heads up though.

> > +    attribute nsIPrincipal tmpTriggeringPrincipalTillFF55;
> I think it's not handy to put version numbers in here... I think a name like
> `deprecatedTriggeringPrincipal`, whilst keeping the comment above, would
> communicate your intent as clearly.

Updated - thanks!
Attachment #8790795 - Flags: review?(mdeboer)
Rebased patch because Bug 1294866 landed in the meantime.
Attachment #8790795 - Attachment is obsolete: true
Attachment #8790795 - Flags: review?(mdeboer)
Attachment #8791142 - Flags: review?(mdeboer)
Attachment #8789892 - Attachment is obsolete: true
Comment on attachment 8790793 [details] [diff] [review]
bug_1297338_add_principaltoinherit_addonmanager.patch

OK.  So the test is doing "user types an XPI URL in the url bar and hits enter".  In that situation, it used to be that we coerced the triggering principal to that of the currently-loaded page, but now we leave it as chrome and just coerce the principalToInherit...

Given that, the substance of this change seems reasonable.  In particular, for all cases when triggeringPrincipal is not system, we will have triggeringPrincipal == principalToInherit, right?

You probably do want to get review from the owner of amContentHandler.js here (mossop?), because if nothing else the member of the "installs" object might need a better name.  But r=me on the security implications.
Attachment #8790793 - Flags: review+
Comment on attachment 8791142 [details] [diff] [review]
bug_1297338_add_principaltoinherit_to_sessionhistory.patch

> +    // (c) triggeringPrincipal is serialized as deprecatedTriggeringPrincipal_b64.

The naming is a bit weird, because this value is the one that's going to stick around in the future, unless we do another migration.  It's "triggeringPrincipal_b64" that will go away, right?

>+    // When deserializing the entry we first read back (a) and if available
>+    // we overwrite values with the more accurate info from (b) and (c).

This comment is very confusing, but see below....

>+    // we can update this code to serialize the triggeringPrincipal

This comment doesn't make sense to me. This code _is_ serializing the triggeringPrincipal!

>+    // issues let's deserialize the entry according to the following rule:

This seems unnecessarily complicated.  It seems to me that there are three possible states here:

1)  "entry" has no principals hanging off it at all.  In that case, shEntry should have both its principals null.
2)  "entry" has a deprecatedTriggeringPrincipal_b64 or a principalToInherit_b64 or both.  In that case it was serialized with our new code and we should deserialize accordingly.
3)  "entry" was serialized with the old code and only has a triggeringPrincipal_b64.  In this case we should deserialize accordingly.

Is the complexity here because we want to be not only backward-compatible but also forward-compatible with a world in which there is no deprecatedTriggeringPrincipal_b64 and triggeringPrincipal_b64 is the actual triggeringPrincipal?  If that's the case, that should be clearly documented....

Assuming that this is the goal, I'm not sure we're achieving it, because falsy deprecatedTriggeringPrincipal_b64 can then mean one of three things:

I) This was serialized with old session restore, which had no concept of principal to inherit at all.
II) This was serialized with current session restore, but shEntry.triggeringPrincipal was null.
III) This was serialized with future session restore.

Can case II happen?  If it can, then the code in this patch, as written, will treat it as case I, which doesn't seem right.  But maybe case II is impossible?  Case III certainly seems possible and will be treated like case I.

If the forward-compat is not the goal, then why not just check for deprecatedTriggeringPrincipal_b64 or principalToInherit_b64 and if so deserialize as saved by current session restore, else deserialize as saved by old session restore.  This will handle case II correctly, since for current session restore either principalToInherit_b64 will be truthy in case II or all the principals should be null anyway.

>+++ b/docshell/shistory/nsISHEntry.idl

I don't understand why we're adding this new attribute.  It does not seem to be used.
Attachment #8791142 - Flags: review-
Comment on attachment 8790793 [details] [diff] [review]
bug_1297338_add_principaltoinherit_addonmanager.patch

(In reply to Boris Zbarsky [:bz] (busy, pun intended) from comment #55)
> Comment on attachment 8790793 [details] [diff] [review]
> bug_1297338_add_principaltoinherit_addonmanager.patch
> 
> OK.  So the test is doing "user types an XPI URL in the url bar and hits
> enter".  In that situation, it used to be that we coerced the triggering
> principal to that of the currently-loaded page, but now we leave it as
> chrome and just coerce the principalToInherit...
> 
> Given that, the substance of this change seems reasonable.  In particular,
> for all cases when triggeringPrincipal is not system, we will have
> triggeringPrincipal == principalToInherit, right?

That sounds correct to me!

> You probably do want to get review from the owner of amContentHandler.js
> here (mossop?), because if nothing else the member of the "installs" object
> might need a better name.  But r=me on the security implications.

Dave, you introduced that code within Bug 1042699, can you take a look and see if my change looks reasonable to you as well? Let me know if you have any questions!
Attachment #8790793 - Flags: review?(dtownsend)
Comment on attachment 8791142 [details] [diff] [review]
bug_1297338_add_principaltoinherit_to_sessionhistory.patch

Mike, I'll flag you again once I have incorporated Boris' suggestions - thanks!
Attachment #8791142 - Flags: review?(mdeboer)
(In reply to Boris Zbarsky [:bz] (busy, pun intended) from comment #56)
> deprecatedTriggeringPrincipal_b64 or principalToInherit_b64 and if so
> deserialize as saved by current session restore, else deserialize as saved
> by old session restore.  This will handle case II correctly, since for
> current session restore either principalToInherit_b64 will be truthy in case
> II or all the principals should be null anyway.

Thanks Boris. All of what you said just makes sense now. The reason I was so confused is, that I always wanted to keep the triggeringPrincipal_b64 around, but in fact there is no need to do that. I updated all of the codepaths and decided to introduce those two new variables: triggeringPrincipal_base64 and principalToInherit_base64. If one of those two variables in non-null, then we know the session was stored using the new code, and otherwise we fall back to using triggeringPrincipal_b64 which can then easily be removed within Bug 1301666.
Attachment #8791142 - Attachment is obsolete: true
Attachment #8791597 - Flags: review?(mdeboer)
Boris, as suggested within comment 38, I am posting a few load examples, where I will post the values of aTriggeringPrincipal and aPrincipalToInherit and with what values we end up in the loadInfo. I am trying to pick different examples, hopefully covering as many cases as possible:

> === EXAMPLE 1:

nsDocShell::InternalLoad {
  uri: resource://gre-resources/hiddenWindow.html
  aTriggeringPrincipal: nullptr
  aPrincipalToInherit: nullptr
}

nsDocShell::LoadInfo {
  uri: resource://gre-resources/hiddenWindow.html
  contentPolicyType: 6
  loadingPrincipal: nullptr
  triggeringPrincipal: SystemPrincipal
  principalToInherit: SystemPrincipal
}

> === EXAMPLE 2:

nsDocShell::InternalLoad {
  uri: chrome://browser/content/browser.xul
  aTriggeringPrincipal: SystemPrincipal
  aPrincipalToInherit: SystemPrincipal
}

nsDocShell::LoadInfo {
  uri: chrome://browser/content/browser.xul
  contentPolicyType: 6
  loadingPrincipal: nullptr
  triggeringPrincipal: SystemPrincipal
  principalToInherit: SystemPrincipal
}

> === EXAMPLE 3a:

nsDocShell::InternalLoad {
  uri: about:blank
  aTriggeringPrincipal: nullptr
  aPrincipalToInherit: NullPrincipal
}

nsDocShell::LoadInfo {
  uri: about:blank
  contentPolicyType: 6
  loadingPrincipal: nullptr
  triggeringPrincipal: SystemPrincipal
  principalToInherit: NullPrincipal
}

> === EXAMPLE 3b:

nsDocShell::InternalLoad {
  uri: about:blank
  aTriggeringPrincipal: SystemPrincipal
  aPrincipalToInherit: nullptr
}

nsDocShell::LoadInfo {
  uri: about:blank
  contentPolicyType: 6
  loadingPrincipal: nullptr
  triggeringPrincipal: SystemPrincipal
  principalToInherit: SystemPrincipal
}

> === EXAMPLE 3c:

nsDocShell::InternalLoad {
  uri: about:blank
  aTriggeringPrincipal: resource://gre-resources/hiddenWindow.html
  aPrincipalToInherit: resource://gre-resources/hiddenWindow.html
}

nsDocShell::LoadInfo {
  uri: about:blank
  contentPolicyType: 29
  loadingPrincipal: resource://gre-resources/hiddenWindow.html
  triggeringPrincipal: resource://gre-resources/hiddenWindow.html
  principalToInherit: resource://gre-resources/hiddenWindow.html
}

Looking at 3a, 3b, and 3c is there some discrepancy, or can that happen?

> === EXAMPLE 4:

nsDocShell::InternalLoad {
  uri: http://mochi.test:8888/tests/docshell/test/iframesandbox/test_child_navigation_by_location.html
  aTriggeringPrincipal: http://mochi.test:8888/tests?autorun=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp
  aPrincipalToInherit: http://mochi.test:8888/tests?autorun=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp
}

nsDocShell::LoadInfo {
  uri: http://mochi.test:8888/tests/docshell/test/iframesandbox/test_child_navigation_by_location.html
  contentPolicyType: 29
  loadingPrincipal: http://mochi.test:8888/tests?autorun=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp
  triggeringPrincipal: http://mochi.test:8888/tests?autorun=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp
  principalToInherit: http://mochi.test:8888/tests?autorun=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp
}

Boris, if you could help me a little with what we want as the documentation for 'aPrincipalToInherit' within nsIDocShell.idl, I would be really greatful. I am happy to add more such debug information if that helps to find out the interaction between aPrincipalToInherit and aTriggeringPrincipal and when which one is allowed to be null.
Comment on attachment 8791597 [details] [diff] [review]
bug_1297338_add_principaltoinherit_to_sessionhistory.patch

Yes, this makes a lot more sense.  r=me.  ;)
Attachment #8791597 - Flags: review+
OK, so trying to make sense of comment 60...

It looks like either one might be null independently of the other in our current code.  Null triggering principal is interpreted as system (?) while null principal to inherit is interpreted as same as triggering at least in these cases.  But it's not clear to me that these cases are exhaustive, and I bet there are not.

What I was looking for was some code examination and then classification for when null can get passed for one or the other principal.  For the "principal to inherit", there are cases when we explicitly pass null (because of the code we added that nulls it out), right?  What does passing null for it mean?  Does it depend on some other arguments (it does!)?  Are there other places that also pass null for it?  Do they actually want the resulting behavior?

Similar for the triggering principal.  What does null actually mean (looks like it means "use referrer or system if there is no referrer", for the moment), and is that the behavior people passing null want (probably, since we didn't really change where it's null... except in the one case when it's system in a content-type docshell and a referrer is passed: before this change that would use the referrer as the triggering principal, but now it will use the system principal.  That's the actual behavior change we want, though so it seems ok...

Anyway, does any of that help at all?  I'd still really like to know what codepaths can pass null for these things, other than the "principalToInherit is null" thing from nsDocShell::LoadURI in the "system doing a load in content-type docshell" case.
Comment on attachment 8791597 [details] [diff] [review]
bug_1297338_add_principaltoinherit_to_sessionhistory.patch

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

Ok, looks good to me! Please fix the nits I found below and you're ready to go.

::: browser/components/sessionstore/SessionHistory.jsm
@@ +376,5 @@
> +    // triggeringPrincipal_base64 and principalToInherit_base64. If not
> +    // we fall back to using the principalToInherit (which is stored
> +    // as triggeringPrincipal_b64) as the triggeringPrincipal and
> +    // the principalToInherit.
> +    // 

nit: trailing whitespace.

@@ +383,5 @@
> +      if (entry.triggeringPrincipal_base64) {
> +        try {
> +          shEntry.triggeringPrincipal =
> +            Utils.deserializePrincipal(entry.triggeringPrincipal_base64);
> +        }

Please put the catch on the same line, like `} catch (ex) {`

@@ +391,5 @@
> +        try {
> +          shEntry.principalToInherit =
> +            Utils.deserializePrincipal(entry.principalToInherit_base64);
> +        }
> +        catch (e) { debug(e); }

Same here.

@@ +393,5 @@
> +            Utils.deserializePrincipal(entry.principalToInherit_base64);
> +        }
> +        catch (e) { debug(e); }
> +      }
> +    }

Same for this else: `} else if (...) {`

@@ +399,5 @@
> +      try {
> +        shEntry.triggeringPrincipal = Utils.deserializePrincipal(entry.triggeringPrincipal_b64);
> +        shEntry.principalToInherit = shEntry.triggeringPrincipal;
> +      }
> +      catch (e) { debug(e); }

Same here.

::: mobile/android/components/SessionStore.js
@@ +1344,5 @@
> +    // triggeringPrincipal_base64 and principalToInherit_base64. If not
> +    // we fall back to using the principalToInherit (which is stored
> +    // as triggeringPrincipal_b64) as the triggeringPrincipal and
> +    // the principalToInherit.
> +    // 

nit: trailing whitespace.

@@ +1367,5 @@
> +      try {
> +        shEntry.triggeringPrincipal = Utils.deserializePrincipal(aEntry.triggeringPrincipal_b64);
> +        shEntry.principalToInherit = shEntry.triggeringPrincipal;
> +      }
> +      catch (e) { debug(e); }

Looks like copy-paste, can you apply my comments for SessionHistory.jsm here too?
Attachment #8791597 - Flags: review?(mdeboer) → review+
Comment on attachment 8790793 [details] [diff] [review]
bug_1297338_add_principaltoinherit_addonmanager.patch

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

I don't like giving you more work but I think we'll be safer if we switch this and the rest of the code that uses it to use the right term here otherwise we'll just end up with confusion somewhere down the line.
Attachment #8790793 - Flags: review?(dtownsend) → review-
(In reply to Mike de Boer [:mikedeboer] from comment #63)
> Ok, looks good to me! Please fix the nits I found below and you're ready to
> go.

Done. Thanks for the review!
Attachment #8791597 - Attachment is obsolete: true
Attachment #8792417 - Flags: review+
(In reply to Dave Townsend [:mossop] from comment #64)
> I don't like giving you more work but I think we'll be safer if we switch
> this and the rest of the code that uses it to use the right term here
> otherwise we'll just end up with confusion somewhere down the line.

That's totally fine, thanks for the review. I replaced the variable triggeringPrincipal with principalToInherit all the way. I also checked within Bug 1042699 where the term triggeringPrincipal to make sure I haven't missed anything. That patch should do it - thanks!
Attachment #8790793 - Attachment is obsolete: true
Attachment #8792418 - Flags: review?(dtownsend)
Attachment #8792418 - Flags: review?(dtownsend) → review+
Blocks: 1303943
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f268206cfe7
Extend LoadInfo with a PrincipalToInherit member. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/48f4c0999b84
Extend WyciwygChannel with concept of principalToInherit. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/dee3d68b0fc8
Introduce concept of principalToInherit to docshell and scriptSecurityManager. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e95355b00798
Introduce concept of principalToInherit to SessionHistory. r=bz,mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3056e9ae41a
Use principalToInherit instead of TriggeringPrincipal within AddonManager.js. r=bz,mossop
I filed Bug 1303943 to update the documentation, but I wanted to avoid bitrot of those patches.
The problem is that without the documentation I can't tell whether the patches are correct.  And once we have the documentation I will have to page all this stuff back in _again_ to recheck whether it makes sense... :(
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ae5fb774393
Don't inherit principal for external loads and update documentation for principalToInherit. r=bz
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d8fce5d995
Backed out changeset 9ae5fb774393 landed with wrong bug number
Depends on: 1313397
Depends on: CVE-2017-5466
You need to log in before you can comment on or make changes to this bug.