AppProcessChecker needs to stop using createCodebasePrincipalFromOrigin

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: huseby, Assigned: baku)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [OA])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
in the file dom/ipc/AppProcessChecker.cpp there's this code:

> 138   nsCOMPtr<nsIPrincipal> principal;
> 139   securityManager->CreateCodebasePrincipalFromOrigin(aOrigin,
> 140                                                      getter_AddRefs(principal));
> 141
> 142   nsCOMPtr<nsIPermissionManager> permMgr = services::GetPermissionManager();
> 143   NS_ENSURE_TRUE(permMgr, false);
> 144
> 145   uint32_t perm;
> 146   nsresult rv = permMgr->TestExactPermissionFromPrincipal(principal, aPermission, &perm);
> 147   NS_ENSURE_SUCCESS(rv, false);

Since this is doing permission checking, the solution is to populate an origin attribute from the origin, then create a GlobalContextOriginAttribute from that and then use createCodebasePrincipal passing the origin and origin attributes instead.
(Assignee)

Comment 1

3 years ago
Created attachment 8702982 [details] [diff] [review]
patch
Assignee: huseby → amarchesini
Attachment #8702982 - Flags: review?(huseby)
(Reporter)

Comment 2

3 years ago
Comment on attachment 8702982 [details] [diff] [review]
patch

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

This is correct except that the class is called DefaultContextOriginAttributes.  It's gone through several naming revisions so the confusion is understandable.  The latest patch that creates it is here: https://bugzilla.mozilla.org/attachment.cgi?id=8704382&action=diff  I just recently added the PopulateFrom*() member functions to make sure that the mUserContextId gets forced to the default value *after* the populate process happens.

I r+ it after you fix the class name.
Attachment #8702982 - Flags: review?(huseby) → review-
(Assignee)

Comment 3

3 years ago
Created attachment 8704569 [details] [diff] [review]
bug_1235668.patch
Attachment #8702982 - Attachment is obsolete: true
Attachment #8704569 - Flags: review?(huseby)
(Assignee)

Updated

3 years ago
Depends on: 1229222
(Assignee)

Updated

3 years ago
Blocks: 1235661
(Assignee)

Updated

3 years ago
No longer blocks: 1235661
(Reporter)

Comment 4

3 years ago
Comment on attachment 8704569 [details] [diff] [review]
bug_1235668.patch

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

This can't land until the patch in 122922 lands.
Attachment #8704569 - Flags: review?(huseby) → review+
(Reporter)

Comment 5

3 years ago
This patch is not needed since permissions aren't isolated by origin attributes.  For this bug in particular, here's why we don't need it.

* First it creates a codebase principal using an origin here: http://mzl.la/1SlTHBw
* Then it uses that principal to test for the permission by calling TestExactPermissionFromPrincipal here: http://mzl.la/1SlTIVX
* TestExactPermissionFromPrincipal calls CommonTestPermission: http://mzl.la/1SlTNJl
* CommonTestPermission calls GetPermissionHashKey here: http://mzl.la/1SlTQVp
* GetPermissionHashKey creates a PermissionKey from the principal here: http://mzl.la/1SlTTkd
* PermissionKey only uses the origin and *NOT* the origin attributes to create the hash string for looking up the permission: http://mzl.la/1SlTY7k

Therefore we do not need to worry about getting the correct origin attributes into the principal we create.  The default origin attributes are just fine.

Closing WON'T FIX.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
(Reporter)

Updated

3 years ago
Whiteboard: [OA]
You need to log in before you can comment on or make changes to this bug.