1284395 - (CVE-2017-5420) Show about:blank (placeholder "Search or enter address" in the URL bar) (spoof)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Abdulrahman Alqabandi]

Attachment: PoC.html

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36

Steps to reproduce:

Open PoC.html and follow instructions


Actual results:

Ended up with completely empty address bar


Expected results:

Main problem I think is the ability to be able to navigate to 'about:home'

So my initial expectation is that about: pages which are

a) Openable by content
b) Have an empty address bar by default

Should not be openable by content.
(So far im only aware of about:home that does this, 'about:newtab' does not meet requirement [a])


Note:

As far as I can tell, this is not a dupe of Bug 1247968 (not fixed) or Bug 1228754 (fixed) but its worth making sure it isn't

Comment 1

[Abdulrahman Alqabandi]

Works on latest nightly and latest stable

Comment 2

[Abdulrahman Alqabandi]

Attachment: New and improved PoC

It does not seem like about:home is needed at all.. So that's not the culprit

This PoC does not need you to manually hit back

Comment 3

[:Gijs (he/him)]

So the way this works is, AFAICT:

a) javascript: URL gets opened in a new tab. At this stage, document.location and document.documentURI both reflect the JS URI;
b) the JS URI returns a string of HTML containing a <script> that calls location.reload(). The reload gets triggered and runs the same JS URI again. When the same JS runs after that happens, both |location| and |documentURI are now about:blank (???). Note that this doesn't happen if the JS URI calls location.reload() directly. The indirection via the "return value" of the JS URI is somehow important.
c) the JS URI goes forward to a data: URI and then back from there.
d) now the location bar displays about:blank. That is also the principal of the page. Also, the tab spinner is permanently in the 'busy' state.

FWIW, I get the same results without steps (c) and (d), so a simpler testcase looks something like this:

data:text/html,<a href="javascript:if(history.length==0){'<script>location.reload()</script>'}else{document.write('<b>Spoof</b>');}">Please-open-in-new-tab.</a>

I think this is basically an issue in DOM/docshell-land. bz and smaug are both out.

There's a number of strange things here:

1) I'd expect the initial JS URI to load against a page that's inherited the principal of the page from which we clicked the link. That might be something that we need to fix in frontend (for dealing with non-left-content-clicks) if it is fixable at all. Not entirely sure how. Mike, I don't suppose you have any ideas?
2) The location.reload() from the 'response of the js URI seems to be running against an "about:blank" document. I've tried reading the spec, but "Setting the document's address" ( https://html.spec.whatwg.org/multipage/browsers.html#set-the-document%27s-address ) has no internal references and so I don't understand at what point that's supposed to apply to the Document we create to process the HTML 'response' from the JS URI. I would argue it should be before script runs on the document, or any other requests to external resources are made... and that doesn't seem like it's happening. Anne, can you elucidate what the spec says we should be doing here?

3) we show a placeholder for about:blank in the location bar. A short-term mitigation would be to just stop doing that for about:blank, but I don't know that the severity of this bug warrants doing that, nor do I think it's really meaningful for users in the first place... Marco, thoughts?

Comment 4

[Anne (:annevk)]

Few things:

1. javascript URLs are defined inline in the navigation algorithm.
2. javascript URLs generate a response and set an override URL for that response. The override URL is that of the browsing context (the one being navigated) its active document.
3. Reload just navigates to the same URL.

So given that, ending up with no URL seems like a bug.

Comment 5

[Abdulrahman Alqabandi]

If we do this:

data:text/html,<a href="javascript:if(history.length==0){'<script>location.reload()</script>'}else{if(history.length==2){alert(location)}else{location=`https://www.facebook.com/favicon.ico`}}">Please-open-in-new-tab.</a>

It almost looks like we got the content from facebook, but I can't seem to find any way to read any of it, it looks all frozen. Is there potential in this?

Also, if we do a window.stop() we can fix the infinite loading + some other things and we get a rough PoC of social engineering. (note that in this case the bug where favicon is not refreshed helps a lot to sell it)

data:text/html,<a href="javascript:if(history.length==0){'<script>location.reload()</script>'}else{if(history.length==2){document.write(`<b>Password:</b><input/type=text>`);window.stop();document.title=`Facebook Login`}else{alert(`Navigate back if its not found`);location=`https://www.facebook.com/temp.ico`}}">Please-open-in-new-tab.</a>

Also side note, sometimes nightly freezes on me when testing this area. Not sure if related, cant repro.

Comment 6

[Abdulrahman Alqabandi]

Note: in the above PoC you need to manually navigate back to see full effect.

Comment 7

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #3)
> 3) we show a placeholder for about:blank in the location bar. A short-term
> mitigation would be to just stop doing that for about:blank, but I don't
> know that the severity of this bug warrants doing that, nor do I think it's
> really meaningful for users in the first place... Marco, thoughts?

From an UX point of view, it seems important to me that we give hints to the user about what he can do on a completely empty page.  Honestly I'm not the best person to sign-off on removing the placeholder from about:blank, off-hand I'd say we should not remove it.
Here the problem is that this is not a really empty page, we seem to keep hitting this problem where about:blank is not really about:blank, but I don't know well enough the platform code handling this to be able to tell if there's an easy way to distinguish those cases. I was hoping bug 1228754 had helped with that.

Comment 8

[:Gijs (he/him)]

(In reply to Abdulrahman Alqabandi[test] from comment #6)
> Note: in the above PoC you need to manually navigate back to see full effect.

All of these still show an empty location bar for me, not one with fb.com in it, when the attacker-controlled page is displayed (rather than facebook.com's actual icon or whatever). Am I misunderstanding how this is supposed to work?

Comment 9

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #7)
> Here the problem is that this is not a really empty page, we seem to keep
> hitting this problem where about:blank is not really about:blank, but I
> don't know well enough the platform code handling this to be able to tell if
> there's an easy way to distinguish those cases. I was hoping bug 1228754 had
> helped with that.

I think it did help, just with different cases. :-)

AIUI per comment #4 this is just a platform bug. We shouldn't end up with a document that has an about:blank URI and principal.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

I'm afraid I'm not likely to be of much use here in the short term. If bz is unavailable, I could go deeper, but he's probably the shortest path to an answer.

Comment 11

[Abdulrahman Alqabandi]

Attachment: screenshot.png

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Abdulrahman Alqabandi[test] from comment #6)
> > Note: in the above PoC you need to manually navigate back to see full effect.
> 
> All of these still show an empty location bar for me, not one with fb.com in
> it, when the attacker-controlled page is displayed (rather than
> facebook.com's actual icon or whatever). Am I misunderstanding how this is
> supposed to work?

Location bar is still empty but (to me at least) it looks like a frozen copy of the content still exists, including the title.

Comment 12

[:Gijs (he/him)]

Boris, any idea what's going on here?

Comment 13

[Abdulrahman Alqabandi]

Reading through the last bug with the same results, is it the case that if about:blank has an opener then it will display 'about:blank' visibly? But using the middle mouse button, there is no opener thus the about:blank we end up in is hidden.

I also mentioned that about:blank should always reflect a blank document, and if it has an opener then the location should change from about:blank to the openers url. If no opener exists then you should not be able to perform document.write(). Not sure if that will break anything though.

Comment 14

[:Gijs (he/him)]

(In reply to Abdulrahman Alqabandi[test] from comment #13)
> Reading through the last bug with the same results, is it the case that if
> about:blank has an opener then it will display 'about:blank' visibly? But
> using the middle mouse button, there is no opener thus the about:blank we
> end up in is hidden.

Yes, this is (1) in comment #3, kind of (ie we should have an opener / inherited principal for the middle/modifier+left mouse click).

But we shouldn't end up in an about:blank in the first place, we should keep the JS URI around, per comment #4 / item (2) in comment #3.

> I also mentioned that about:blank should always reflect a blank document,
> and if it has an opener then the location should change from about:blank to
> the openers url.

We could consider doing this, but it'd probably require some consensus with other browser makers and would provide "interesting" user behaviour as well (e.g. what if they go to the URL in the URL bar and hit enter? Do we load the opener or the current page? It's kind of bizarre to have a page that doesn't correspond to the URL we show in a direct sense.

> If no opener exists then you should not be able to perform
> document.write(). Not sure if that will break anything though.

I don't know that we have a way of freezing the DOM. In any case, I would expect that to break the web, just because the web is crazy. Not super-clear how this would work anyway - what happens to data: and javascript: URIs and assigning those to document.location? Well, you say, those aren't about:blank - and now we're back on the initial point: I don't think we should have a document here with an about:blank-only principal that's controlled by the JS URI.

Comment 15

[Abdulrahman Alqabandi]

Shouldn't this be sec-moderate since Bug 1228754 was also?

Comment 16

[Boris Zbarsky [:bzbarsky]]

In terms of the comment 3 behavior, here's what's going on.

1) The javascript: URL gets opened in a new tab.  Arguably this is already a bug; note that there is no "open in new tab" context menu item for javascript: links, and I don't think we should open them in new tab for modifier-click either.  Anyway, we start loading the javascript: URI via nsDocShell::LoadURIWithOptions.  This does not pass in an "owner" and doesn't say an explicit owner is required, so in the nsIDocShellLoadInfo version of nsDocShell::LoadURI we do:

  inheritOwner = nsContentUtils::LegacyIsCallerChromeOrNativeCode();

In this case that sets inheritOwner to true.  This is a javascript: URI, so does in fact want an inherited principal in general, so we call nsDocShell::GetInheritedPrincipal.  This returns the principal of the document in the newly created tab where we're doing the load, which is a codebase principal for "about:blank".  We go ahead and run the script against the about:blank document in the new tab and it produces a document in the form of a string return value.

2) When a javascript: URL produces a document, we give it a URL that is the javascript: URL involved.  This is actually NOT what the HTML spec says to do; it says to give it a URL that is the URL of the document the javascript: is running against, iirc.

3) Anyway, now the script does a location.reload() (though I'm not sure _why_ history.length == 0 at this point!).  This performs a history navigation to a javascript: URI.  we have a special case in our history navigation code: when the navigation is to a javascript: URI we will create an about:blank document to run it against.  The reason for that is as follows.  Say I load site A, which loads a javascript: URI which produces a document which then links to site B which is known to do a history.back().  When the history.back() happens, we do NOT want to run the javascript: script against site B!  So in practice we throw our hands up in the air and CreateAboutBlankContentViewer to run the javascript: against.  Note that if we followed the HTML spec here that would amount to throwing our hands up in the air in a different way; we'd simply load the document the javascript: URL had run against and not run the javascript: at all.

4) Anyway, the javascript: runs a second time but this time history.length == 1, so it doesn't return a string.  Instead it navigates to a data: URL and then goes back and runs yet another branch that does a document.write.  As comment 3 points out this part isn't really that relevant.  It's just ending up at about:blank, then doing a document.write.  The spinner is going because we did document.write on an already-loaded document and then didn't document.close().

Anyway, from Gecko's point of view none of this is fundamentally different from this testcase:

  data:text/html,<div onclick="var w = window.open('about:blank'); w.setTimeout('document.write(&quot;<b>Is this a spoof?</b>&quot;)', 1000);">Click me</div>

This testcase shows "about:blank" in the URL bar, though...  Why doesn't the other?

Responding to the list of "strange things" in comment 3:

> I'd expect the initial JS URI to load against a page that's
> inherited the principal of the page from which we clicked the link.

Gecko isn't being told anything about any of that.  It's just being told to run a javascript: URL against the newly create tab, which just has an about:blank inside it so far.

> The location.reload() from the 'response of the js URI seems to be running against an "about:blank" document. 

So is the original execution, of course.  Anyway, see above for why this is happening, along with the differences between what we do and what the spec says to do.  In any case, per spec all the reload() would do is load about:blank and not run any more code.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Oh, and note that the javascript: URL in question _sometimes_ creates a new document (by returning a string) and sometimes does _NOT_.  Which is why the URL is sometimes "javascript:stuff" and sometimes "about:blank": the latter is when the script execution did not create a new document.

Comment 18

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #16)
> In terms of the comment 3 behavior, here's what's going on.
> 
> 1) The javascript: URL gets opened in a new tab.  Arguably this is already a
> bug; note that there is no "open in new tab" context menu item for
> javascript: links, and I don't think we should open them in new tab for
> modifier-click either.

I expect we do this because having literally nothing happen when you middle/alt/cmd-click a link is counterintuitive, as is having something happen in the current tab (if you were trying to make sure that didn't happen). It's hard to be consistent with user expectations for js links.

The other thing I noticed is that the context menu determination is made in http://searchfox.org/mozilla-central/rev/c44d0b1673fef5e0e2e19fa82d6780a74c186151/browser/base/content/nsContextMenu.js#942-951 .

It doesn't include data: URIs which I imagine would have the same problem (but maybe not because they can't not "return" a document in the same way js URIs can?).

> Anyway, we start loading the javascript: URI via
> nsDocShell::LoadURIWithOptions.  This does not pass in an "owner" and
> doesn't say an explicit owner is required, so in the nsIDocShellLoadInfo
> version of nsDocShell::LoadURI we do:
> 
>   inheritOwner = nsContentUtils::LegacyIsCallerChromeOrNativeCode();
> 
> In this case that sets inheritOwner to true.

This seems wrong. We should make sure that our chrome-js-based thing that deals with loading web content in tabs/windows based on clicks doesn't trigger 'caller is chrome'... :-\
Is there a "manual" way to avoid doing this?

>  This is a javascript: URI, so
> does in fact want an inherited principal in general, so we call
> nsDocShell::GetInheritedPrincipal.  This returns the principal of the
> document in the newly created tab where we're doing the load, which is a
> codebase principal for "about:blank".  We go ahead and run the script
> against the about:blank document in the new tab and it produces a document
> in the form of a string return value.

Is there something chrome JS (which is what's handling the click) can do here to make it inherit the principal of the linking page instead? Pass a principal to loadURI or whatever? Should we manually call createAboutBlankContentViewer (yay xpcom) on the docshell (with the originating page's principal as the argument) and then load the js/data URI? Feels like there should be a better way...

> 2) When a javascript: URL produces a document, we give it a URL that is the
> javascript: URL involved.  This is actually NOT what the HTML spec says to
> do; it says to give it a URL that is the URL of the document the javascript:
> is running against, iirc.
> 
> 3) Anyway, now the script does a location.reload() (though I'm not sure
> _why_ history.length == 0 at this point!).

Yeah, this might be a separate bug. I'd like to sort out the spoof / security stuff first though - you could accomplish the same branching in other ways, I expect.

>  This performs a history
> navigation to a javascript: URI.  we have a special case in our history
> navigation code: when the navigation is to a javascript: URI we will create
> an about:blank document to run it against.  The reason for that is as
> follows.  Say I load site A, which loads a javascript: URI which produces a
> document which then links to site B which is known to do a history.back(). 
> When the history.back() happens, we do NOT want to run the javascript:
> script against site B!  So in practice we throw our hands up in the air and
> CreateAboutBlankContentViewer to run the javascript: against.  Note that if
> we followed the HTML spec here that would amount to throwing our hands up in
> the air in a different way; we'd simply load the document the javascript:
> URL had run against and not run the javascript: at all.

Where does this live, and can we make it just create the about:blank viewer and then do nothing? Feels like that wouldn't break anything we care about, I hope... (maybe vainly... the web is weird?)

> Anyway, from Gecko's point of view none of this is fundamentally different
> from this testcase:
> 
>   data:text/html,<div onclick="var w = window.open('about:blank');
> w.setTimeout('document.write(&quot;<b>Is this a spoof?</b>&quot;)',
> 1000);">Click me</div>
> 
> This testcase shows "about:blank" in the URL bar, though...  Why doesn't the
> other?

This testcase will have browser.hasContentOpener be true (ie window.opener is non-null), in which case we show about:blank: 

http://searchfox.org/mozilla-central/rev/c44d0b1673fef5e0e2e19fa82d6780a74c186151/browser/base/content/browser.js#6451

The one from comment #0 / this bug doesn't because the new window is not generated by web content / window.open, but by the browser "manually" opening a new tab/window/whatever and loading the URL in there.

> > The location.reload() from the 'response of the js URI seems to be running against an "about:blank" document. 
> 
> So is the original execution, of course.  Anyway, see above for why this is
> happening, along with the differences between what we do and what the spec
> says to do.  In any case, per spec all the reload() would do is load
> about:blank and not run any more code.

Right. I think this is identical to what I asked earlier. Is this difficult to do?

Feels like the following could be improved:
1) make browser code tell gecko something that'd set up the right principal to begin with
2) make browser code tell gecko something that'd pass the original tab as 'owner' - maybe? What does that do, and do we want it? (e.g. don't want opening link from mozilla.org to google.com in a new tab give google.com a ref to mozilla.org as 'opener' so it can navigate that tab - maybe it should only happen for inherit_principal URIs, and only if not opening in a different container / private browsing context ?)
3) browser code loading links from the web in tabs shouldn't trigger 'is chrome' caller checks... I think? Maybe I'm being too paranoid?
4) reloading js URIs should just load about:blank
5) can browser code tell gecko something that'd make inherit_principal URIs have a window.opener when opened in a new tab/window that isn't in a different container / pb context?

Because of private browsing / containers, I think (2) and (5) might not necessarily be sufficient/helpful. I'm not sure (3) even makes sense. So I *think* doing both (1) and (4) is our best chance for fixing the spoofing in that we should be able to tell we can't show a clear location bar where remote web content controls the page (and perhaps, in the future, we can show an origin'd thing instead of the string "about:blank"). We need (1) because otherwise, AIUI, we might still not show the right thing if there aren't any location.reload()s or whatever. We need (4) because otherwise the location.reload does something wonky. Is that right? Am I missing something or is my reasoning broken in places (probably!)?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> It doesn't include data: URIs which I imagine would have the same problem 

Not entirely, because they just produce a document and don't involve interacting with another one to do so, unlike javascript:.  There are literally zero use cases I can think of for taking a javascript: link in a page and running it against a new about:blank tab....

> We should make sure that our chrome-js-based thing that deals with loading
> web content in tabs/windows based on clicks doesn't trigger 'caller is chrome'

That's a bit tough.  Also, why would you want that?

Anyway, in this case the only practical impact would be to not inherit the owner, so you can just pass LOAD_FLAGS_DISALLOW_INHERIT_OWNER to accomplish that in this case, I would think.

> Is there something chrome JS (which is what's handling the click)
> can do here to make it inherit the principal of the linking page instead?

Looks like right now the answer is "no" for the load directly, because nsIDocShell::loadURI is noscript.  We could add another scriptable API on nsIDocShell or nsIWebNavigation that allowed one to do this.

However note that in this case what that would simply make the javascript: not run because its principal would not match that of the about:blank involved so Gecko would disallow its execution.  Maybe that's fine, though.  Again, the use cases for modifier-click on a javascript: URL seem pretty slim to me.

> Should we manually call createAboutBlankContentViewer (yay xpcom) on
> the docshell (with the originating page's principal as the argument)

That would work, yes.  I agree it's a bit action-at-a-distance.

> Where does this live, and can we make it just create the about:blank viewer and then do nothing?

Where does which live?  The history traversal to javascript: hack?  That's at http://searchfox.org/mozilla-central/rev/c44d0b1673fef5e0e2e19fa82d6780a74c186151/docshell/base/nsDocShell.cpp#12314

> maybe vainly... the web is weird?

Indeed.  If we're going to change anything about this stuff, I'd rather we just aligned the URI of the document for the result string of javascript: with the spec and other browsers.

> This testcase will have browser.hasContentOpener be true (ie window.opener is non-null)

Should that be true for things resulting from modifier-click too?

> Is this difficult to do?

I assume you mean aligning with the spec in terms of the URL that the javascript: result document has.  Apart from some thinking about security and auditing all our code for places where we check for "javascript" as a URL, probably not that hard, really.  We'd basically have to treat it more as an internal redirect or something in the jschannel (i.e. actually send redirect notifications).  That might have problems; not sure.

> What does that do, and do we want it?

It [the owner] sets the principal to use for things that inherit principal (javascript: and data:, mostly).  It doesn't involve having a window.opener backref.

> 4) reloading js URIs should just load about:blank

Again, the right thing here is to never have js URIs as a thing you can "reload".

> can browser code tell gecko something that'd make inherit_principal URIs
> have a window.opener when opened in a new tab/window 

So basically treat modifier-click as more like "the link had target=_blank"?

Comment 20

[:Gijs (he/him)]

Preamble in case this is helpful: here are the parameters with which I'm trying to work:
A) user manually typing about:blank in the location bar (proxy for: setting about:blank as their new tab page) should result in an empty location bar
B) any web-controlled about:blank page should not result in an empty location bar

I'm bearing in mind that:
- window.open(...) is one of the cases where web content can control about:blank, but AIUI even besides this bug there are other such cases. That is, it is possible for web content to have a content-controlled toplevel about:blank document without that document having an opener (am I wrong in the latter respect?)
- window.opener can be used for evil, there are non-trivial rules governing when it's set, and per the previous point, it's unreliable as an indicator of whether about:blank is controlled by content (see also bug 1228754) so it's not sufficient for determining that such a document exists.


AIUI the problem in this case is that browser code is treating the testcase as (A) when it should be treating it as (B).

The reason for this is that the document principal of the loaded document after the js-triggered location.reload() has the URI of about:blank, the document has no opener, and the document URI (docshell.currentURI / content.document.documentURI) is also about:blank. We need to make sure that one of these things happens:

1) we don't let you open the js URL this way
2) the principal's URI and the document URI should not both be about:blank - both before and after the reload()
3) the principal's URI and the document URI should not both be about:blank before the reload and (i) the reload is disallowed or a no-op; or (ii) the reload results in a non-web-controlled about:blank loading.

1) is hard from my perspective because it's a behaviour change that has UX implications, and not one I feel I could just make unilaterally (whatever we do - Safari does nothing, and Chrome opens about:blank, haven't checked IE/Edge). We'd need to consult UX about what to do and that's a whole general debate about what to do with the js URL, which is ancient (bug 138198), and I'm worried we'll get stuck in weeds trying to just get anything that's better than the current solution while people try and get The Perfect Thing (whatever that is). So I'd prefer a more short-term fix, but if we think that just removing support is easier I can file a public bug and try to poke UX folks.

I don't know which is easier between 2 and 3 and/or if I'm misunderstanding the problem completely.



(In reply to Boris Zbarsky [:bz] from comment #19)
> > Is there something chrome JS (which is what's handling the click)
> > can do here to make it inherit the principal of the linking page instead?
> 
> Looks like right now the answer is "no" for the load directly, because
> nsIDocShell::loadURI is noscript.  We could add another scriptable API on
> nsIDocShell or nsIWebNavigation that allowed one to do this.
> 
> However note that in this case what that would simply make the javascript:
> not run because its principal would not match that of the about:blank
> involved so Gecko would disallow its execution.  Maybe that's fine, though. 
> Again, the use cases for modifier-click on a javascript: URL seem pretty
> slim to me.

My suggestion was that the about:blank should have the linking page's principal, and that the JS should inherit principal from that about:blank-with-linking-page's-principal. In which case I'd expect it to "work". Am I missing something?

And I agree the usecases are slim. If this is a giant engineering project we should come up with a simpler solution. If it's a few lines of code and it fixes this bug, good enough for me.

> Indeed.  If we're going to change anything about this stuff, I'd rather we
> just aligned the URI of the document for the result string of javascript:
> with the spec and other browsers.

So, to be clear, this is saying the URI of the document of the `<script>location.reload()</script>` and the result of the reload should all be the javascript: URI?

> > This testcase will have browser.hasContentOpener be true (ie window.opener is non-null)
> 
> Should that be true for things resulting from modifier-click too?

"It depends". Likely not for e.g. noreferrer/noopener and maybe others. I don't think we can rely on it exclusively. :-(

> > Is this difficult to do?
> 
> I assume you mean aligning with the spec in terms of the URL that the
> javascript: result document has.  Apart from some thinking about security
> and auditing all our code for places where we check for "javascript" as a
> URL, probably not that hard, really.  We'd basically have to treat it more
> as an internal redirect or something in the jschannel (i.e. actually send
> redirect notifications).  That might have problems; not sure.

So I'm confused. Looking at clicking the link in:

data:text/html,<a href="javascript:if(true){'<p>Hi</p>'}">click</a>

the document.documentURI of the resulting tab is already javascript:... (and so the location bar shows the js URI rather than about:blank, as the document's principal is about:blank but the document's URI isn't)

That seems correct per what you're saying / the spec. Right? So do we "just" need to make reload() not work / produce a 'plain' non-web-controlled about:blank / produce a document with a JS URI too? Am I still missing the point?

> > 4) reloading js URIs should just load about:blank
> 
> Again, the right thing here is to never have js URIs as a thing you can
> "reload".

What happens in this case, then? location.reload throws an exception? Or something else?

> > can browser code tell gecko something that'd make inherit_principal URIs
> > have a window.opener when opened in a new tab/window 
> 
> So basically treat modifier-click as more like "the link had target=_blank"?

Well, it'd only work for new tab/window with the same container / private browsing, and I'd probably want to limit it to inherit_principal URIs. With those caveats, yes. Because of the caveats, not sure that fixes the issue fully.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> it is possible for web content to have a content-controlled toplevel
> about:blank document without that document having an opener

Sure.  Consider a page A that window.opens page B (same-origin with A).  Then B does opener.location.href = "about:blank".

I was going to suggest that we simply check whether the principal URI is "about:blank".  But of course the whole point of this bug is that this is exactly the situation we end up in, because we execute a web-provided javascript: URI against a browser-created about:blank document.

> 2) the principal's URI and the document URI should not both be about:blank -
> both before and after the reload()

The reload() call preserves the principal's URI.  So if it's not about:blank before the reload(), it won't be after either.

> I don't know which is easier between 2 and 3 and/or if I'm misunderstanding the
> problem completely.

I think your understanding of the problem is correct.

Right now, the principal's URI and document URI can both be trivially about:blank before the reload, or indeed without any reload.  Just do this:

  data:text/html,<a href="javascript:document.write('spoof'); void(0);">Middle-click me</a>

Given that, we can aim to fix the pre-reload state principal or the pre-reload state URL.  The reload preserves the former and would preserve the latter if it were not "javascript:".  In the short term, I expect simply using a non-about:blank principal for the pre-reload state is the simplest thing, but that would need to be done in the frontend...  Longer term... well, longer-term once we changed to follow the spec for URLs resulting from javascript: evaluation the URL will be that of the document the evaluation happened in, so it would be about:blank.  So the principal change is the only one that fixes things here.

> My suggestion was that the about:blank should have the linking page's principal

That should work, yes.

> So, to be clear, this is saying the URI of the document of the
> `<script>location.reload()</script>` and the result of the reload should all be
> the javascript: URI?

No, they should all be "about:blank" per spec.  The only thing it would fix is that the reload() would actually load about:blank and not run any script on it.  But as my example above shows, we don't even need the reload().

> data:text/html,<a href="javascript:if(true){'<p>Hi</p>'}">click</a>
> the document.documentURI of the resulting tab is already javascript:

Per spec it shouldn't be.

> What happens in this case, then?

The spec has no concept of a document with a "javascript:" URI.  There is simply no such thing.  A "javascript:" URI, in the spec, is just something that gets handled in a special hack in the navigation code, and produces a document with some other URI, if it produces a document at all.

Comment 22

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #21)
> Given that, we can aim to fix the pre-reload state principal or the
> pre-reload state URL.  The reload preserves the former and would preserve
> the latter if it were not "javascript:".  In the short term, I expect simply
> using a non-about:blank principal for the pre-reload state is the simplest
> thing, but that would need to be done in the frontend...  

I'll try and look at this in the near-ish future, then. Thanks!

Comment 23

[:Gijs (he/him)]

Attachment: Patch v0.1

I believe this works.

The gist of it is that for content clicks and context menu opening of links, we pass the principal of the page with the link to openLinkIn, and then all the way down into addTab code, and then we use that code to inherit the principal of the linking page into the about:blank viewer we create.

The net result is that we display "about:blank" in the URL bar for the testcase on this bug, when we previously cleared the URL bar.

bug 1297008 would independently change what we do in these cases to display a/the URI associated with the principal that controls the about:blank viewer, but is orthogonal to this patch.

I'm also not trying to address issues with the 'spinner' in the tab, which I suspect could be dealt with in a non-sec-sensitive bug with non-"malicious"-JS.


I'm not very happy with the complexity the patch adds, but I suspect we might be wanting the principal stuff anyway for things like bug 1289097. I'd be curious if this looks right to you both, Boris and Mike.

NB: in case it wasn't obvious from the patch: the window opening (rather than tab) path ends up calling loadURI to call openLinkIn to open a link in the current tab, and to avoid always creating a new about:blank viewer in that tab, I'm specialcasing the case where this is triggered through window opening to create said new content viewer. I hope this makes sense.

Comment 24

[:Gijs (he/him)]

Orthogonal: the context menu options to open links in a new tab/window/whatever is not available for javascript: links.

Perhaps a simpler solution would be to make middle/modifier-clicks on such links no-op? Though that feels like it has a UX impact, and could potentially break actual usecases (though they'd be a little byzantine).

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8788460 [details] [diff] [review]
Patch v0.1

>+      json.originPrincipal = ownerDoc.nodePrincipal;

How is that supposed to work?  Is the thing being sent not really json?

Modulo that, r=me on the general idea, but I don't know enough about the details of these codepaths to say something intelligent.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8788460 [details] [diff] [review]
Patch v0.1

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

::: browser/base/content/content.js
@@ +502,5 @@
>            sm.checkSameOriginURI(docshell.mixedContentChannel.URI, targetURI, false);
>            json.allowMixedContent = true;
>          } catch (e) {}
>        }
> +      json.originPrincipal = ownerDoc.nodePrincipal;

AIUI this is all structured clone stuff for e10s, which should be able to deal with serializing and deserializing nsIURI and nsIPrincipal objects... mconley should be able to confirm?

Separately, I think this assignment might need to be outside (before) this particular if block to cover the 'middle click as paste to navigate' case as well.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8788460 [details] [diff] [review]
Patch v0.1

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

Thanks Gijs. Would it be possible to add a regression test?

(In reply to :Gijs Kruitbosch from comment #26)
> Comment on attachment 8788460 [details] [diff] [review]
> Patch v0.1
> 
> Review of attachment 8788460 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/content.js
> @@ +502,5 @@
> >            sm.checkSameOriginURI(docshell.mixedContentChannel.URI, targetURI, false);
> >            json.allowMixedContent = true;
> >          } catch (e) {}
> >        }
> > +      json.originPrincipal = ownerDoc.nodePrincipal;
> 
> AIUI this is all structured clone stuff for e10s, which should be able to
> deal with serializing and deserializing nsIURI and nsIPrincipal objects...
> mconley should be able to confirm?

Yep - billm added this capability in bug 1032592.

::: browser/base/content/tabbrowser.xml
@@ +2081,5 @@
>              var evt = new CustomEvent("TabOpen", { bubbles: true, detail });
>              t.dispatchEvent(evt);
>  
> +            if (!usingPreloadedContent && aOriginPrincipal) {
> +              b.createAboutBlankContentViewer(aOriginPrincipal);

Hrm. Hopefully this new work doesn't burn us on some of our talos tests...

::: toolkit/content/widgets/browser.xml
@@ +1043,5 @@
> +      <method name="createAboutBlankContentViewer">
> +        <parameter name="aPrincipal"/>
> +        <body>
> +          <![CDATA[
> +            this.docShell.createAboutBlankContentViewer(aPrincipal);

We could alternatively use the shared browser-content.js so that both remote and non-remote browsers share the same codepath for this. I don't mind either way, tbh.

Comment 28

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - (Digging through needinfos and reviews) from comment #27)
> Comment on attachment 8788460 [details] [diff] [review]
> Patch v0.1
> 
> Review of attachment 8788460 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Gijs. Would it be possible to add a regression test?

Maybe. I'll have a look.

> (In reply to :Gijs Kruitbosch from comment #26)
> ::: browser/base/content/tabbrowser.xml
> @@ +2081,5 @@
> >              var evt = new CustomEvent("TabOpen", { bubbles: true, detail });
> >              t.dispatchEvent(evt);
> >  
> > +            if (!usingPreloadedContent && aOriginPrincipal) {
> > +              b.createAboutBlankContentViewer(aOriginPrincipal);
> 
> Hrm. Hopefully this new work doesn't burn us on some of our talos tests...

I would assume they don't go through the tab opening code path via content clicks.

Of course, where it does impact perf in those cases, this would still be a concern even if talos didn't measure it.

Boris, would it be safe to restrict this "create our own viewer" malarkey to cases where we know the URL is going to inherit, and if so, what's the correct way to determine that?

Comment 29

[Boris Zbarsky [:bzbarsky]]

You can determine that by checking for the URI_INHERITS_SECURITY_CONTEXT flag.  Though there's some weirdness around about:blank, which does not have that flag but _can_ inherit...

For whether it's safe...  I'm not sure what problem the createAboutBlankContentViewer() call is trying to solve, so can't tell you for sure.

Comment 30

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #29)
> You can determine that by checking for the URI_INHERITS_SECURITY_CONTEXT
> flag.  Though there's some weirdness around about:blank, which does not have
> that flag but _can_ inherit...

Good to know.

> For whether it's safe...  I'm not sure what problem the
> createAboutBlankContentViewer() call is trying to solve, so can't tell you
> for sure.

Well, this:

(In reply to :Gijs Kruitbosch from comment #22)
> (In reply to Boris Zbarsky [:bz] from comment #21)
> > Given that, we can aim to fix the pre-reload state principal or the
> > pre-reload state URL.  The reload preserves the former and would preserve
> > the latter if it were not "javascript:".  In the short term, I expect simply
> > using a non-about:blank principal for the pre-reload state is the simplest
> > thing, but that would need to be done in the frontend...  
> 
> I'll try and look at this in the near-ish future, then. Thanks!

In order to get a non-about:blank principal (for the javascript:foo URI we're loading in the new tab), I'm making the about:blank window (content viewer) inherit the principal of the page on which the user clicks the js:foo link. Does that make sense? The case we're trying to avoid is where js: or data: URIs inherit the principal of the new about:blank tab/window in which they're being opened, AIUI. Is there a better way to accomplish this? (ni for this)

But really, maybe it's worth doing some more digging here. In particular, I suspect (but haven't tested) that one can't currently do the same thing with a data: URI as the testcase here does, given sufficient messings-about (e.g. having the data URI set location.href to a javascript: URI which then does whatever it's doing here, etc.). There are some... odd... specialcases in the JS code here for javascript: URIs going through openLinkIn, which I suspect are from bug 1018154. I'll have to dig into it more next week. :-\

Comment 31

[Boris Zbarsky [:bzbarsky]]

OK.  So this is specific to the case when we have just opened a new tab, right?

In an ideal world, what we would do is just tell it what principal to use for its about:blank when we create the docshell.  But in practice, the createAboutBlankContentViewer call you have may well be fine, if it happens before we create the initial about:blank.  It's extra work if we do it _after_ that, but it's not really that much work compared to the general work to set up a new docshell...  and if we do care to optimize it we can add some way to hand the principal to the docshell before the initial about:blank is created.

Comment 32

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #31)
> OK.  So this is specific to the case when we have just opened a new tab,
> right?

Well, or a new (private/non-private) window. But more or less "yes", especially to "have just created / are just creating a new content docshell".

> In an ideal world, what we would do is just tell it what principal to use
> for its about:blank when we create the docshell.  But in practice, the
> createAboutBlankContentViewer call you have may well be fine, if it happens
> before we create the initial about:blank.

I... doubt it's before that, but OK.

>  It's extra work if we do it
> _after_ that, but it's not really that much work compared to the general
> work to set up a new docshell...  and if we do care to optimize it we can
> add some way to hand the principal to the docshell before the initial
> about:blank is created.

Fair. Let's just try how this goes and we can see if anybody (esp. perf test infra) notices anything.

I still need to write a regression test per comment #27.

Comment 33

[:Gijs (he/him)]

Attachment: Patch v0.2

Now with test. I also had to declare a variable in tabbrowser.xml that I missed (oops).

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8791412 [details] [diff] [review]
Patch v0.2

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

This looks okay to me. And I definitely think it makes sense to be passing the principal around like this anyways - I'm actually surprised we weren't doing that before.

::: browser/base/content/tabbrowser.xml
@@ +2098,5 @@
>              var evt = new CustomEvent("TabOpen", { bubbles: true, detail });
>              t.dispatchEvent(evt);
>  
> +            if (!usingPreloadedContent && aOriginPrincipal) {
> +              b.createAboutBlankContentViewer(aOriginPrincipal);

I wonder if this is going to cause any Talos regressions...

::: browser/base/content/test/general/browser_modifiedclick_inherit_principal.js
@@ +3,5 @@
> +const kURL =
> +  "http://example.com/browser/browser/base/content/test/general/dummy_page.html";
> +  "data:text/html,<a href=''>Middle-click me</a>";
> +
> +add_task(function* () {

Please add a comment at the top here about what exactly is being tested.

@@ +17,5 @@
> +   yield BrowserTestUtils.synthesizeMouseAtCenter("a", {button: 1}, browser);
> +   let newTab = yield newTabPromise;
> +   is(newTab.linkedBrowser.contentPrincipal.origin, "http://example.com",
> +      "Principal should be for example.com");
> +   yield BrowserTestUtils.removeTab(newTab);

Should we be testing the URL bar here as well?

Comment 35

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #34)
> Comment on attachment 8791412 [details] [diff] [review]
> Patch v0.2
> 
> Review of attachment 8791412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks okay to me. And I definitely think it makes sense to be passing
> the principal around like this anyways - I'm actually surprised we weren't
> doing that before.
> 
> ::: browser/base/content/tabbrowser.xml
> @@ +2098,5 @@
> >              var evt = new CustomEvent("TabOpen", { bubbles: true, detail });
> >              t.dispatchEvent(evt);
> >  
> > +            if (!usingPreloadedContent && aOriginPrincipal) {
> > +              b.createAboutBlankContentViewer(aOriginPrincipal);
> 
> I wonder if this is going to cause any Talos regressions...

I don't think so, because all of this is about what happens if code from the Firefox UI opens stuff. It doesn't happen for window.open and so on. So I don't think we'll hit this on Talos. I also don't think the hit will be serious enough to worry about.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6036b8acdab5

I'll CC jmaher in case we do see a talos hit on central and this patch is to blame. There are options to reduce the impact (like only doing this for URIs that inherit the principal (see comment #30)) but that might be tricky to get completely right when it comes to think like URI fixup and so on... so if we can avoid it, I would like not to bother. :-)

Comment 36

[:Gijs (he/him)]

So this is getting backed out because of the containers code which means we can't change origin attributes after the docshell has been created.

I'm not so interested in the origin attributes, I'm interested in the principal.

How do I get the same principal with origin attributes that make this code happy?

Comment 37

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #36)
> So this is getting backed out because of the containers code which means we
> can't change origin attributes after the docshell has been created.
> 
> I'm not so interested in the origin attributes, I'm interested in the
> principal.
> 
> How do I get the same principal with origin attributes that make this code
> happy?

Failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=36082765&repo=mozilla-inbound#L3461

to be very specific: given principal X and principal Y, I want a principal X' that has the same origin usercontextid as Y and is otherwise identical to X.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Is the setup that we have a link in a docshell with user context id A and then we "open link in new tab" it?  Ad the origin principal has user context id A?

If that's the situation, where is another user context id coming from?  Are we creating the new tab with a different user context id than A?

Comment 39

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (TPAC) from comment #38)
> Is the setup that we have a link in a docshell with user context id A and
> then we "open link in new tab" it?  Ad the origin principal has user context
> id A?
> 
> If that's the situation, where is another user context id coming from?  Are
> we creating the new tab with a different user context id than A?

The latter. Nightly has context menus with "open in a new container tab", and so that produces a tab with a different user context id (or should, anyway).

Comment 40

[:Gijs (he/him)]

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/97d4dcf688a0

Comment 41

[Boris Zbarsky [:bzbarsky]]

Ah, I see.

So for codebase principals , you can do nsIScriptSecurityManager.createCodebasePrincipal.  For a nullprincipal you can nsIScriptSecurityManager.nsIScriptSecurityManager.  For system principals, you're out of luck because those have no originattributes anyway.  Expanded principals should never be an origin principal here...

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged backout: https://hg.mozilla.org/mozilla-central/rev/6036b8acdab5

Not sure what that means for status flags in this bug.

Comment 43

[:Gijs (he/him)]

Attachment: Patch v0.3

I verified this avoids a crash. baku, can you sanity-check the utility function in utilityOverlay.js?

(I realize that this means that if you have a null-principal'd origin and you change the usercontextid when opening the link in a new tab, the link will no longer be same-origin. I believe that that would be the case as a result of the different usercontextid already, so I think that's OK.)

Trypush:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6de29bafe59

Comment 44

[Andrea Marchesini [:baku]]

Comment on attachment 8792848 [details] [diff] [review]
Patch v0.3

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

Sorry for the delay. looks good to me.

Comment 45

[:Gijs (he/him)]

Attachment: Patch v0.4

try is still orange, because it doesn't like opening private browsing windows with links, which have non-0 privateBrowsingIds and so assigning a 0 there makes it... unhappy.

So really, we should copy all the originAttributes from the initial about:blank, I think. That is slightly problematic with the approach in the previous patch which did this in the parent on e10s, which doesn't work because of bug 1304492.

Really, if createAboutBlankContentViewer called with the 'wrong' principal is always wrong, it seems there's little point in calling it with such principals, so I've pushed the responsibility for changing those down into the browser implementations with the utility function moving to BrowserUtils and taking a full-on second principal. Principal rather than just the OA so we can easily ask for originSuffix and determine if we need to actually bother to create a second principal.

(We should still fix bug 1304492, but that might get hairy and I would prefer not to hold up this change for its sake.)

Comment 46

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dca3eb50711

Comment 47

[Andrea Marchesini [:baku]]

Comment on attachment 8793491 [details] [diff] [review]
Patch v0.4

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

::: toolkit/modules/BrowserUtils.jsm
@@ +88,5 @@
> +      return principal;
> +    }
> +
> +    // If they're the same, just return the principal as-is.
> +    if (existingPrincipal.originSuffix == principal.originSuffix) {

existingPrincipal.equals(principal)

Comment 48

[:Gijs (he/him)]

(In reply to Andrea Marchesini [:baku] from comment #47)
> Comment on attachment 8793491 [details] [diff] [review]
> Patch v0.4
> 
> Review of attachment 8793491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/modules/BrowserUtils.jsm
> @@ +88,5 @@
> > +      return principal;
> > +    }
> > +
> > +    // If they're the same, just return the principal as-is.
> > +    if (existingPrincipal.originSuffix == principal.originSuffix) {
> 
> existingPrincipal.equals(principal)

This wouldn't be the case because their codebases will be different, no? In particular, the "whole point" of this patch is to get the new about:blank viewer that we're creating the inherited principal from example.com, and the existing principal is likely either for null or moz-safe-about:blank or whatever. The check here is if the *origin attributes* are the same - hence checking originSuffix, which AIUI is basically a serialization of the OA.

Comment 49

[Andrea Marchesini [:baku]]

> > existingPrincipal.equals(principal)

You are right. We must check only the originSuffix. I was confused by 'if they are the same'.

Comment 50

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8793491 [details] [diff] [review]
Patch v0.4

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

With baku's issue fixed and my docstring fixed, I think we're good to go here. Thanks Gijs!

::: toolkit/modules/BrowserUtils.jsm
@@ +81,5 @@
>        throw "Load of " + aURL + principalStr + " denied.";
>      }
>    },
>  
> +  principalWithMatchingOA(principal, existingPrincipal) {

Please add a docstring.

Comment 51

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a45cfe898352

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a45cfe898352

Comment 53

[:Gijs (he/him)]

Can this be opened up, given it's sec-low and fixed in 52esr?