Closed Bug 1302047 Opened 3 years ago Closed 3 years ago

Ignore userContextId and firstPartyDomain when matching permissions.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jhao, Assigned: jhao)

Details

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

Attachments

(2 files, 2 obsolete files)

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
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.
Priority: -- → P2
Hi baku, please review these patches.  They're just like bug 1301274 and bug 1301617.
Attachment #8790629 - Flags: review?(amarchesini)
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.
Attachment #8790632 - Flags: review?(amarchesini)
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 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;
}
Attachment #8790629 - Flags: review?(amarchesini) → review-
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
Hi Baku,

Would you take a look again, please?
Attachment #8794308 - Flags: review?(amarchesini)
Attachment #8790629 - Attachment is obsolete: true
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.
Attachment #8794308 - Flags: review?(amarchesini) → review+
Attachment #8794308 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b747b6b2db17
https://hg.mozilla.org/mozilla-central/rev/299a09f24493
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.