1235668 - AppProcessChecker needs to stop using createCodebasePrincipalFromOrigin

Status: RESOLVED WONTFIX

(Firefox :: Security, defect)

Description

[Dave Huseby [:huseby]]

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Dave Huseby [:huseby]]

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.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: bug_1235668.patch

Comment 4

[Dave Huseby [:huseby]]

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.

Comment 5

[Dave Huseby [:huseby]]

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.