1228754 - (CVE-2016-1958) Show about:blank (placeholder "Search or enter address" in the URL bar) using javascript URI scheme (spoof)

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Abdulrahman Alqabandi]

Attachment: FF-urlhider.html

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

View attached file 'FF-urlhider.html' and simply click the innocent looking anchor tag.


Actual results:

We end up in a completely empty URL address yet we control its documents content.


Expected results:

The problem here is that we are able to hide the URL from the user and make it seem like they visited a trusted website. Since we control the content displayed then this kind of bug could easily be used to perform social engineering/phishing attacks.

If the address bar is set to an absolute empty 'about:blank' (which displays nothing in the address bar except 'search or enter address' then its contents should reflect that and be empty, otherwise its a security risk.

Comment 1

[Abdulrahman Alqabandi]

Attachment: Screenshot of the resulting 'spoofed' page

Comment 2

[Andrew McCreight [:mccr8]]

I'm not sure if this is something going wrong with the displaying of the URL bar in the front end, or if it could be some kind of document navigation issue. mwobensmith said he confirmed this issue.

Comment 3

[Abdulrahman Alqabandi]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> I'm not sure if this is something going wrong with the displaying of the URL
> bar in the front end, or if it could be some kind of document navigation
> issue. mwobensmith said he confirmed this issue.

The key to this bug is the passed variable 'q', when we first click and we are directed to 'javascript:q;..etc' 'q' is defined as the anchor tag but after we do location.reload() (within the javascript scheme) the 'q' variable is no longer defined thus throwing and error and we end up in the blank url.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, so stepping through this carefully:

1)  There is a click on a javascript: URI (ignoring the obfuscation bit for the user).  This sets the page url to javascript:stuff, sets the favicon to the Google favicon using the <link rel="favicon">, sets the tab title via the document title.

2)  A timer is started to do location.reload().  But you can achieve the same effect by simply taking out that call from the testcase and clicking the reload button yourself, for convenience.

3)  In LoadHistoryEntry(), there is a CreateAboutBlankContentViewer call, with this comment:

12001       // We're loading a URL that will execute script from inside asyncOpen.
12002       // Replace the current document with about:blank now to prevent
12003       // anything from the current document from leaking into any JavaScript
12004       // code in the URL.

This was added to prevent back/forward to javascript: URLs from running script in the context of some random page.  But in this case the upshot is that we change the docshell's mContentViewer.  Note that we do NOT call Show() on it at this point, so we're still showing the previous document viewer.  This is exactly the state we would be in if we had started getting data for a new document but hadn't started layout on it yet.  Note that the docshell's current URI is about:blank at this point.

4) We go to run the javascript: URL.  The fact that it throws isn't even all that important.  You could wrap the whole thing in a try/catch and it would have the same behavior.  The important part is that it does not return a string, so doesn't produce a new document, so nothing else in the docshell changes.

Note that even without the reload() call we have a tab with the Google url and favicon.

Note further that even without all this stuff a page could window.open("about:blank", "_self") and then inject content into there, right?

We _could_ try to do something in nsJSProtocolHandler wherein if we don't plan to load any data and the current content viewer on the docshell is not showing and was created by CreateAboutBlankContentViewer we would just go ahead and Show() it.  In this testcase that means that an actual blank page would be showing in the browser window.

We could even put this Show() call on a runnable that docshell posts after doing the InternalLoad(), assuming that content viewer is still current at that point (which would imply that the script didn't return a string).  

That would be good for maintaining our internal invariants in some sane way, but wouldn't address the spoofing issue, imo.

Comment 7

[Abdulrahman Alqabandi]

> Note further that even without all this stuff a page could window.open("about:blank", "_self") and then inject content into there, right?
> 

The only difference between this bug and using window.open is that the placeholder 'Search or enter address' only occurs when using the bug described here, but if we use window.open() the address URL will be visibly  'about:blank'

Comment 8

[Boris Zbarsky [:bzbarsky]]

> the address URL will be visibly  'about:blank'

OK.  Gijs, why is the UI not managing that in this case?

Comment 9

[Abdulrahman Alqabandi]

Interestingly, testing this on FF nightly 45.0a1 the document completely freezes (with the 'red' clicked anchor tag frozen) right clicking does not work.

Comment 10

[Abdulrahman Alqabandi]

Attachment: A test case that works for Firefox nightly v45.0a1

With a few adjustments to the original testcase (that worked for FF v42.0) we can make it work on the latest nightly

Comment 11

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> > the address URL will be visibly  'about:blank'
> 
> OK.  Gijs, why is the UI not managing that in this case?

I don't know offhand, but I can look into it.

Comment 12

[:Gijs (he/him)]

(to be clear, I expected the placeholder would always be shown for about:blank, also for the window.open case, and the fact that it doesn't in that case is surprising to me. I don't know if it's deliberate, or just something that happens to 'work' right now.)

Comment 13

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> > the address URL will be visibly  'about:blank'
> 
> OK.  Gijs, why is the UI not managing that in this case?

https://dxr.mozilla.org/mozilla-central/rev/eec9a0bfd929a68237f0ef799a9d8f28c4749296/browser/base/content/browser.js#2363-2368

IOW, if there's a content opener, we will not replace about:blank, but we will in all other cases. I think this is because this used to be the new tab page, and so if you opened the new tab page you wouldn't want there to be anything in there. We kept this for about:blank because some people overrode the default new tab page with about:blank because they didn't like it and/or it was slower, so it stayed in the list.

It seems like rather than just for openers, we would also want to do this if a page changed its own window to be about:blank. I think that would require exposing that on docshell somehow, unless there's already some way to determine that that I'm not aware of... Perhaps the progress listeners could figure this out and set a flag?

Comment 14

[Abdulrahman Alqabandi]

Attachment: FF-URL-SPOOF.html

I have managed to come up with a test case that demonstrate full url spoofing. The only condition is that our spoofed url will end with an invalid url char. This bug also involves the user hitting the back button (which in this case will most likely occur naturally because of the error page)

PoC:
1. Open Poc page attached
2. Click anchor tag
3. After error is displayed, press back

We end up in the URL 'https://facebook.com;' (note the semicolon) 

Not sure if this needs a seperate report since its based off of the javascript URI scheme behavior discussed here.
Tested on Firefox v42

Comment 15

[Abdulrahman Alqabandi]

Attachment: Screenshot of the spoofed url, its stuck loading but we control document

Comment 16

[Abdulrahman Alqabandi]

Works on latest nightly v45.0a1

Comment 17

[Abdulrahman Alqabandi]

Attachment: A better PoC (fixed the indefinite 'connecting' and other things)

Tested on FFv42 and FF Nightly v45.0a1

Comment 18

[Abdulrahman Alqabandi]

We can achieve an actual full URL spoof (without the need to put invalid characters) by simply changing the latest testcase that uses 'https://facebook.com`' with 'http:https://facebook.com', this will result in the URL address displaying 'https://facebook.com' with its document controlled by us.

Comment 19

[Abdulrahman Alqabandi]

(In reply to Abdulrahman Alqabandi from comment #18)
> We can achieve an actual full URL spoof (without the need to put invalid
> characters) by simply changing the latest testcase that uses
> 'https://facebook.com`' with 'http:https://facebook.com', this will result
> in the URL address displaying 'https://facebook.com' with its document
> controlled by us.

Correction, the URL displayed in that case is 'https//facebook.com'

Comment 20

[Abdulrahman Alqabandi]

Would this be considered a high risk bug given the new information?

Comment 21

[Andrew McCreight [:mccr8]]

I'm not sure. Maybe Dan can answer that.

Comment 22

[Abdulrahman Alqabandi]

May I also add that its possible to spoof a website completely (char per char) if the targeted website triggers a "This Connection is Untrusted" error when visited through the HTTPS URI scheme. (AKA about:certerror?e=nssBadCert&...etc)

PoC:

Simply modify the test case in comment #17 where instead of navigating to 'https://www.facebook.com`' we have it go to 'https://qabandi.com/' (this site does not use trusted certs) which will give us the desired error page.

Follow the same steps and the URL bar will end up displaying 'https://qabandi.com/' with the document controlled by us.

Comment 23

[Abdulrahman Alqabandi]

Attachment: Screenshot of the full url spoof

Comment 24

[Abdulrahman Alqabandi]

I guess to sum it up, what we are able to do is spoof the location only if some sort of error page is shown when visiting the target url. (or IOW, when the browser navigates to some sort of 'about:{error-page}') This does not seem to work when we navigate to a website normally (due to existing security measures mentioned in Comment 6 I assume)

Comment 25

[Andrew McCreight [:mccr8]]

I'll clear the rating so it gets looked at again at our next security triage meeting.

Comment 26

[Abdulrahman Alqabandi]

Is it possible to get an update on this?

Comment 27

[Andrew McCreight [:mccr8]]

Sorry, with the end of the year we haven't had a chance to look at this in the triage meeting yet.

Comment 28

[Abdulrahman Alqabandi]

Here is a small bug that I believe is due to the faulty behavior described here.

PoC:

<a id='q' href='moz-action:switchtab,' target='_blank'>click</a>
<script>q.onclick=$=>{
	setTimeout($=>{alert('View the new tab and check the url\n\n\n\nby:@qab')},33);
}</script>

http://js.do/code/80600

For some reason the new tab contains the same url as the opener (once you switch tabs)

Comment 29

[Andrew McCreight [:mccr8]]

Gijs, can you look at this some more? This has been sitting around for a few months. Thanks!

Comment 30

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #29)
> Gijs, can you look at this some more? This has been sitting around for a few
> months. Thanks!

Yes, I assumed it was waiting for a new sec rating and verification of the new suggestion in comment #22. FWIW, the end-result and requirements (user presses back button) seem fairly similar to bug 1245264 (which might help in terms of the sec rating), but the cause is different. Ni for updated sec rating.

Anyway, in terms of looking at this, it seems comment #22 changes the mechanics on our side enough that I'm not entirely sure if the same issue I identified in comment #13 is still causing what's going on there, though it doesn't help that it's 10pm and Nightly keeps crashing when I try to use the browser debugger.

Really, Marco might be a better person to investigate this.

Marco might also know the answer to the question at the tail end of comment #13 about how we determine whether about:blank is a user load or not (even if that does turn out to not be relevant for the change in STR from comment #22). Yes, we could trivially remove about:blank from that set of pages, but it seems likely that new-tab-url-replacement add-ons will just add it back, and that wouldn't actually fix the security issue with that.

Comment 31

[Abdulrahman Alqabandi]

Would like to point out that according to ( https://wiki.mozilla.org/Security_Severity_Ratings .) 'sec-high' is the recommended rating for a "Spoofing of full URL bar or bypass of SSL integrity checks."

Comment 32

[:Gijs (he/him)]

I just made some more time to look at this. When I use the testcase and steps from comment #22, the <browser>'s currentURI is wrong. It should be pointing to about:blank, or, failing that, the URI that originally opened about:blank, and instead I get the URI from which we went back (so https://qabandi.com/). The browser's documentURI is correct (about:blank), as is the document element's nodePrincipal.

Boris, that seems like a docshell issue. Am I missing something?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Sounds like it, but it also sounds like something different from the original issue reported in this bug (the one I analyzed in comment 6).  Seems like we should get a new bug filed on the interaction involving session history, and probably ask Olli whether he can take a look.

Comment 34

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #33)
> Sounds like it, but it also sounds like something different from the
> original issue reported in this bug (the one I analyzed in comment 6). 
> Seems like we should get a new bug filed on the interaction involving
> session history, and probably ask Olli whether he can take a look.

Done, bug 1247968.

Updating the summary back to the original issue at hand and looking at that again. :-\

Comment 35

[:Gijs (he/him)]

Attachment: Patch v1

This forces about:blank to be shown in these cases, AFAICT. This depends on us actually using wyciwyg to identify cases where we construct these documents based on something other than just the user requesting that URI. I'd like to know if that is a sane assumption.

Comment 36

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #35)
> Created attachment 8718889 [details] [diff] [review]
> Patch v1
> 
> This forces about:blank to be shown in these cases

Err, specifically, I tested attachment 8695015 [details] which produces the empty URL bar on current nightly. I don't think this will help for the later testcases covered in bug 1247968

Comment 37

[Boris Zbarsky [:bzbarsky]]

I don't understand what that patch is trying to do and what problem it's actually solving and why...  I assume in this case aURI is falsy?  What is gBrowser.currentURI?  Is it wycywig:about:blank or some such?

Why can we not just set canOmitURI to false if uri.spec != originalURI.spec ?

All that said, how is this all different from clicking a link opening a popup that then navigates the _parent_ to about:blank?  In that situation, what will show up in the URL bar with the patch?  Without the patch?

Comment 38

[:Gijs (he/him)]

Comment on attachment 8718889 [details] [diff] [review]
Patch v1

(In reply to Boris Zbarsky [:bz] from comment #37)
> I don't understand what that patch is trying to do

Address comment #8 - force about:blank to show up instead of an empty address bar. This was the original problem reported in this bug.

> and what problem it's
> actually solving and why...  I assume in this case aURI is falsy?

IIRC yes, from: https://dxr.mozilla.org/mozilla-central/rev/d719ac4bcbec13e0ba13a41547788e3bf365c679/browser/base/content/tabbrowser.xml#667-668,678-680,687-688

but it doesn't matter much if it's that or gBrowser.currentURI that's producing the wyciwyg URL here.

>  What is
> gBrowser.currentURI?  Is it wycywig:about:blank or some such?

More or less, IIRC wyciwyg:/0/about:blank or whatever. But I can't seem to reproduce this again, though it could be because my build state is a bit messed up at the minute. I'm traveling this weekend and I don't think I'll be able to dive into detail on this before late on Sunday, or Monday.

> Why can we not just set canOmitURI to false if uri.spec != originalURI.spec ?

We could, I think. Are there other cases that that would catch?

> All that said, how is this all different from clicking a link opening a
> popup that then navigates the _parent_ to about:blank?  In that situation,
> what will show up in the URL bar with the patch?  Without the patch?

Well, it turns out that AFAICT I messed up the ternary in the patch and so it just does the wrong thing (pretty much always shows the URI, when it shouldn't do that for e.g. just opening a new tab). I'll clear review and keep needinfo to look at this again on Monday. Not sure how I missed that. :-\

Comment 39

[:Gijs (he/him)]

Attachment: Patch v2

Right, after having a few days to ponder this one, this seems like a more correct fix. It fixes both the original testcase and the case described by Boris in comment 37 (pre-patch: empty location bar; post-patch: 'about:blank').

I fixed a number of other cases where we check hasContentOpener. I don't know if hasContentOpener is really still useful, but we can figure that out in a followup; I'd rather not accidentally introduce a different security issue while we're fixing this one...

Boris/Marco, does this look safe/sensible to you?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8719441 [details] [diff] [review]
Patch v2

Ah, this is much clearer.  At least it directly compares the two things we care about: the URI we're going to show and the URI of the origin involved, if any.

r=me.

As for removing browser.hasContentOpener, I suspect the only case where it probably matters is if you open an about:blank tab (or other initial page URI) and then evaluate window.open(location.href) from a click handler (for the about:blank case this would be a handler attached via the console or something).  I think with this patch, and on tip, the new window shows "about:blank" in the url bar, but if we dropped the hasContentOpener check it might show "" (because the principal and URI would in fact match, afaict).  I'm not sure how much we care about this edge case...

Comment 41

[Andrew McCreight [:mccr8]]

I'm going to mark this sec-moderate. Showing a blank URL bar instead of about:blank is a little confusing, but isn't really claiming anything false. (The stronger spoof in comment 22 has been split into a separate bug.)

Comment 42

[Marco Bonardo [:mak]]

Comment on attachment 8719441 [details] [diff] [review]
Patch v2

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

How hard would be to write a test for this? Based on the change sounds like something that could be easily reverted inadvertently.
Does the test for bug 1247968 cover this one as well?

As a side note,

::: browser/base/content/browser.js
@@ +6387,5 @@
>  
>    return true;
>  }
>  
> +function isEmptyPageReallyEmpty(browser = gBrowser.selectedBrowser,

this needs a nice javadoc explaining why it exists and when it is supposed to be used. We should not really expect devs hitting it having to do code archeology and go through all of this bug.

I'd also suggest to rename it to something more meaningful, for example something like checkEmptyPageIdentity or such, otherwise the code where it appears becomes a little bit more obscure.

@@ +6389,5 @@
>  }
>  
> +function isEmptyPageReallyEmpty(browser = gBrowser.selectedBrowser,
> +                                uri = browser.currentURI) {
> +  if (browser.hasContentOpener) {

Please add a comment before each heuristic explaining why it's a good way to distinguish valid empty pages from invalid ones. Again, we don't want people to guess or go through the whole discussion.

@@ +6397,5 @@
> +  if (contentPrincipal.URI) {
> +    return contentPrincipal.URI.equals(uri);
> +  }
> +  let ssm = Services.scriptSecurityManager;
> +  return ssm.isSystemPrincipal(contentPrincipal);

Sorry, this last heuristic is a little bit unclear to me, which are the cases hitting it?

Comment 43

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #42)
> Comment on attachment 8719441 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8719441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How hard would be to write a test for this? Based on the change sounds like
> something that could be easily reverted inadvertently.

I'll have a look. I wouldn't expect it to be particularly difficult necessarily.

> Does the test for bug 1247968 cover this one as well?

There is no test there, but the cause of the issue there is different, AFAICT, so I'd expect the fix and test to be different, too.

> As a side note,
> 
> ::: browser/base/content/browser.js
> @@ +6387,5 @@
> >  
> >    return true;
> >  }
> >  
> > +function isEmptyPageReallyEmpty(browser = gBrowser.selectedBrowser,
> 
> this needs a nice javadoc explaining why it exists and when it is supposed
> to be used. We should not really expect devs hitting it having to do code
> archeology and go through all of this bug.
> 
> I'd also suggest to rename it to something more meaningful, for example
> something like checkEmptyPageIdentity or such, otherwise the code where it
> appears becomes a little bit more obscure.

We're not really checking 'identity', though, are we? We're essentially checking "does the web control the contents of this page", except that if people use an add-on to explicitly set the new tab page to a web URI, we're clearing the location bar for that, too. I think that's a bad idea, but it means that naming the method "isPageControlledByWeb" doesn't really work. Maybe "pageURIMatchesPrincipal".

> @@ +6397,5 @@
> > +  if (contentPrincipal.URI) {
> > +    return contentPrincipal.URI.equals(uri);
> > +  }
> > +  let ssm = Services.scriptSecurityManager;
> > +  return ssm.isSystemPrincipal(contentPrincipal);
> 
> Sorry, this last heuristic is a little bit unclear to me, which are the
> cases hitting it?

There are a number of different principal implementations. Some of them have URIs (normal web pages, and null principals have a moz-null-principal URI). However, not all of them do - and in particular, the system principal has no URI, and expanded principals do not have a single URI either (their .URI property is null). The code implies that we only think the page is empty / the URI matches the principal if the principal is the system principal. AIUI web code shouldn't normally be able to cause a page to be created that has an expanded principals. Add-ons might be able to; I'm not sure. Either way, restricting the case where we think a tab is empty/not-web-controllable, when it has no URI, to cases where it has the system principal seemed safest.

Comment 44

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #43)
> We're not really checking 'identity', though, are we?

I know identity is not a perfect word, but at least it clarifies the kind of check. We are checking if the empty page is really what it asserts to be, like an identity check.
Another name could be checkEmptyPageOrigin? (but again, origin has lots of meanings in our codebase)

> Maybe "pageURIMatchesPrincipal".

if not that we're also checking other stuff, like contentOpener, and system principal doesn't really match anything. I was thinking of something a little bit more generic.

> > @@ +6397,5 @@
>  Either way,
> restricting the case where we think a tab is empty/not-web-controllable,
> when it has no URI, to cases where it has the system principal seemed safest.

yep, I agree. Also putting a sum up of your comment above the heuristic would be nice :)

Comment 45

[:Gijs (he/him)]

Attachment: Patch v3

I noticed from running the test in e10s that in e10s, contentPrincipal is null if we switch selection to an unloaded tab that has about:blank in it but has never had any state changes to tell us what's loaded. So I had to make an exception for that. :-(

Comment 46

[:Gijs (he/him)]

Comment on attachment 8720530 [details] [diff] [review]
Patch v3

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

::: browser/base/content/test/urlbar/browser_urlbar_blanking.js
@@ +21,5 @@
> +    content.document.querySelector('a').click();
> +  });
> +  yield browserLoaded;
> +  ok(gURLBar.value.startsWith("javascript"), "The URL bar should have the JS URI");
> +  SimpleTest.expectUncaughtException(true);

In case it's not obvious: loading the 'broken' JS URI causes an exception here that the test framework complains about otherwise.

Comment 47

[Marco Bonardo [:mak]]

Comment on attachment 8720530 [details] [diff] [review]
Patch v3

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

::: browser/base/content/browser.js
@@ +6427,5 @@
> +  if (!contentPrincipal && uri.spec == "about:blank") {
> +    // Need to specialcase this because of how tabbrowser switches selected
> +    // tabs before it's heard anything from that tab, so there's no
> +    // contentPrincipal in the e10s case.
> +    return true;

So we can assume this is safe cause browser never got the contentPrincipal from the child, thus it's unlikely the child tried to forge it?

I'd rephrase to:
"Need to specialcase this because, in the e10s case, tabbrowser caches the contentPrincipal when it gets it from the child process LocationChange listener. If it never heard anything about this browser, it won't have a contentPrincipal for it yet. In such a case it's unlikely the child may have forged a fake empty location."

::: browser/base/content/test/urlbar/browser_urlbar_blanking.js
@@ +21,5 @@
> +    content.document.querySelector('a').click();
> +  });
> +  yield browserLoaded;
> +  ok(gURLBar.value.startsWith("javascript"), "The URL bar should have the JS URI");
> +  SimpleTest.expectUncaughtException(true);

yeah, please add a comment above the expectUncaughtException, it's clear only to whoever wrote the test. Moreover the "broken" js is not visible in the test file since it's in another file.

Comment 48

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #47)
> Comment on attachment 8720530 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8720530 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +6427,5 @@
> > +  if (!contentPrincipal && uri.spec == "about:blank") {
> > +    // Need to specialcase this because of how tabbrowser switches selected
> > +    // tabs before it's heard anything from that tab, so there's no
> > +    // contentPrincipal in the e10s case.
> > +    return true;
> 
> So we can assume this is safe cause browser never got the contentPrincipal
> from the child, thus it's unlikely the child tried to forge it?
> 
> I'd rephrase to:
> "Need to specialcase this because, in the e10s case, tabbrowser caches the
> contentPrincipal when it gets it from the child process LocationChange
> listener. If it never heard anything about this browser, it won't have a
> contentPrincipal for it yet. In such a case it's unlikely the child may have
> forged a fake empty location."

I was not happy with this, so I kept digging. Basically, it turns out this is bug 1249362. I'll make the check specific to e10s as well, reference that bug, and the e10s team can hopefully fix that separately. Many thanks to mconley for helping me sort this one out.

Comment 49

[Marco Bonardo [:mak]]

I actually thought it was on purpose exactly because the parent process was waiting for the principal from the content process... Good that we can fix it.

Comment 50

[:Gijs (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/30c194c260a7

Comment 51

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #50)
> https://hg.mozilla.org/integration/fx-team/rev/30c194c260a7

This tripped up some other tests that open about:blank in a new tab and expect that tab to be treated as 'empty'. So I pushed a followup:

https://hg.mozilla.org/integration/fx-team/rev/ae5d37aea5c8

--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -6438,20 +6438,23 @@ function checkEmptyPageOrigin(browser = 
       !contentPrincipal && uri.spec == "about:blank") {
     // Need to specialcase this because of how stopping an about:blank
     // load from chrome on e10s causes a permanently null contentPrincipal,
     // see bug 1249362.
     return true;
   }
   // Not all principals have URIs...
   if (contentPrincipal.URI) {
+    if (uri.spec == "about:blank" && contentPrincipal.isNullPrincipal) {
+      return true;
+    }
     return contentPrincipal.URI.equals(uri);
   }
   // ... so for those that don't have them, enforce that the page has the
-  // system principal (this matches e.g. on about:blank).
+  // system principal (this matches e.g. on about:home).
   let ssm = Services.scriptSecurityManager;
   return ssm.isSystemPrincipal(contentPrincipal);
 }

bz, I *think* this is accurate (if not, please let me know!), but is there an easier way to do this? (could be a followup) I'm assuming not because we essentially want to say something like "does this URI match this principal" which doesn't work so well for about:blank / the null principal, because anybody can open it...

Ugh, and that comment should quote about:newtab, not about:home. I can fix that in a DONTBUILD push, I guess, assuming that this fix is correct.

Comment 52

[Boris Zbarsky [:bzbarsky]]

That seems fine, yes, since a nullprincipal about:blank is clearly one that a web page did not open (and can't control).

Comment 53

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/30c194c260a7
https://hg.mozilla.org/mozilla-central/rev/ae5d37aea5c8

Comment 54

[Abdulrahman Alqabandi]

Does the fix work for the little bug mentioned in Comment 28 also?

Comment 55

[:Gijs (he/him)]

(In reply to Abdulrahman Alqabandi from comment #54)
> Does the fix work for the little bug mentioned in Comment 28 also?

No. That's a separate issue caused by the use of a moz-action URI. This is what happens when too many separate things get stuck into a single bugreport. :-\

I'll file a separate bug about this.

Comment 56

[Carsten Book [:Tomcat]]

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

Comment 57

[:Gijs (he/him)]

I'll try to put up a patch for uplift tomorrow.

Comment 58

[:Gijs (he/him)]

Attachment: Patch for beta

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: security issue with the location bar being emptied too eagerly, potentially leading to user confusion as to who controls the content area
[Describe test coverage new/current, TreeHerder]: comes with tests, has been on Nightly for a while, the url bar has decent test coverage already;
[Risks and why]: low-ish, as it is a fairly focused change, it has tests, and the URL bar has reasonable test coverage in general, but obviously it's starting to get late-ish in this cycle.
[String/UUID change made/needed]: no

Comment 59

[:Gijs (he/him)]

Attachment: Patch for aurora

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: security issue with the location bar being emptied too eagerly, potentially leading to user confusion as to who controls the content area
[Describe test coverage new/current, TreeHerder]: comes with tests, has been on Nightly for a while, the url bar has decent test coverage already;
[Risks and why]: low, as it is a fairly focused change, it has tests, and the URL bar has reasonable test coverage in general.
[String/UUID change made/needed]: no

Comment 60

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8723072 [details] [diff] [review]
Patch for beta

Taking it for the reasons that Gijs exposed + 45 is an esr
Should be in 45 beta 10

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/91ddff279311
https://hg.mozilla.org/releases/mozilla-beta/rev/80ce3f1ffe03

Comment 62

[Paul Silaghi, QA [:pauly]]

This is what I see in the latest nightly using the testcase in comment 10 - http://i.imgur.com/Ht87gOM.png
Thoughts?

Comment 63

[:Gijs (he/him)]

(In reply to Paul Silaghi, QA [:pauly] from comment #62)
> This is what I see in the latest nightly using the testcase in comment 10 -
> http://i.imgur.com/Ht87gOM.png
> Thoughts?

Looks good to me? The problem was that the URL bar would display nothing. Now it explicitly says about:blank. AIUI that's the desired behaviour here.

Comment 64

[Paul Silaghi, QA [:pauly]]

(In reply to :Gijs Kruitbosch from comment #63)
> Now it explicitly says about:blank. AIUI that's the desired behaviour here.
Ok, but the content is still loaded.
Moreover, the tab circle spins forever. And the Inspector is frozen.
Still expected?

Comment 65

[:Gijs (he/him)]

(In reply to Paul Silaghi, QA [:pauly] from comment #64)
> (In reply to :Gijs Kruitbosch from comment #63)
> > Now it explicitly says about:blank. AIUI that's the desired behaviour here.
> Ok, but the content is still loaded.

Yes. It's perfectly legitimate for a web site to do:

var w = window.open("about:blank", "_blank");

w.document.appendChild(w.document.createElement('p'));
w.querySelector('p').textContent = "Hello";

or whatever. Not much we can do about that without breaking the internet.

> Moreover, the tab circle spins forever.

It doesn't in your screenshot.

> And the Inspector is frozen.

Also no inspector in your screenshot.

> Still expected?

Both of these also happened before this fix, no, or are they regressions?

Both of these seem like issues that could be worth followups if you can distill down why/when they happen from the original testcase.

Comment 66

[Paul Silaghi, QA [:pauly]]

Thanks for clarifications.

(In reply to :Gijs Kruitbosch from comment #65)
> Not much we can do about that without breaking the internet.
FWIW, Google Chrome doesn't load the content.
 
> Both of these also happened before this fix, no, or are they regressions?
Yes, both happened before the fix too.

Based on comments 62-65, marking this as verified on FX 45, 46.0a2 (2016-03-02), 47.0a1 (2016-03-02) Win 7

Comment 67

[:Gijs (he/him)]

(In reply to Paul Silaghi, QA [:pauly] from comment #66)
> Thanks for clarifications.
> 
> (In reply to :Gijs Kruitbosch from comment #65)
> > Not much we can do about that without breaking the internet.
> FWIW, Google Chrome doesn't load the content.

I'm not entirely sure what would cause different behaviour in Chrome. It's possible they don't let you refresh javascript URIs. Probably worth a followup bug as well.

> > Both of these also happened before this fix, no, or are they regressions?
> Yes, both happened before the fix too.

OK, please CC me on any followup bugs you file.

Comment 68

[Abdulrahman Alqabandi]

The tab circling happens (for some reason) whenever we do a document.write() (possible bug?), I used window.stop() to fix this in the test case in Comment 17 (please test all test cases mentioned)

Also, in my opinion, 'about:blank' should always reflect a blank document, if it contains a document created by 'http://localhost' then the url should change to 'http://localhost'

Comment 69

[Paul Silaghi, QA [:pauly]]

(In reply to Abdulrahman Alqabandi from comment #68)
> test case in Comment 17
I believe that was filed as a separate issue and it's not fixed yet.

Comment 70

[:Gijs (he/him)]

(In reply to Abdulrahman Alqabandi from comment #68)
> Also, in my opinion, 'about:blank' should always reflect a blank document,
> if it contains a document created by 'http://localhost' then the url should
> change to 'http://localhost'

This is an interesting proposal, but not the reality in any other browser, either:

http://jsbin.com/rexaqatado/edit?html,js,output

being a trivial testcase (on both Chrome and IE on Windows, the location bar there shows about:blank, as it does for us).

I don't know if this stuff is specced at all, but it seems worth converging on with other browsers if a change does need to be made. Probably worth taking up with the relevant w3c/whatwg working groups.

Comment 71

[Boris Zbarsky [:bzbarsky]]

> I don't know if this stuff is specced at all

It is.  What we do now follows the current spec.

Comment 72

[Boris Zbarsky [:bzbarsky]]

Well, I mean the URL bar is not specced, so I guess we can do whatever we want there.  But observable things about the document and window like location.href and document.URL are.

Comment 73

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #72)
> Well, I mean the URL bar is not specced, so I guess we can do whatever we
> want there.  But observable things about the document and window like
> location.href and document.URL are.

Right, I assumed that we were specifically talking about what we show users. As an engineer, making the location bar value be different from location.href irks me, but it's pretty clear that if mozilla.org controls the content of about:blank, that it's more useful to display mozilla.org than to display about:blank. Yes, if you go to the URL bar and hit enter you don't get the same document, but that has always been true and is also true for about:blank...

As I noted earlier, it seems all the browsers behave the same right now, but I don't really see a good reason not to change that (beyond engineer-me-being-irked :-) ).