External loads needlessly force creating an about:blank content viewer with confusing (wrong?) unload semantics

NEW
Unassigned

Status

()

enhancement
P3
normal
8 months ago
7 months ago

People

(Reporter: Gijs, Unassigned)

Tracking

(Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 affected)

Details

(Whiteboard: [qf:p3])

Reporter

Description

8 months ago
In nsDocShell::InternalLoad, https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/docshell/base/nsDocShell.cpp#9640-9659


  // Before going any further vet loads initiated by external programs.
  if (aLoadType == LOAD_NORMAL_EXTERNAL) {
    loadFromExternal = true;
    // Disallow external chrome: loads targetted at content windows
    bool isChrome = false;
    if (NS_SUCCEEDED(aURI->SchemeIs("chrome", &isChrome)) && isChrome) {
      NS_WARNING("blocked external chrome: url -- use '--chrome' option");
      return NS_ERROR_FAILURE;
    }

    // clear the decks to prevent context bleed-through (bug 298255)
    rv = CreateAboutBlankContentViewer(nullptr, nullptr);
    if (NS_FAILED(rv)) {
      return NS_ERROR_FAILURE;
    }

    // reset loadType so we don't have to add lots of tests for
    // LOAD_NORMAL_EXTERNAL after this point
    aLoadType = LOAD_NORMAL;
  }


The CreateAboutBlankContentViewer call is indeed from bug 298255.

I'm pretty sure we could avoid doing this for everything except inherited URLs. We might also just want to safe-list the schemes we allow if we're worried about other vectors here (e.g. to just http/https/ftp/file ) instead of only blocking chrome: (e.g. what about moz-extension URIs or about: URLs). In fact, it looks like slightly earlier in InternalLoad (line 9361 in above link) we already avoid inheriting principals for external loads.

Purely from a very quick read of the code, it looks like we could not be sending beforeunload events in this case, because we'll replace the document with an about:blank one (without passing options to fire beforeunload to CreateAboutBlankContentViewer), and only then replace the document with the URI we're actually loading, and presumably then call PermitUnload() on the about:blank doc we've just created, which will be more or less a no-op...

Anyway, the main reason I filed this is that (at a glance) it seems like it'll needlessly slow down loading stuff from external programs.
Reporter

Updated

8 months ago
Blocks: 1493606

Updated

8 months ago
Whiteboard: [qf] → [qf:p3:f67]

Updated

8 months ago
Priority: -- → P3
> I'm pretty sure we could avoid doing this for everything except inherited URLs.

We would need to double-check that we don't pick up referrers from the currently loaded page or something.  But if that true, then you're probably right.   And even there, we don't really need to throw in the about:blank.  Just make sure to not inherit the principal for the LOAD_EXTERNAL case, which as you note we already do (bug 304690 came after bug 298255, and was needed for the reasons described in bug 304690 comment 17).

That said, we could still have some confusion here.  For example, say https://trusted.com is loaded and we get an external load for https://evil.com that leads to a helper app prompt.  What will the user see in terms of where the content being handed to the helper app comes from?

> it looks like we could not be sending beforeunload events in this case

Seems plausible.

> it seems like it'll needlessly slow down loading stuff from external programs.

What is the current Firefox behavior for this case?  By default we're opening a new tab anyway, right?  And then doing this additional about:blank, then doing the normal load?  If so, I agree that this is rather silly...

Updated

7 months ago
Whiteboard: [qf:p3:f67] → [qf:p3]
You need to log in before you can comment on or make changes to this bug.