1245124 - logged into bugzilla on default container after opening link via gmail in another container

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Kamil Jozwiak [:kjozwiak]]

Opening a bugzilla link through gmail will open the link in the default container and you'll be logged into bugzilla even though you've never logged into bugzilla within the default container. I checked the cookie manager and didn't see any "none" container entries in the bugzilla category, so it must have been registered as a "Work" container.

This is a bit misleading, if we're still in the "Work" container, we should add the correct UI to indicate that. However, if we're actually in the default container, we need to figure out why the user is still logged in even though he's never logged into bugzilla via the default container. This most likely affects other websites as well.

I've attached a video of the issue (might be easier to understanding seeing it happen)
* https://youtu.be/12U9ldMw7ic

STR:

* log into gmail via the Work container (id=2), I used my Mozilla account
* log into bugzilla via the Work container (id=2) (make sure you've never been logged into bugzilla via the default container)
* Close the bugzilla tab that's opened in the Work container (id=2)
* Find an email within gmail that includes a bugzilla link (this tab should be opened in the work container)
* click on the bugzilla link within gmail, this will open another tab with you being logged into bugzilla but in the "default" container

Comment 1

[Andrea Marchesini [:baku]]

I'm not able to reproduce this issue using the latest m-c. Can you confirm that you can? Thanks.

Comment 2

[Tanvi Vyas[:tanvi]]

Kamil can you redo this test with a latest nightly?

Comment 3

[Kamil Jozwiak [:kjozwiak]]

Yup, I can still reproduce this on both fx47 and fx46, I used the following builds:

* https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-09-03-03-47-mozilla-central/
* https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-10-00-40-06-mozilla-aurora/

I've attached a video [1] that illustrates the issue. Notice how I'm being logged into my bugzilla account via the default container when opening bugzilla links via gmail even though I've never logged into bugzilla using the default container. When I visit bugzilla using the default container, you'll notice that I'm not logged in.

- install either fx46 or fx47
- once installed, change privacy.userContext.enabled;true under about:config
- open a personal container and login into gmail
- open a personal container and login into bugzilla
- close the personal container that has bugzilla opened
- click on a bugzilla link inside gmail, you'll be logged into bugzilla in the new tab even though it's using the default container

[1] https://youtu.be/jnMWIwk6uHs

Comment 4

[Andrea Marchesini [:baku]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #3)
> Yup, I can still reproduce this on both fx47 and fx46, I used the following
> builds:
> 
> *
> https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-09-03-03-47-
> mozilla-central/
> *
> https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-10-00-40-06-
> mozilla-aurora/
> 
> I've attached a video [1] that illustrates the issue. Notice how I'm being
> logged into my bugzilla account via the default container when opening
> bugzilla links via gmail even though I've never logged into bugzilla using
> the default container. When I visit bugzilla using the default container,
> you'll notice that I'm not logged in.
> 
> - install either fx46 or fx47
> - once installed, change privacy.userContext.enabled;true under about:config
> - open a personal container and login into gmail
> - open a personal container and login into bugzilla
> - close the personal container that has bugzilla opened
> - click on a bugzilla link inside gmail, you'll be logged into bugzilla in
> the new tab even though it's using the default container


Ok I can reproduce it! Thanks. The reason why I could not was that I was doing right-click->open in a new tab.

So, the issue is that google is using window.open(). We use the correct nsIPrincipal (userContextId=personal), a new tab is open with the correct userContextId=principal, but the UI doesn't show it (no decorations are shown, blue bar, "personal" in the URL bar, etc...).

Just to confirm: once you reproduce the issue, can you open a new tab (default container) and open bugzilla. You will not be logged in, there. And, another confirmation: in the bugzilla-logged-in-tab-without-decoration, if you write: gmail.com, you will find yourself into your gmail account.

I'll work on this tomorrow.

Comment 5

[Kamil Jozwiak [:kjozwiak]]

> Just to confirm: once you reproduce the issue, can you open a new tab
> (default container) and open bugzilla. You will not be logged in, there.
> And, another confirmation: in the bugzilla-logged-in-tab-without-decoration,
> if you write: gmail.com, you will find yourself into your gmail account.

Yup, that's what I'm seeing as well. I wasn't sure if the new tab was the same container with missing UI or the default container. Is there a way of checking which container a tab is using via the browser console?

Comment 6

[Andrea Marchesini [:baku]]

Attachment: bugzilla.patch

Smaug, I don't know if you have time for this and, in case, just move this review to somebody else.

This patch does 2 things:

1. for e10s, it propagates the OriginAttributes of the caller from the child to the parent process. This is used to set the userContextId value correctly in the tab.

2. for non-e10s, it does something similar, taking that originAttributes from the opener document.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: bugzilla.patch

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8719297 [details] [diff] [review]
bugzilla.patch


>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
A browser peer needs to review these changes.

>       case Ci.nsIBrowserDOMWindow.OPEN_NEWTAB :
>         // If we have an opener, that means that the caller is expecting access
>         // to the nsIDOMWindow of the opened tab right away. For e10s windows,
>         // this means forcing the newly opened browser to be non-remote so that
>         // we can hand back the nsIDOMWindow. The XULBrowserWindow.shouldLoadURI
>         // will do the job of shuttling off the newly opened browser to run in
>         // the right process once it starts loading a URI.
>         let forceNotRemote = !!aOpener;
>+        let userContextId = aOpener && aOpener.document
>+                              ? aOpener.document.nodePrincipal.originAttributes.userContextId
>+                              : Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID;
>         let browser = this._openURIInNewTab(aURI, referrer, referrerPolicy,
>-                                            isPrivate, isExternal,
>-                                            forceNotRemote);
>+                                            isPrivate, userContextId,
>+                                            isExternal, forceNotRemote);
So this handles only OPEN_NEWTAB case. Does OPEN_NEWWINDOW work without any changes?
I don't see how.

>+++ b/browser/base/content/tabbrowser.xml
>@@ -1830,18 +1830,20 @@
> 
>             if (!aURI || isBlankPageURL(aURI)) {
>               t.setAttribute("label", this.mStringBundle.getString("tabs.emptyTabTitle"));
>             } else if (aURI.toLowerCase().startsWith("javascript:")) {
>               // This can go away when bug 672618 or bug 55696 are fixed.
>               t.setAttribute("label", aURI);
>             }
> 
>-            if (aUserContextId)
>+            if (aUserContextId) {
>               t.setAttribute("usercontextid", aUserContextId);
>+            }
>+
Looks like unrelated change.
(and I'm not even sure this is the coding style we use in browser chrome)


>+++ b/docshell/base/nsDocShell.cpp
>@@ -7902,16 +7902,26 @@ nsDocShell::CreateAboutBlankContentViewe
>     //
>     // It is important to fire the unload() notification *before* any state
>     // is changed within the DocShell - otherwise, javascript will get the
>     // wrong information :-(
>     //
>     (void)FirePageHideNotification(!mSavingOldViewer);
>   }
> 
>+  if (aPrincipal) {
>+    uint32_t userContextId;
>+    rv = aPrincipal->GetUserContextId(&userContextId);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return rv;
>+    }
>+
>+    SetUserContextId(userContextId);
>+  }
This looks wrong. We should have set the right UCI when we created the docshell, right?
Why do we need this here?


>                       PBrowser aNewTab,
>                       uint32_t aChromeFlags,
>                       bool aCalledFromJS,
>                       bool aPositionSpecified,
>                       bool aSizeSpecified,
>                       nsCString aURI,
>                       nsString aName,
>                       nsCString aFeatures,
>-                      nsCString aBaseURI)
>+                      nsCString aBaseURI,
>+                      DocShellOriginAttributes aOperOriginAttributes)
aOpener...

Comment 9

[Andrea Marchesini [:baku]]

Attachment: bugzilla.patch

I would like to do the opening of a new window in a separate patch or a follow up because the logic will be quite different. Are you ok with this?

Comment 10

[:Gijs (he/him)]

Comment on attachment 8720256 [details] [diff] [review]
bugzilla.patch

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

::: browser/base/content/browser.js
@@ +4712,5 @@
>  nsBrowserAccess.prototype = {
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIBrowserDOMWindow, Ci.nsISupports]),
>  
>    _openURIInNewTab: function(aURI, aReferrer, aReferrerPolicy, aIsPrivate,
> +                             aUserContextId, aIsExternal, aForceNotRemote=false) {

Don't change the ordinals (index) of existing params; this will break add-ons. r- for this.

Add new params last, and give them a sensible default value (probably 0 in this case?).

On a larger scale, what's the plan for add-ons? They will likely need substantial changes to do the right thing with containers...

@@ +4849,5 @@
>      var isExternal = (aContext == Ci.nsIBrowserDOMWindow.OPEN_EXTERNAL);
>      let browser = this._openURIInNewTab(aURI, aParams.referrer,
>                                          aParams.referrerPolicy,
> +                                        aParams.isPrivate,
> +                                        aParams.openerOriginAttributes.userContextId,

openURIInFrame is not just implemented here. The other implementation(s) is/are probably less important but you should file a followup to make sure they do the right thing (whatever that is).

Is openerOriginAttributes (and its properties) here guaranteed to be useful (and not null or otherwise weird)? I don't claim to understand the implicit_jscontext idl stuff, so I have no idea. :-\

Comment 11

[Andrea Marchesini [:baku]]

> On a larger scale, what's the plan for add-ons? They will likely need
> substantial changes to do the right thing with containers...

We don't have any plan for now. But addons should not change anything in order to work with containers.
Usually the want to open the "default" container, I would say.

> openURIInFrame is not just implemented here. The other implementation(s)
> is/are probably less important but you should file a followup to make sure
> they do the right thing (whatever that is).

I don't know what you mean here. Tell me more.

> 
> Is openerOriginAttributes (and its properties) here guaranteed to be useful
> (and not null or otherwise weird)? I don't claim to understand the
> implicit_jscontext idl stuff, so I have no idea. :-\

OpenerOriginAttributes should be always set. In case it's not, the default value is userContextId = 0, and we are going to open a standard new tab in the default container.

Comment 12

[:Gijs (he/him)]

(In reply to Andrea Marchesini (:baku) from comment #11)
> > On a larger scale, what's the plan for add-ons? They will likely need
> > substantial changes to do the right thing with containers...
> 
> We don't have any plan for now. But addons should not change anything in
> order to work with containers.

I'm sorry, but this is naive and just not likely to be the case with the idea behind containers as I understand it.

For instance, bug 1245262 implies new tabs should open in the same container. For add-ons to comply with that logic (and similarly for opening links in new tabs, or working 'correctly' from other added (context) menu items, they would need to obtain and then pass the correct userContextId.

That's not even speaking about the internal changes to cookies and all the other container-ized data that will lead to API changes that add-ons using those APIs will need to know/think about.

> > openURIInFrame is not just implemented here. The other implementation(s)
> > is/are probably less important but you should file a followup to make sure
> > they do the right thing (whatever that is).
> 
> I don't know what you mean here. Tell me more.

I mean that as far as I can tell that hunk is inside:

https://dxr.mozilla.org/mozilla-central/rev/6ea654cad929c9bedd8a4161a182b6189fbeae6a/browser/base/content/browser.js#4844-4853

That is implementing the openURIInFrame method of nsIBrowserDOMWindow. It is not the only implementation of that interface, cf. https://dxr.mozilla.org/mozilla-central/rev/6ea654cad929c9bedd8a4161a182b6189fbeae6a/browser/base/content/chatWindow.xul#125-128 .

> > Is openerOriginAttributes (and its properties) here guaranteed to be useful
> > (and not null or otherwise weird)? I don't claim to understand the
> > implicit_jscontext idl stuff, so I have no idea. :-\
> 
> OpenerOriginAttributes should be always set. In case it's not, the default
> value is userContextId = 0

if openerOriginAttributes is null, the code I was commenting on will throw...

Comment 13

[:Gijs (he/him)]

As a trivial example from:

https://mxr.mozilla.org/addons/search?string=openURIInFrame&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons

it seems that this add-on by :markh:

https://addons.mozilla.org/en-US/firefox/addon/pinned-window-extension-for/

(as well as tree style tabs and potentially others)

messes with the implementation of the method you're modifying here, which means that depending on how that's implemented (I didn't check exactly), either the add-on breaks or this bug's patch won't work correctly, unless the author modifies their add-on.

Comment 14

[Andrea Marchesini [:baku]]

> For instance, bug 1245262 implies new tabs should open in the same
> container. For add-ons to comply with that logic (and similarly for opening
> links in new tabs, or working 'correctly' from other added (context) menu
> items, they would need to obtain and then pass the correct userContextId.

Correct. We already broke the nsICookieManager in order to support userContextId.
I would like to reduce the impact for addons as much as we can but yes, it's better to have userContextId everywhere or use a default value if we can.

> I mean that as far as I can tell that hunk is inside:

I see. but my changes should be compatible with all of this. In particular here:

> https://dxr.mozilla.org/mozilla-central/rev/
> 6ea654cad929c9bedd8a4161a182b6189fbeae6a/browser/base/content/chatWindow.
> xul#125-128 .

we already ignore all the values in aParams.

> if openerOriginAttributes is null, the code I was commenting on will throw...

openerOriginAttributes is stored as a reference. It cannot be null.

Comment 15

[Andrea Marchesini [:baku]]

> https://mxr.mozilla.org/addons/
> search?string=openURIInFrame&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=ad
> dons

Ah ok! Now I see what you mean. We must definitely check the nature of aParam before using it.
I'll submit a new patch.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: bugzilla.patch

This patch should be compatible with existing addons.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8720306 [details] [diff] [review]
bugzilla.patch

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

r=me with the QI stuff removed.

::: browser/base/content/browser.js
@@ +4852,5 @@
>        return null;
>      }
>  
>      var isExternal = (aContext == Ci.nsIBrowserDOMWindow.OPEN_EXTERNAL);
> +    var params = aParams.QueryInterface(Ci.nsIOpenURIInFrameParams);

This isn't how this works in JS. aParams could be anything, and it might not even have a QI method. If it does have one, and doesn't implement this interface, it will throw.

I also don't see how this follows from any of the add-on callers. AFAICT all would pass something that they were passed as aParams, which should be the right type of object?

Just omit this check. We already assume that aParams is an object, so that seems fine,. We might want to nullcheck openerOriginAttributes and potentially check the userContextId thing exists (and pass 0 instead of undefined if not), but even there I don't know if we really need to bother doing that.

So in summary, I think you can omit all these checks. We just need to make sure that userContextId is the last parameter we pass to _openURIInNewTab.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6d2df3ca23

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e6d2df3ca23

Comment 20

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build:
* https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-23-03-03-04-mozilla-central/

I went through the following use cases when going through verification:

* logged into gmail via the "personal" container
* logged into bugzilla via the "personal" container
* logged into facebook via the "shopping" container
* logged into twitter via the "banking" container

Used the following test cases against the use cases mentioned above:

* ensured that clicking on a link opened a new tab in the correct tab with the correct UI decorations
* ensured "Open Link in New Tab" opened a new tab using the default container
* ensured "Open Link in New Window" opened a new window using the default container
* ensured "Open Link in New Private Window" opened a new window using PB mode
* ensured that opening containers via "Open Link in New Container Tab" opens the correct container with the correct UI decoration

* ensured that I wasn't logged into gmail in the other containers (default, shopping, banking, work) when using the above cases
** ensured that I was only logged into gmail when opening personal containers
* ensured that I wasn't logged into bugzilla in the other containers (default, shopping, banking, work) when using the above cases
** ensured that I was only logged into bugzilla when opening personal containers
* ensured that I wasn't logged into facebook in the other containers (default, personal, banking, work) when using the above cases
** ensured that I was only logged into facebook when opening shopping containers
* ensured that I wasn't logged into twitter in the other containers (default, personal, shopping, work) when using the above cases
** ensured that I was only logged into twitter when opening banking containers