1301274 - 1st party isolation + cookie whitelist causes security error on stackoverflow.com

Status: RESOLVED FIXED

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

Description

[The 8472]

Nightly	20160907030427

1. options -> privacy -> disable "accept cookies from sites" 
2. add https://stackoverflow.com to the exceptions
3. go to https://stackoverflow.com/questions/39337639
4. note the presence of the buttons in the rich text editor
5. set privacy.firstparty.isolate = true
6. reload the page, note the security error on the console and the absence of the buttons

This only seems to happen when a whitelist based cookie policy is used. But I expect that to be the common case for people who would also use 1st party isolation.

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Thanks for reporting this bug.

To clarify the STR, one has to change "Remember history" to "Use custom setting for history" before "accept cookies from sites" appears.

Comment 2

[Jonathan Hao (inactive) [:jhao]]

I guess when we match the whitelist, there's mismatch in origin attributes.  I'll look into this.

Comment 3

[The 8472]

Cookies themselves seem to work to some extent. I can login on the site in both cases.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Just discussed with Jonathan I already have a fix for this

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

This is like Bug 1280497 to use default value for firstPartyDomain like what userContextId did.
So I ask baku for review.

Comment 6

[Jonathan Hao (inactive) [:jhao]]

I wonder if we also need to modify nsIPermission::Matches?

http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/extensions/cookie/nsPermission.cpp#67

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jonathan Hao [:jhao] from comment #6)
> I wonder if we also need to modify nsIPermission::Matches?
> 
> http://searchfox.org/mozilla-central/rev/
> 950e13cca1fda6507dc53c243295044e8eda4493/extensions/cookie/nsPermission.
> cpp#67

The mPrincipal in nsPermisison is created by the GetPrincipalFromOrigin in nsPermissionManager, which uses default userContextId and default firstPartyDomain as this bug is trying to fix, so that looks okay to me.

"perm-changed" seems only used in Mozapps, so that should also be fine for userContextId and firstPartyDomain.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8789336 [details] [diff] [review]
Patch.

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +125,5 @@
>    // set to default to disable user context isolation for permissions
>    attrs.mUserContextId = nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID;
>  
> +  // set to default to disable firstParty isolation for permissions.
> +  attrs.mFirstPartyDomain = EmptyString();

attrs.mFirstPartyDomain.Truncate()

@@ +145,5 @@
>    // set to default to disable user context isolation for permissions
>    attrs.mUserContextId = nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID;
>  
> +  // set to default to disable firstParty isolation for permissions.
> +  attrs.mFirstPartyDomain = EmptyString();

same here.

@@ +2207,5 @@
>      // ensure that the user context isolation is disabled
>      attrs.mUserContextId = nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID;
>  
> +    // ensure firstPartyIsolation is disabled.
> +    attrs.mFirstPartyDomain = EmptyString();

and here.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Dave
I've heard from Tanvi we don't want permission to be isolated on 1st party domain,

Can you confirm?

Thanks

Comment 10

[Dave Huseby [:huseby]]

That is true.  Back in Bug 1233885 I modified the permissions manager so that it would slam the userContextId to the default value so that permissions would not be isolated.  This was mostly because of a lack of UI and some potentially bad user experiences.

This patch appears to doing the right thing.  Although I'm thinking we may want to modify the PrincipalOriginAttributes to have a method for setting the default value of all of the origin attributes instead of manually setting each one wherever we need to do this.  It was fine when it was just one origin attribute.  Now we should refactor the class so we stay D.R.Y.

I currently have Bug 1284986 which appears to be a problem with the changes that I made to the permission manager.  It may or may not be relevant.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Dave Huseby [:huseby] from comment #10)
> This patch appears to doing the right thing.  Although I'm thinking we may
> want to modify the PrincipalOriginAttributes to have a method for setting
> the default value of all of the origin attributes instead of manually
> setting each one wherever we need to do this.  It was fine when it was just
> one origin attribute.  Now we should refactor the class so we stay D.R.Y.
> 
jhao is doing this at bug 1302047, so I'll let him doing this.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch. v2

addressed baku's comment

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34f11f589c4c
use default firstPartyDomain in PermissionManager. r=baku

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/34f11f589c4c