Closed Bug 1237479 Opened 4 years ago Closed 4 years ago
Script Security Manager needs to use the correct user context id in the origin attributes in a few places .
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.
Comment on attachment 8717720 [details] [diff] [review] Bug_1237479.patch this one is good to go.
Attachment #8717720 - Flags: review?(jonas)
Comment on attachment 8717720 [details] [diff] [review] Bug_1237479.patch 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. So PrincipalOriginAttributes attrs; aPrincipal->GetOriginAttributes(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]
already r+ by :sicking. this rebases the patch and adds a better commit message.
r+ by :sicking. rebasing to fix compile error.
Whiteboard: [userContextId] [domsecurity-active] → [userContextId] [domsecurity-active][OA]
You need to log in before you can comment on or make changes to this bug.