Closed Bug 1301274 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: bugzilla.mozilla.org, Assigned: allstars.chh)

References

()

Details

(Whiteboard: [tor][domsecurity-active])

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1260931
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [tor][domsecurity-backlog1]
I guess when we match the whitelist, there's mismatch in origin attributes.  I'll look into this.
Assignee: nobody → jhao
Cookies themselves seem to work to some extent. I can login on the site in both cases.
Just discussed with Jonathan I already have a fix for this
Assignee: jhao → allstars.chh
Attached patch Patch. (obsolete) — Splinter Review
This is like Bug 1280497 to use default value for firstPartyDomain like what userContextId did.
So I ask baku for review.
Attachment #8789336 - Flags: review?(amarchesini)
Whiteboard: [tor][domsecurity-backlog1] → [tor][domsecurity-active]
Status: NEW → ASSIGNED
(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.
Priority: -- → P2
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.
Attachment #8789336 - Flags: review?(amarchesini)
Attachment #8789336 - Flags: review+
Hi Dave
I've heard from Tanvi we don't want permission to be isolated on 1st party domain,

Can you confirm?

Thanks
Flags: needinfo?(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.
Flags: needinfo?(huseby)
(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.
Attached patch Patch. v2Splinter Review
addressed baku's comment
Attachment #8789336 - Attachment is obsolete: true
Attachment #8792430 - Flags: review+
Pushed by yhuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34f11f589c4c
use default firstPartyDomain in PermissionManager. r=baku
https://hg.mozilla.org/mozilla-central/rev/34f11f589c4c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.