Open Bug 1979032 Opened 1 year ago Updated 10 months ago

sync-about-blank: Skip SetupInheritingPrincipal / ignore null principals to inherit?

Categories

(Core :: DOM: Navigation, task)

Product:
Core
Component:
DOM: Navigation
Type:
task

Tracking

()

People

(Reporter: vhilla, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(5 obsolete files)

To my knowledge, the last open question for bug 543435 is how we handle principals, especially null principals. Tests are currently all passing.

The tl;dr is that in DoURILoad for about:blank, we early return by staying on the already eagerly created document. But we fail to assert aLoadState->PrincipalToInherit()->Equals(GetDocument()->GetPrincipal()) in the case where both are null principals. SetupInheritingPrincipal will naturally create a non-equal null principal to the ones we create in e.g. ContentParent::CreateBrowser or nsFrameLoader::MaybeCreateDocShell. MaybeHandleSubframeHistory might also retreive a null principal from SH that doesn't equal the one on the eagerly created document.

For some background, consider what happens if we create a new docshell and load about:blank. Right now on central, the docshell starts out without document. If one tries to access it, a transient about:blank will be generated lazily while the actual about:blank loads asynchronous. When this async load finishes, it'll replace the transient document. Afaik this async load does some special things to take only one tick, but it largely follows the normal load path.

Our approach with bug 543435 is to create the about:blank eagerly when a docshell is initialized. And if we then reach DoURILoad for about:blank, we don't start an async load but instead commit to the already existing document. This has largely two consequences

  • DoURILoad returns early by calling CompleteInitialAboutBlankLoad. We pulled all kinds of things that would happen during the load path into this method, like firing a load event.
  • Certain state needs to be known at DocShell initialization and this first about:blank document isn't transient anymore, it's what we stay on. We use nsOpenWindowInfo for that.

This was mostly the work of :hsivonen, apologies for my vague explanation.

This state that needs to be known at DocShell initialization includes the principal to be inherited. My understanding of the related code is not sufficient to determine whether we handle that correctly and what might need changing. I think the most relevant places where nsOpenWindowInfo::mPrincipalToInheritForAboutBlank is set are ContentParent::CreateBrowser and nsFrameLoader::MaybeCreateDocShell. I believe before our changes, the principal would've come from nsDocShellLoadState::SetupInheritingPrincipal, or via MaybeHandleSubframeHistory from session history, or maybe other places.

The question is whether the null principals assigned to mPrincipalToInheritForAboutBlank are good enough and we can live with the following assertion in https://phabricator.services.mozilla.com/D155376 nsDocShell.cpp. I see SetupInheritingPrincipals sets some precursor and I don't know the implications of not doing that.

 MOZ_ASSERT_IF(
      isAboutBlankLoadOntoInitialAboutBlank && inheritPrincipal,
      aLoadState->PrincipalToInherit()->Equals(GetDocument()->GetPrincipal()) ||
          (aLoadState->PrincipalToInherit()->GetIsNullPrincipal() &&
           GetDocument()->GetPrincipal()->GetIsNullPrincipal()));

I suppose the risk is some documents being restricted from access due to having different null principals. Maybe this is unlikely to occur on the web?

The only web-observable change I'm aware of is navigating back to data:text/html,<body onunload=''><iframe></iframe></body>. On central, the parent will get a new null principal while the child gets it's principal restored from session history. With bug 1955324, we don't restore null principals and therefore both get the same new null principal. This seems like a good thing.

Blocks: 1955324
Severity: -- → N/A

Leaving a ni? for myself so I can look into the situation here a bit more.

Flags: needinfo?(nika)

Ugh, the much more common case than data URIs might be sandboxed iframes.

ifr = document.createElement("iframe")
ifr.sandbox = ""
ifr.onload = console.log
document.body.appendChild(ifr)

This will hit the (edit: more strict) assert. Interesting that no test does that (edit: didn't run thorough testing with more strict assert given the many failures). If we skip the initial about:blank handling (as discussed) for non-matching principals, sandboxed iframes will have the old async about:blank behavior.

In the sandboxed case, the conflicting principals come from nsDocShellLoadState::SetupInheritingPrincipal vs nsDocShell::CreateAboutBlankDocumentViewer ln6741.

Ah but the patch changed SetupInheritingPrincipal to create a null principal if sandboxed. I think on central, mPrincipalToInherit could be a non-null principal and during the async load, mForceInheritPrincipalDropped causes the principal to be overwritten. I guess to pass aLoadState->PrincipalToInherit()->Equals(GetDocument()->GetPrincipal()) || both null, we made sure the principal is dropped earlier.

So this should only be relevant for initial loads that don't go through a channel, and CreateAboutBlankDocumentViewer already created a new null principal for sandboxing. Maybe we need some way to communicate to SetupInheritingPrincipal or DoURILoad that the eagerly created document indeed got a new null principal, not a null principal inherited from somewhere. Like a mNullPrincipalIsNew flag. Then there could be a debug assertion. Not sure how exactly the code should look like though. Like if mNullPrincipalIsNew, then skip asserting the principals match? Or should mPrincipalToInherit take on the current principal if we determine we want to create a new null principal and the document already is on mNullPrincipalIsNew?

I still struggle to understand all this principal code, why is something named mPrincipalToInherit when it might be a newly created principal, i.e. explicitly not an inherited one?

I still struggle to understand all this principal code, why is something named mPrincipalToInherit when it might be a newly created principal, i.e. explicitly not an inherited one?

Some of this code is unfortunately quite old, and I am guessing mPrincipalToInherit is a name which has been around for a long time. My guess is that at one point it was exclusively the principal being inherited from another document, and that it ended up also being used for other things due to it being a convenient way to manipulate the principal behaviour in a useful way.

In the sandboxed case, the conflicting principals come from nsDocShellLoadState::SetupInheritingPrincipal vs nsDocShell::CreateAboutBlankDocumentViewer ln6741.

TBH I'm a bit suspicious about the change to nsDocShellLoadState::SetupInheritingPrincipal where you add the block of:

  if (mPrincipalToInherit && (aSandboxFlags & SANDBOXED_ORIGIN)) {
    mPrincipalToInherit =
        NullPrincipal::CreateWithInheritedAttributes(mPrincipalToInherit);
  }

IIRC in the current world, we actually have mPrincipalToInherit be the unsandboxed principal here, and end up creating the final sandboxed principal for the load when it is finishing in https://searchfox.org/mozilla-central/rev/59cf9b740d7f5db77c595d0c886734780bfb257b/caps/nsScriptSecurityManager.cpp#334-354. In this logic in nsScriptSecurityManager, the result principal for the load is derived from the load info thanks to GetSandboxedNullPrincipalID which is intended to ensure that the null principal is actually deterministic after the load info is created.

Theoretically we could determine an initial SandboxedNullPrincipalID earlier (similar to how we determine the principal earlier) and ensure that it is passed down both into the code creating the LoadInfo, as well as to the code which is creating the initial sandboxed null principal for the iframe document.

Not sure what mechanisms you're using right now for doing this kind of pre-computation as I haven't really read through the patch as you've written it so far, but perhaps that can give you a pointer as to a direction to take?

Flags: needinfo?(nika) → needinfo?(vhilla)

If something needs to be known earlier, we usually store the pre-computed value on nsOpenWindowInfo so that the docshell can create the right document at initialization.

In this case, we could determine the sandboxed principal in nsFrameLoader::ReallyStartLoadingInternal before MaybeCreateDocShell and LoadURI. (I hope we never already have a docshell at that point?)

This sandboxed principal is determined by that code you linked in nsSecurityManager. Based on the comments, it seems the channel principal comes either from SetupInheritingPrincipal, InternalLoad here, or the channel internals.

I assume we cannot pre-compute this in the general case due to redirects? But maybe we can at least for content about:blanks.

My idea would be the following

  • SetupInheritingPrincipal is currently only called by nsDocShell::LoadURI and there's this mPrincipalIsExplicit member. We remove mPrincipalIsExplicit and let SetupInheritingPrincipal early-return if mPrincipalToInherit is non-null. This should give us better control over what mPrincipalToInherit will be.
  • nsFrameLoader::ReallyStartLoadingInternal already has the nsDocShellLoadState and should call SetupInheritingPrincipal. The second call in LoadURI won't override mPrincipalToInherit due to our change.
  • If this suffices to have a principal for initial content about:blanks, we can create the sandboxed principal in ReallyStartLoadingInternal.
    • My try push to assert we always have a principal looks not too bad: https://treeherder.mozilla.org/jobs?repo=try&revision=c1fecc499c6ea4b24eda779edd32afbc5fbc3b2b
    • We would need to communicate to CreateAboutBlankDocumentViewer that it already received a sandboxed principal, i.e. should skip this. I think the simplest option is to keep some asserts in CreateAboutBlankDocumentViewer and do the sanboxing for that viewer in nsFrameLoader.
    • The nsLoadInfo is created right before CompleteInitialAboutBlankLoad in nsDocShell::DoURILoad, so nsFrameLoader cannot use GetSandboxedNullPrincipalID. If we care about the load info matching the principal, the DocShell could pass the UID from mPrincipalToInherit to the nsLoadInfo ctor.
Flags: needinfo?(vhilla)

In this case, we could determine the sandboxed principal in nsFrameLoader::ReallyStartLoadingInternal before MaybeCreateDocShell and LoadURI. (I hope we never already have a docshell at that point?)

So that doesn't work as other callers of MaybeCreateDocShell sometimes initialize the docshell. Maybe we can store the nsDocShellLoadState in nsFrameLoader and create it for the first call of MaybeCreateDocShell... But I worry that could be too early and some members might not be initialized jet.

But I worry that could be too early and some members might not be initialized jet.

That turned out true, docshell/test/mochitest/test_bug1507702.html causes a call into nsFrameLoader::Show which calls nsFrameLoader::MaybeCreateDocShell() before we got mURIToLoad, so we cannot determine our load state yet.

Not having mURIToLoad yet in MaybeCreateDocShell() is pretty bad because it means we won't know if we'll do the sync about:blank. My idea was that if this is the case, we do the sandboxing here and otherwise let the channel do this. But without mURIToLoad we can't know.

I didn't run extensive tests yet, but if it only impacts about:crash... URLs, maybe it's not relevant to the web?

This is removing a likely wrong part of the main patch.

This is what we discussed. If the load state principal doesn't match
the principal of the eagerly created blank document, we do the safe thing and
load a new and async about:blank.

This should hopefully be rare and a safety measure.

My changes to principal inheritance should make this less likely, but I have
not touched the open window paths yet.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1979032#c7. Changes:

  • SetupInheritingPrincipal won't override mPrincipalToInherit
  • Therefore, we don't need mPrincipalIsExplicit anymore
  • I extract nsFrameLoader::GetLoadState as separate method to contain that
    complexity.
    This gives us more control over mPrincipalToInherit and allows us to set up
    the principal already in nsFrameLoader.

This could land separately.

I pull out a method MaybeInheritDocumentPrincipal from InternalLoad and
already use it in nsFrameLoader::GetLoadState when determining the principal.

I think having that method is a bit cleaner anyway. And it enables us
to reuse it in nsFrameLoader.

This could land separately.

Now to the final part of getting the right principal for sandboxed iframes.

We sandbox the principal for the initial document of an iframe
already in nsFrameLoader and pass the principal both to docshell creation
as well as URI load.

Here are WIP patches to show my idea how we could get the correct sandboxed principal earlier. There are no WPT failures: https://treeherder.mozilla.org/jobs?repo=try&revision=95b1bfec2b7956d171305745dae6bb846934f685

But several tests fail aPrincipalToInherit->Equals(GetDocument()->GetPrincipal()) and I think this happens when nested Iframes cause nsFrameLoader::Show to be called before nsFrameLoader::ReallyStartLoading. I don't yet understand what's going on there, but the show call is coming from the synchronous initial about:blank load of the parent iframe. E.g. see docshell/test/iframesandbox/test_child_navigation_by_location.html with code like <iframe sandbox="allow-same-origin" srcdoc="<iframe></iframe><iframe sandbox></iframe>"</iframe>.

:nika, :hsivonen could you have a look at these patches? I'm not sure if what I did makes sense. I would appreciate feedback before I try to green up try.

Maybe we can do some of the refactoring separately. Then that can land before the main patch so regressions might be easier to fix. And it might be easier to reason about whether those changes are correct.

Flags: needinfo?(nika)
Flags: needinfo?(hsivonen)

We've discussed these patches on zoom, so clearing the ni? for now.

Flags: needinfo?(nika)

Also clearing needinfo to me considering discussion (in another earlier meeting).

Flags: needinfo?(hsivonen)

To summarize the discussion, we agreed to not move forward with the patches. Instead, if the eagerly created document has a principal that doesn't match the load state, we'll synchronously create a new document and continue with the commit steps. This is working well so far and I'll submit that to the main patch soon.

I'm aware of two cases so far where this initial document is clobbered

  • Sandboxed iframes
  • Back navigation to an URI like data:text/html,<body onunload=''><iframe></iframe></body>, see comment 1.
Attachment #9509216 - Attachment is obsolete: true
Attachment #9509217 - Attachment is obsolete: true
Attachment #9509218 - Attachment is obsolete: true
Attachment #9509219 - Attachment is obsolete: true
Attachment #9509220 - Attachment is obsolete: true

Clobbering the document incidentally also fixes bug 1955324. And that means there's a third case where clobbering happens: Privileged content loading an about:blank.

(In reply to Vincent Hilla [:vhilla] from comment #3)

... (edit: didn't run thorough testing with more strict assert given the many failures).

As I read this and looked at ContentParent::CreateBrowser, I think any remote browser that loads about:blank will hit the clobbering case.

Depends on: sync-about-blank

Another possible principal mismatch is bug 2004165, though the fix for this bug is to use the normal load path instead of getting to the mismatch. E.g.

  • Page opens an about:blank popup, which gets a content principal, then restore the popup from SH. A top level about:blank initially has a null principal while the initial load gets the content principal from SH.
  • Page contains a cross-origin iframe which navigates itself to about:blank, then restore the page. The iframe inherits the principal from the parent, then the initial about:blank load goes through SH and obtains the cross-origin principal.
    • A more realistic way for this to occur might be if the iframe undergoes a cross process redirect, we create a transient about:blank and then the load is stopped?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: