1319051 - Only allow top-level load for data: URIs in non-default remote type when opened from an existing page.

Status: NEW

(Core :: Security: Process Sandboxing, defect, P3)

Description

[Bob Owen (:bobowen)]

As part of bug 1147911 we had to allow data: URIs in a top level load in remote types other than the default one for some talos tests.

If we can we should add a test that data: URI is only allowed in the non-default content process remote type (web), when opened from another page.

I think we can do this by adding a check at [1], if we are being loaded from another page (via link or window.open), I think we'll always have got a TabParent from TabParent::GetNextTabParent.
So after that point we should only get data: URIs for the web remote type.

[1] https://hg.mozilla.org/mozilla-central/file/b7f895c1dc2e/dom/ipc/ContentParent.cpp#l950

Comment 1

[Henry Chang [:hchang]]

(In reply to Bob Owen (:bobowen) from comment #0)
> As part of bug 1147911 we had to allow data: URIs in a top level load in
> remote types other than the default one for some talos tests.
> 
> If we can we should add a test that data: URI is only allowed in the
> non-default content process remote type (web), when opened from another page.
> 
> I think we can do this by adding a check at [1], if we are being loaded from
> another page (via link or window.open), I think we'll always have got a
> TabParent from TabParent::GetNextTabParent.
> So after that point we should only get data: URIs for the web remote type.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/b7f895c1dc2e/dom/ipc/
> ContentParent.cpp#l950

Comment 2

[Henry Chang [:hchang]]

I finally got more clear to this bug:

As Bob said in comment 0, at [1]

ContentParent::CreateBrowser()
{
  // ...
  if (TabParent* parent = TabParent::GetNextTabParent()) {
    parent->SetOwnerElement(aFrameElement);
    // case (1): 
    //   - Called from ContentParent::RecvCreateWindow()
    //   - ContentChild::ProvideWindowCommon() decided not to change process.
    return parent;
  }

  // case (2): 
  //   - ContentParent::RecvCreateWindowInDifferentProcess()
  //   - ContentChild::ProvideWindowCommon() decided to load in a new process.
}

Both case 1 and case 2 are in the middle of browser.js::openURIInFrame()
and are loading "about:blank" and the actual URI respectively. For case (1),
the actual URI will be loaded after ContentParent::RecvCreateWindow() returns.

For case 1, the URI will be loaded in the current process and we don't have
the actual URI at this point (the child is responsible for loading it instead.)
Therefore we have nothing to do in this case. (Or we should propagate
required information to here to do some safety check?)

For case 2, we can get the remote type from |aFrameElement| (as well as
the "URI To load" supposedly). What we should check here is if the current
remote type and new window remote type are expected. For example, if
the current remote type is web, then new window remote type MUST be web.


[1] https://hg.mozilla.org/mozilla-central/file/b7f895c1dc2e/dom/ipc/ContentParent.cpp#l950

Comment 3

[Henry Chang [:hchang]]

Attachment: propagate_url.diff

Comment 4

[Henry Chang [:hchang]]

The call stack of Content::CreateBrowser (for the change process required case)

ContentParent::RecvCreateWindowInDifferentProcess
 ContentParent::CommonCreateWindow
  browser.js::openURIInFrame
   tabbrowser.xml::loadOneTab
    tabbrowser.xml::addTab
     tabbrowser.xml::_linkBrowserToTab (where we decide the remote type)
      NodeBinding::appendChild
       nsFrameLoader::Show
        nsFrameLoader::ShowRemoteFrame
         nsFrameLoader::TryRemoteBrowser
          ContentParent::CreateBrowser (where we change process)

Since the intention is to check if the correct process type is going to be used for data: URI,
the check can be done between tabbrowser.xml::_linkBrowserToTab and
ContentParent::CreateBrowser. If we wanna do it in ContentParent::CreateBrowser,
the required information that we don't have is the "URL to load" because
ContentParent::CreateBrowser seems to be solely responsible for creating the
browser without loading the URL [1]. I had to tag the "URL to load" to
the browser element in tabbrowser.xml::_linkBrowserToTab (like how we tag
the remote type) and getAttribute in CotentParent::CreateBrowser to get
the actual URL to load. The attached patch is a prototype that how it might
be done.

I don't feel like this is the final solution but I can't figure out a better way.
Bob, what do you think of this issue?

[1] The counter part of ContentParent::RecvCreateWindowInDifferentProcess is
ContentChild::SendCreateWindowInDifferentProcess, which is called from
ContentChild::ProvideWindow as the implementation of nsIWindowProvider::ProvideWindow.
According to

https://dxr.mozilla.org/mozilla-central/rev/9b8bf5feb0b52aa4b03aa5fa3d4f0727b2974663/embedding/nsIWindowProvider.idl#52

"The nsIWindowProvider implementation must not load this URI into the
window it returns.  This URI is provided solely to help the
nsIWindowProvider implementation make decisions; the caller will
handle loading the URI in the window returned if provideWindow
returns a window."

Comment 5

[Bob Owen (:bobowen)]

Ah right, I hadn't thought this through fully when I wrote the bug, it was just a nice to have.

If we don't have the URL at this point then maybe this is the wrong place to have the check and we'll have to rethink.

It's possible it will be a difficult thing to check in a generic way like this.

Comment 6

[Jim Mathies [:jimm]]

Hey bob, does this block a feature (read-access restrictions)?

Comment 7

[Bob Owen (:bobowen)]

(In reply to Jim Mathies [:jimm] from comment #6)
> Hey bob, does this block a feature (read-access restrictions)?

No, it's a nice to have sanity check for something that couldn't happen at the moment.

I think we should just sb+ and I'll de-assign henry for the moment.