Closed
Bug 1237479
Opened 8 years ago
Closed 8 years ago
nsScriptSecurityManager needs to use the correct user context id in the origin attributes in a few places.
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: huseby, Assigned: huseby)
References
Details
(Whiteboard: [userContextId] [domsecurity-active][OA])
Attachments
(1 file, 2 obsolete files)
1.60 KB,
patch
|
huseby
:
review+
|
Details | Diff | Splinter Review |
There are a number of calls in caps/nsScriptSecurityManager.cpp that need to be fixed to properly handle the user context id.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Whiteboard: [userContextId]
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Comment 5•8 years ago
|
||
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+
Updated•8 years ago
|
Whiteboard: [userContextId] → [userContextId] [domsecurity-active]
Assignee | ||
Comment 7•8 years ago
|
||
already r+ by :sicking. this rebases the patch and adds a better commit message.
Attachment #8717720 -
Attachment is obsolete: true
Attachment #8737427 -
Flags: review+
Assignee | ||
Comment 8•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=010d530b3791
Assignee | ||
Comment 9•8 years ago
|
||
r+ by :sicking. rebasing to fix compile error.
Attachment #8737427 -
Attachment is obsolete: true
Attachment #8737470 -
Flags: review+
Assignee | ||
Comment 10•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc9facf097f1
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8038df809897
Keywords: checkin-needed
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8038df809897
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Updated•8 years ago
|
Whiteboard: [userContextId] [domsecurity-active] → [userContextId] [domsecurity-active][OA]
You need to log in
before you can comment on or make changes to this bug.
Description
•