Closed Bug 1237479 Opened 4 years ago Closed 4 years ago

nsScriptSecurityManager needs to use the correct user context id in the origin attributes in a few places.


(Core :: DOM: Security, defect)

Not set



Tracking Status
firefox48 --- fixed


(Reporter: huseby, Assigned: huseby)


(Blocks 1 open bug)


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


(1 file, 2 obsolete files)

There are a number of calls in caps/nsScriptSecurityManager.cpp that need to be fixed to properly handle the user context id.
I'm not sure what to do with this call site:

> 294     PrincipalOriginAttributes attrs(appId, false);
> 295     nsCOMPtr<nsIPrincipal> appPrin = BasePrincipal::CreateCodebasePrincipal(appURI, attrs);
> 296     NS_ENSURE_TRUE(appPrin, nsIPrincipal::APP_STATUS_NOT_INSTALLED);
> 297     return aPrin->Equals(appPrin) ? status
> 298                                   : nsIPrincipal::APP_STATUS_NOT_INSTALLED;

The call to aPrin->Equals(appPrin) will compare the origin attributes on the two principals.  If aPrin has a user context set, then the Equals call will return false because the attrs used to create appPrin won't have a user context.  I think the correct thing to do here is to manually copy the user context id from the aPrin origin attributes into the attrs origin attributes before calling CreateCodebasePrincipal.
The next call site is interesting:

> 408     //TODO: Bug 1211590. inherit Origin Attributes from LoadInfo.
> 409     PrincipalOriginAttributes attrs(UNKNOWN_APP_ID, false);
> 410     rv = MaybeSetAddonIdFromURI(attrs, uri);
> 411     NS_ENSURE_SUCCESS(rv, rv);
> 412     nsCOMPtr<nsIPrincipal> prin = BasePrincipal::CreateCodebasePrincipal(uri, attrs);
> 413     prin.forget(aPrincipal);
> 414     return *aPrincipal ? NS_OK : NS_ERROR_FAILURE;

We should probably do the TODO and inherit from the LoadInfo which should be necko origin attributes.  If we do the plumbing right, the necko origin attributes should have the correct user context id set from the docshell that the necko origin attributes were inherited from.
I'm not sure what to do with this one:

> 1102   PrincipalOriginAttributes attrs(aAppId, aInMozBrowser);                       
> 1103   nsCOMPtr<nsIPrincipal> prin = BasePrincipal::CreateCodebasePrincipal(aURI, attrs);

I'm pretty sure that with apps, we should be using the default user context id.  That would just require changing the PrincipalOriginAttributes object to a DefaultContextOriginAttributes object.
Whiteboard: [userContextId]
Attached patch Bug_1237479.patch (obsolete) — Splinter Review
Comment on attachment 8717720 [details] [diff] [review]

this one is good to go.
Attachment #8717720 - Flags: review?(jonas)
Comment on attachment 8717720 [details] [diff] [review]

Review of attachment 8717720 [details] [diff] [review]:

r=me with that fixed.

::: caps/nsScriptSecurityManager.cpp
@@ +296,5 @@
> +    PrincipalOriginAttributes attrs;
> +    NS_ENSURE_TRUE(attrs.PopulateFromOrigin(NS_ConvertUTF16toUTF8(aOrigin), suffix),
> +                   nsIPrincipal::APP_STATUS_NOT_INSTALLED);
> +    attrs.mAppId = appId;
> +    attrs.mInBrowser = false;

What is 'aOrigin'? That doesn't seem to exist in this function. Does this compile?

Anyhow, I think you're making this more complicated than it needs to be. What you want to do is simply grab the OAs from aPrincipal.

PrincipalOriginAttributes attrs;
appPrin = ...CreateCodebasePrincipal(appURI, attrs);

The only reason it's passing 'false' for mInBrowser is because we already know that the aPrincipal.OA.mInBrowser flag is false. See the if-statement in the beginning of this function.
Attachment #8717720 - Flags: review?(jonas) → review+
Whiteboard: [userContextId] → [userContextId] [domsecurity-active]
Attached patch Bug_1237479.patch (obsolete) — Splinter Review
already r+ by :sicking.  this rebases the patch and adds a better commit message.
Attachment #8717720 - Attachment is obsolete: true
Attachment #8737427 - Flags: review+
r+ by :sicking.  rebasing to fix compile error.
Attachment #8737427 - Attachment is obsolete: true
Attachment #8737470 - Flags: review+
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Whiteboard: [userContextId] [domsecurity-active] → [userContextId] [domsecurity-active][OA]
You need to log in before you can comment on or make changes to this bug.