1302047 - Ignore userContextId and firstPartyDomain when matching permissions.

Status: RESOLVED FIXED

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

Description

[Jonathan Hao (inactive) [:jhao]]

In bug 1280497, bug 1233885 and bug 1301274, we set default values to userContextIds and firstPartyDomains of permissions to default values so that they won't be isolated.

I think we should also fix nsIPermission::Matches to match permissions with different userContextIds or firstPartyDomains.

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

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Aren't permissions one of the things we want isolated?  For example, if I have a "video chat" container that I grant camera access to, I don't want any of my other containers to have that camera access.

Comment 2

[Jonathan Hao (inactive) [:jhao]]

Attachment: Ignore userContextId and firstPartyDomain when matching permissions.

Hi baku, please review these patches.  They're just like bug 1301274 and bug 1301617.

Comment 3

[Jonathan Hao (inactive) [:jhao]]

Attachment: Test if permission matches for different userContextId or firstPartyDomain.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86567996f94c

Comment 4

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8790632 [details] [diff] [review]
Test if permission matches for different userContextId or firstPartyDomain.

(In reply to Ben Kelly [:bkelly] from comment #1)
> Aren't permissions one of the things we want isolated?  For example, if I
> have a "video chat" container that I grant camera access to, I don't want
> any of my other containers to have that camera access.

Hi Ben,

We still haven't decided whether permissions should be isolated by first party domain.  Tor browser people think so, but haven't done that yet.  That's why I'm canceling the review request for now.

As for containers, the previous decision was not to isolate permissions, but we can still discuss it if you think it's a must-have.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

If we are storing isolation in the principal as an extended attribute, then IMO we must also isolate permissions.  Permissions are per-origin and the principal is our representation of the origin concept.  If you don't want to isolate everywhere we use principals then don't stick this attribute in the principal.

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8790629 [details] [diff] [review]
Ignore userContextId and firstPartyDomain when matching permissions.

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

::: caps/BasePrincipal.cpp
@@ +717,5 @@
>    return BasePrincipal::CreateCodebasePrincipal(uri, attrs);
>  }
>  
> +already_AddRefed<BasePrincipal>
> +BasePrincipal::StripUserContextIdAndFirstPartyDomain()

CloneStrippingUserContextIdAndFirstPartDomain ?

@@ +721,5 @@
> +BasePrincipal::StripUserContextIdAndFirstPartyDomain()
> +{
> +  PrincipalOriginAttributes attrs = OriginAttributesRef();
> +  attrs.mUserContextId = nsScriptSecurityManager::DEFAULT_USER_CONTEXT_ID;
> +  attrs.mFirstPartyDomain = EmptyString();

Truncate() ?

@@ +728,5 @@
> +  nsresult rv = GetOriginNoSuffix(originNoSuffix);
> +  NS_ENSURE_SUCCESS(rv, nullptr);
> +
> +  nsCOMPtr<nsIURI> uri;
> +  rv = NS_NewURI(getter_AddRefs(uri), originNoSuffix);

NS_ENSURE_SUCCESS(rv, nullptr);

::: extensions/cookie/nsPermission.cpp
@@ +24,5 @@
>   , mExpireType(aExpireType)
>   , mExpireTime(aExpireTime)
>  {
> +  mPrincipal =
> +    mozilla::BasePrincipal::Cast(aPrincipal)->StripUserContextIdAndFirstPartyDomain();

mPrincipal can be null here, right? You should handle this.
I would make this:

1. already_AddRefed<nsPrincipal> nsPrincipal::Create(...)
2. make nsPrincipal CTOR private.

@@ +72,5 @@
>  
>    *aMatches = false;
>  
> +  nsCOMPtr<nsIPrincipal> principal =
> +    mozilla::BasePrincipal::Cast(aPrincipal)->StripUserContextIdAndFirstPartyDomain();

if (!principal) {
  *aMatch = false;
  return NS_OK;
}

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8790629 [details] [diff] [review]
Ignore userContextId and firstPartyDomain when matching permissions.

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

::: extensions/cookie/nsPermission.cpp
@@ +24,5 @@
>   , mExpireType(aExpireType)
>   , mExpireTime(aExpireTime)
>  {
> +  mPrincipal =
> +    mozilla::BasePrincipal::Cast(aPrincipal)->StripUserContextIdAndFirstPartyDomain();

Sorry: what I meant was:

1. already_AddRefed<nsPermission> nsPermission::Create(...)
2. make nsPermission CTOR private

Comment 8

[Jonathan Hao (inactive) [:jhao]]

Attachment: Ignore userContextId and firstPartyDomain when matching permissions.

Hi Baku,

Would you take a look again, please?

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8794308 [details] [diff] [review]
Ignore userContextId and firstPartyDomain when matching permissions.

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

::: extensions/cookie/nsPermission.cpp
@@ +39,5 @@
> +    mozilla::BasePrincipal::Cast(aPrincipal)->CloneStrippingUserContextIdAndFirstPartyDomain();
> +
> +  NS_ENSURE_TRUE(principal, nullptr);
> +
> +  RefPtr<nsPermission> permission = new nsPermission(principal, aType, aCapability, aExpireType, aExpireTime);

80 chars max.

::: extensions/cookie/nsPermissionManager.cpp
@@ +2058,5 @@
> +  nsCOMPtr<nsIPermission> r = nsPermission::Create(principal,
> +                                                   mTypeArray.ElementAt(perm.mType),
> +                                                   perm.mPermission,
> +                                                   perm.mExpireType,
> +                                                   perm.mExpireTime);

if (NS_WARN_IF(!r)) {
  return NS_ERROR_FAILURE;
}

@@ +2237,5 @@
> +        nsPermission::Create(principal,
> +                             mTypeArray.ElementAt(permEntry.mType),
> +                             permEntry.mPermission,
> +                             permEntry.mExpireType,
> +                             permEntry.mExpireTime);

if (NS_WARN_IF(!permission)) {
  continue;
}

@@ +2268,5 @@
> +        nsPermission::Create(principal,
> +                             mTypeArray.ElementAt(permEntry.mType),
> +                             permEntry.mPermission,
> +                             permEntry.mExpireType,
> +                             permEntry.mExpireTime);

same here.

@@ +2319,5 @@
> +        nsPermission::Create(principal,
> +                             mTypeArray.ElementAt(permEntry.mType),
> +                             permEntry.mPermission,
> +                             permEntry.mExpireType,
> +                             permEntry.mExpireTime);

same here.

@@ +2392,5 @@
> +        nsPermission::Create(principal,
> +                             mTypeArray.ElementAt(permEntry.mType),
> +                             permEntry.mPermission,
> +                             permEntry.mExpireType,
> +                             permEntry.mExpireTime);

same here.

Comment 10

[Jonathan Hao (inactive) [:jhao]]

Attachment: Ignore userContextId and firstPartyDomain when matching permissions.

Thank you, baku.

Comment 11

[Jonathan Hao (inactive) [:jhao]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b747b6b2db17
Test if permission matches for different userContextId or firstPartyDomain. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/299a09f24493
Ignore userContextId and firstPartyDomain when matching permissions. r=baku

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b747b6b2db17
https://hg.mozilla.org/mozilla-central/rev/299a09f24493