1334587 - Work container tab forgets GitHub login after relaunch

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Richard Newman [:rnewman]]

(This seems a bit like Bug 1280590.)

I am logged into the same GitHub account in both the default container and Work.

Each time I relaunch the browser, and visit GitHub in a Work tab, I'm not signed in. I continue to be signed in in the default container across relaunches.

This is super frustrating, because I click GitHub links from my Work Gmail tab, then have to go through the 2FA SMS dance for GitHub auth upwards of three times a week (Nightly).

ni :baku 'cos Gijs told me to :D

Comment 1

[Andrea Marchesini [:baku]]

> I am logged into the same GitHub account in both the default container and
> Work.

Do you use double factor authentication?
I ask because I cannot reproduce it.

Comment 2

[Richard Newman [:rnewman]]

Yes; see comment 0 ("2FA").

I'm also happily logged in to GH on my iPad and phone at the same time.

Comment 3

[Andrea Marchesini [:baku]]

Kamil, do you have time to test this a bit and tell me if there is a STR? I cannot reproduce it locally.

Comment 4

[Kamil Jozwiak [:kjozwiak]]

(In reply to Andrea Marchesini [:baku] from comment #3)
> Kamil, do you have time to test this a bit and tell me if there is a STR? I
> cannot reproduce it locally.

I've looked at this briefly and I couldn't reproduce the issue. However, I did have the default container log me out of my github session once. I'll spend some more time this week and see if I can reproduce and get some reliable STR.

Comment 5

[Kestrel]

STR (Firefox 52 and Nightly 55):
1. Create a container tab, go to http://winware.org/en/cookie-permanent.php and set a permanent cookie with a name.
2. Create a different container tab and do the same.
3. Shutdown and restart Firefox.
4. Go back to the site in each container, note that both cookies still exist (if you restored the session, don't forget to refresh the cache).
5. Shutdown and restart Firefox a second time.
6. Go back to the site in each container and note that now only one of the container tabs has remembered its permanent cookie.

Bug 1283709 fixed a similar issue that affected session cookies.

Comment 6

[Kestrel]

When the site is revisited after the first restart (step 4), the permanent cookie for each container is deleted from cookies.sqlite and a new cookie created for the current container. Only cookies.sqlite is affected so it does not become apparent until after the second restart.

::: netwerk/cookie/nsCookieService.cpp#L1514
>  rv = mDefaultDBState->dbConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
>    "DELETE FROM moz_cookies "
>    "WHERE name = :name AND host = :host AND path = :path"),
>    getter_AddRefs(mDefaultDBState->stmtDelete));
>  NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
>  
>  rv = mDefaultDBState->dbConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
>    "UPDATE moz_cookies SET lastAccessed = :lastAccessed "
>    "WHERE name = :name AND host = :host AND path = :path"),
>    getter_AddRefs(mDefaultDBState->stmtUpdate));
>  NS_ENSURE_SUCCESS(rv, RESULT_RETRY);

Notice anything missing? There's no originAttributes in these conditions, so it is deleting/updating cookies as if they belong to the same container.

Comment 7

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f1ded34c386

Comment 8

[Jonathan Kingston [:jkt] he/him]

:kjozwiak are you able to replicate with the STR Kestral provided? I was able to on Ubuntu and the code highlighted was the issue, I was able to break the STR with the try patch that I have just submitted. Making a STR into a testcase might be a pain though.

I was able to reproduce this on latest central so 55 is affected.

I couldn't find any other cases where the database wasn't considering OAs.

Marking this as sec-audit as this might impact the security of cookies, I'm not sure on the code branches that update method could be called however that would be bad news if a normal cookie can update a private mode / container cookie.

Regression range still needed.

Comment 9

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1334587.patch

Comment 10

[Jonathan Kingston [:jkt] he/him]

Hey Baku,

Would you be able to checkout the patch that I made, if you can think of a suitable test too that would be great.

Thanks
Jonathan

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8858659 [details] [diff] [review]
bug-1334587.patch

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

Can you send it with these comments applied?
About a test: I don't know if this is covered, but maybe you can do:

1. a browser mochitest with 2 tabs: 1 container 1 normal, same URL.
2. the URL sets 1 cookie (so we will have 2 cookies with different OA)
3. using nsICookieManager to remove the normal cookie (no container) using nsICookieManager.remove()
4. check if the other cookie exists.

::: netwerk/cookie/nsCookieService.cpp
@@ +5007,5 @@
>      rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("path"),
>                                        aIter.Cookie()->Path());
>      NS_ASSERT_SUCCESS(rv);
>  
> +    nsCookieKey key = nsCookieKey(aIter.entry);

Instead doing this, expose OriginAttributes from nsCookie adding:

inline const mozilla::OriginAttributes& OriginAttributes() const { return mOriginAttributes; }

@@ +5012,5 @@
> +    nsAutoCString suffix;
> +    key.mOriginAttributes.CreateSuffix(suffix);
> +    rv = params->BindUTF8StringByName(
> +      NS_LITERAL_CSTRING("originAttributes"), suffix);
> +    NS_ASSERT_SUCCESS(rv);

This will simply be:

nsAutoCString suffix;
aIter.Cookie()->OriginAttributes().CreateSuffix(suffix);
...

@@ +5163,5 @@
>  void
>  nsCookieService::UpdateCookieInList(nsCookie                      *aCookie,
>                                      int64_t                        aLastAccessed,
> +                                    mozIStorageBindingParamsArray *aParamsArray,
> +                                    nsCookieEntry* aEntry)

No needs for this.

@@ +5194,5 @@
>      rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("path"),
>                                        aCookie->Path());
>      NS_ASSERT_SUCCESS(rv);
>  
> +    nsCookieKey key = nsCookieKey(aEntry);

Use the same approach as described before.

::: netwerk/cookie/nsCookieService.h
@@ +300,5 @@
>      bool                          SetCookieInternal(nsIURI *aHostURI, const nsCookieKey& aKey, bool aRequireHostMatch, CookieStatus aStatus, nsDependentCString &aCookieHeader, int64_t aServerTime, bool aFromHttp, nsIChannel* aChannel);
>      void                          AddInternal(const nsCookieKey& aKey, nsCookie *aCookie, int64_t aCurrentTimeInUsec, nsIURI *aHostURI, const char *aCookieHeader, bool aFromHttp);
>      void                          RemoveCookieFromList(const nsListIter &aIter, mozIStorageBindingParamsArray *aParamsArray = nullptr);
>      void                          AddCookieToList(const nsCookieKey& aKey, nsCookie *aCookie, DBState *aDBState, mozIStorageBindingParamsArray *aParamsArray, bool aWriteToDB = true);
> +    void                          UpdateCookieInList(nsCookie *aCookie, int64_t aLastAccessed, mozIStorageBindingParamsArray *aParamsArray, nsCookieEntry *aEntry);

no needs for this.

Comment 12

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1334587-2.patch

The suggested test technique actually was very similar to an existing test which doesn't catch the issue. In the test I made it doesn't catch the issue either, changing file_reflect_cookie_into_title.html to have an expires doesn't help even thought the code path with this issue also seems to only be dependent on the cookie not being a session cookie. I can't see any tests that check for session restore, is that the case?

OriginAttributesRef was also existing on cookie already so I used that.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e8d97df010b40a923cc01b28595f13079b85df5

Are we going to want this on Beta too? If so, please nominate :)

Comment 14

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8864063 [details] [diff] [review]
bug-1334587-2.patch

Approval Request Comment
[Feature/Bug causing the regression]: Containers
[User impact if declined]: Users get cookies deleted cookies on session restore
[Is this code covered by automated tests?]: partially, couldn't replicate the issue with the automated test itself due to the nature of restore. If anyone has ideas here I'm happy to do follow ups in case it happens in future.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, STR in Comment 5
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not much
[Why is the change risky/not risky?]: We are modifying how cookies are cleared on session restore. It has the ability to make that worse.
[String changes made/needed]: None

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/1e8d97df010b

Comment 16

[Gerry Chang [:gchang]]

Hi :jkt,
AFAIK, container feature is enabled only in Nightly and this patch modifies originAttributes a bit. Not sure if this will cause other regression. Can we let this ride the train?

Comment 17

[Kestrel]

Verified as fixed on 55.0a1 (20170510030301).

Comment 18

[Jonathan Kingston [:jkt] he/him]

Hey :gchang,

This also impacts all of the ~12k Containers Test Pilot users too.

Technically there may be security implications here as I modified the stmtUpdate for cookies. To my knowledge this is only called on session restore so a PM mode cookie will already be gone and a containers one might leak across a container.

I am however not really sure when things should ride the trains vs uplift so willing to hear others advice.

Thanks

Comment 19

[Gerry Chang [:gchang]]

Hi Daniel,
Any thoughts in terms of security concerns? 
My thought is that this might impact test pilot users but I'm more concerned about changing the cookies part. This issue has been there for a while and Containers is disabled by default, I would like to let this change ride the train and see if any side effect.

Comment 20

[Daniel Veditz [:dveditz]]

This will indeed impact test pilot users: it will fix an annoyance in a feature we are testing. The security risks of this are not big since originAttributes will be empty for people not using the Containers feature. In fact, since Jonathan did the sec-audit and checked for places that might be missing originAttributes it doesn't look like this is a security bug after all. My fear was that if we were deleting cookies in the wrong container we were maybe also leaking cookies from one container to another, but that turned out not to be the case.

So this should be evaluated strictly on whether it's worth fixing the bug for the Containers test pilot cohort. It's a small group, but we want to get good/valid feedback on their use of the feature.

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8864063 [details] [diff] [review]
bug-1334587-2.patch

Thanks :jkt & Daniel for the information. 
From my point of view, Containers is not enabled by default and it's a small group. I want to be more cautious about the change. Let's let it ride the train. Beta54-.

Comment 22

[Daniel Veditz [:dveditz]]

FWIW that means people using the Containers Test Pilot (or either of a couple of alternate add-ons built on the feature) won't get a working experience until August 2017. If you look at the patch it will not affect anyone who is _not_ using the feature.

Comment 23

[Nathan Froyd [:froydnj]]

Yes, this doesn't seem like the correct decision.  ni? to Gerry for comment 22.

Comment 24

[Gerry Chang [:gchang]]

Comment on attachment 8864063 [details] [diff] [review]
bug-1334587-2.patch

That's a fair point. Sincde the patch will not affect users who are _not_ using the feature, it should be low risk. Let's take it in 54 Beta. Should be in 54 beta 10.

Comment 25

[Nathan Froyd [:froydnj]]

(In reply to Gerry Chang [:gchang] from comment #24)
> That's a fair point. Sincde the patch will not affect users who are _not_
> using the feature, it should be low risk. Let's take it in 54 Beta. Should
> be in 54 beta 10.

Thank you!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e609219b9f58

Comment 27

[Jonathan Kingston [:jkt] he/him]

Just a quick note :RyanVM added a quick patch to add OriginAttributesRef as it was missing from nsCookie.h due to that not riding the trains yet.

It appears this was added in: https://bugzilla.mozilla.org/show_bug.cgi?id=1356165