flypig.co.uk

As we rounded things off yesterday I was heading into a new task. My plan is to look into the User Agent string functionality of ESR 91. My hunch is that the overrides aren't working correctly in the updated build, although I'm not yet sure whether this is the case or why it might be the case. But I'm hoping that this is the reason for some sites currently working poorly with the updated browser engine.

But before I do that I want to try to address an issue raised by rob_kouw on the Sailfish forum. Or, to be more accurate, raised by Rob's wife. Here's Rob's comment:
 

Apart from the fact that I'm chuffed that Rob is taking the time to read these posts, it's also a very good point: are these hidden browser tabs I've spent the last couple of weeks implementing going to be safe? Wouldn't it be a bad thing if websites have the ability to spawn hidden tabs without the user realising it?

I also want to acknowledge the useful input I received privately from thigg on this topic as well. These comments combined have provided really useful input when considering the final architecture.

So when we consider the security of these hidden tabs my claim is that it's safe because although there is now functionality to hide tabs, there is in fact only one way that this can happen and that's as a direct consequence of the user selecting the "Save web page to PDF" option in the Sailfish Browser menu. There's no other way it can happen and especially not through some malicious Web site creating hidden tabs without the user's consent.

But right now this is just a hypothesis. It deserves exploring in more depth.

Let's start at the sharp end. The tabs are hidden or shown based on the value stored in the hidden role in the DeclarativeTabModel class:

QVariant DeclarativeTabModel::data(const QModelIndex & index, int role) const {
    if (index.row() < 0 || index.row() >= m_tabs.count())
        return QVariant();

    const Tab &tab = m_tabs.at(index.row());
    if (role == ThumbPathRole) {
        return tab.thumbnailPath();
    } else if (role == TitleRole) {
        return tab.title();
    } else if (role == UrlRole) {
        return tab.url();
    } else if (role == ActiveRole) {
        return tab.tabId() == m_activeTabId;
    } else if (role == TabIdRole) {
        return tab.tabId();
    } else if (role == DesktopModeRole) {
        return tab.desktopMode();
    } else if (role == HiddenRole) {
        return tab.hidden();
    }
    return QVariant();
}

This hidden value is exposed in the QML, but it's not exposed to the JavaScript of the page in the tab and it's also not a value that can be set after the page has been created: it's essentially constant.

As we can see from the last few lines, the hidden state comes from the Tab class. The only way to set it is when the tab is instantiated:

Tab::Tab(int tabId, const QString &url, const QString &title,
    const QString &thumbPath, const bool hidden)
    : m_tabId(tabId)
    , m_requestedUrl(url)
    , m_url(url)
    , m_title(title)
    , m_thumbPath(thumbPath)
    , m_desktopMode(false)
    , m_hidden(hidden)
    , m_browsingContext(0)
    , m_parentId(0)
{
}

As we've seen in previous posts, the route that the hidden flag takes to get to this point is circuitous at best. But it all goes through only C++ code in the EmbedLite portion of the Sailfish gecko-dev project. None of this is exposed, either for reading or writing, to JavaScript. And it all leads back to this one location:

NS_IMETHODIMP
WindowCreator::CreateChromeWindow(nsIWebBrowserChrome *aParent,
                                  uint32_t aChromeFlags,
                                  nsIOpenWindowInfo *aOpenWindowInfo,
                                  bool *aCancel,
                                  nsIWebBrowserChrome **_retval)
{
[...]
  const bool isForPrinting = aOpenWindowInfo->GetIsForPrinting();

  mChild->CreateWindow(parentID, reinterpret_cast<uintptr_t>(parentBrowsingContext.get()), aChromeFlags, isForPrinting, &createdID, aCancel);

This requires just a little explanation: it's the isForPrinting flag which becomes the hidden flag. This point here is the only route to the flag being set. But that's not enough to give us the confidence that there's only one way to control this flag. We also need to confirm how the isForPrinting flag gets set.

There are two places this gets called from, both of them in WindowWatcher.cpp. The first:

NS_IMETHODIMP
nsWindowWatcher::OpenWindowWithRemoteTab(nsIRemoteTab* aRemoteTab,
                                         const nsACString& aFeatures,
                                         bool aCalledFromJS,
                                         float aOpenerFullZoom,
                                         nsIOpenWindowInfo* aOpenWindowInfo,
                                         nsIRemoteTab** aResult) {

And the second:

nsresult nsWindowWatcher::OpenWindowInternal(
    mozIDOMWindowProxy* aParent, const nsACString& aUrl,
    const nsACString& aName, const nsACString& aFeatures, bool aCalledFromJS,
    bool aDialog, bool aNavigate, nsIArray* aArgv, bool aIsPopupSpam,
    bool aForceNoOpener, bool aForceNoReferrer, PrintKind aPrintKind,
    nsDocShellLoadState* aLoadState, BrowsingContext** aResult) {
[...]

For the first, the crucial data going in to this method is aOpenWindowInfo. For the second the flag gets set like this:

    openWindowInfo->mIsForPrinting = aPrintKind != PRINT_NONE;

Consequently the crucial value going in is aPrintKind.

Let's now follow back the first of these. It comes via ContentParent::CommonCreateWindow() where it's passed in as the aForPrinting parameter like this:

mozilla::ipc::IPCResult ContentParent::CommonCreateWindow(
    PBrowserParent* aThisTab, BrowsingContext* aParent, bool aSetOpener,
    const uint32_t& aChromeFlags, const bool& aCalledFromJS,
    const bool& aWidthSpecified, const bool& aForPrinting,
    const bool& aForWindowDotPrint, nsIURI* aURIToLoad,
    const nsCString& aFeatures, const float& aFullZoom,
    BrowserParent* aNextRemoteBrowser, const nsString& aName, nsresult& aResult,
    nsCOMPtr<nsIRemoteTab>& aNewRemoteTab, bool* aWindowIsNew,
    int32_t& aOpenLocation, nsIPrincipal* aTriggeringPrincipal,
    nsIReferrerInfo* aReferrerInfo, bool aLoadURI,
    nsIContentSecurityPolicy* aCsp, const OriginAttributes& aOriginAttributes) {
[...]
  openInfo->mIsForPrinting = aForPrinting;
[...]
  aResult = pwwatch->OpenWindowWithRemoteTab(thisBrowserHost, aFeatures,
                                             aCalledFromJS, aFullZoom, openInfo,
                                             getter_AddRefs(aNewRemoteTab));
[...]

Where this is called, in one case we have a hard coded false value passed in for isForPrinting:

  mozilla::ipc::IPCResult ipcResult = CommonCreateWindow(
      aThisTab, parent, /* aSetOpener = */ false, aChromeFlags, aCalledFromJS,
      aWidthSpecified, /* aForPrinting = */ false,
      /* aForPrintPreview = */ false, aURIToLoad, aFeatures, aFullZoom,
      /* aNextRemoteBrowser = */ nullptr, aName, rv, newRemoteTab, &windowIsNew,
      openLocation, aTriggeringPrincipal, aReferrerInfo,
      /* aLoadUri = */ true, aCsp, aOriginAttributes);

In the other case the value is triggered via a call to ContentParent::RecvCreateWindow(). Following this back via SendCreateWindow() we find it eventually gets back to a call to nsWindowWatcher::OpenWindowInternal(). In other words, we find ourselves in the same place as the other path.

So let's move over to this path. We want to follow back to see what calls nsWindowWatcher::OpenWindowInternal() and in particular the value passed in for aPrintKind.

There are several ways this method can be called, all from inside nsWindowWatcher.cpp. One of them straightforwardly sets aPrintKind to PRINT_NONE:

  MOZ_TRY(OpenWindowInternal(aParent, aUrl, aName, aFeatures,
                             /* calledFromJS = */ false, dialog,
                             /* navigate = */ true, argv,
                             /* aIsPopupSpam = */ false,
                             /* aForceNoOpener = */ false,
                             /* aForceNoReferrer = */ false, PRINT_NONE,
                             /* aLoadState = */ nullptr, getter_AddRefs(bc)));

The others both end up getting called in from two separate places in nsGlobalWindowOuter.cpp:

$ grep -rIn "OpenWindow2" * --include="*.cpp"
gecko-dev/dom/base/nsGlobalWindowOuter.cpp:7134:
    rv = pwwatch->OpenWindow2(this, url, name, options,
gecko-dev/dom/base/nsGlobalWindowOuter.cpp:7154:
    rv = pwwatch->OpenWindow2(this, url, name, options,
gecko-dev/toolkit/components/windowwatcher/nsWindowWatcher.cpp:353:
    nsWindowWatcher::OpenWindow2(mozIDOMWindowProxy* aParent,

Both of these have the aPrintKind parameter set like this:

  const auto wwPrintKind = [&] {
    switch (aPrintKind) {
      case PrintKind::None:
        return nsPIWindowWatcher::PRINT_NONE;
      case PrintKind::InternalPrint:
        return nsPIWindowWatcher::PRINT_INTERNAL;
      case PrintKind::WindowDotPrint:
        return nsPIWindowWatcher::PRINT_WINDOW_DOT_PRINT;
    }
    MOZ_ASSERT_UNREACHABLE("Wat");
    return nsPIWindowWatcher::PRINT_NONE;
  }();

In all cases the crucial aPrintKind value seen here is passed in as a parameter to nsGlobalWindowOuter::OpenInternal() which, in four out of five cases look like this:

  return OpenInternal(aUrl, aName, aOptions,
                      true,                     // aDialog
                      false,                    // aContentModal
                      true,                     // aCalledNoScript
                      false,                    // aDoJSFixups
                      true,                     // aNavigate
                      nullptr, aExtraArgument,  // Arguments
                      nullptr,                  // aLoadState
                      false,                    // aForceNoOpener
                      PrintKind::None, _retval);

In these cases we can see it passed in explicitly as PrintKind::None. The exception is this call here:

      auto printKind = aForWindowDotPrint == IsForWindowDotPrint::Yes
                           ? PrintKind::WindowDotPrint
                           : PrintKind::InternalPrint;
      aError = OpenInternal(u""_ns, u""_ns, u""_ns,
                            false,             // aDialog
                            false,             // aContentModal
                            true,              // aCalledNoScript
                            false,             // aDoJSFixups
                            true,              // aNavigate
                            nullptr, nullptr,  // No args
                            nullptr,           // aLoadState
                            false,             // aForceNoOpener
                            printKind, getter_AddRefs(bc));

Crucially, this route is found inside the following method:

Nullable<WindowProxyHolder> nsGlobalWindowOuter::Print(
    nsIPrintSettings* aPrintSettings, nsIWebProgressListener* aListener,
    nsIDocShell* aDocShellToCloneInto, IsPreview aIsPreview,
    IsForWindowDotPrint aForWindowDotPrint,
    PrintPreviewResolver&& aPrintPreviewCallback, ErrorResult& aError) {
[...]

In other words, only a call to Print() will result in a value other than PRINT_NONE being passed in for aPrintKind and consequently the only way isForPrinting can be set to true.

We've followed all of the paths from beginning to end and hopefully it's clear that only this printing route will end up generating any hidden tabs. That's what we need. But in the future we'll also have to stay vigilant to the fact that this is possible; it's important we don't open any paths that allow it to be controlled by JavaScript coming from external sites.

My plan was to continue on today to looking into the User Agent strings, but this is already rather a long post so I'll leave it there for today. It remains to thank rob_kouw once again for the excellent question, as well as the useful and always-appreciated input from thigg.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.