Content sandbox rules should use actual profile directory, not Profiles/*/ regex's

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: haik, Assigned: haik)

Tracking

50 Branch
mozilla51
Unspecified
macOS
Points:
---

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

(Whiteboard: sbmc1)

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

3 years ago
This bug is to address two issues with the way paths to the Firefox profiles are handled in the OS X content sandbox. Using the actual profile path in the ruleset instead of hardcoded ~/Library/Application Support/Firefox/Profiles/*/ regex's would solve both problems.

1) The ruleset prevents read/write access to ~/Library. There are exceptions for various paths within ~/Library including the extensions and weave subdirectories of the Firefox profiles and this is implemented with regex's:

  // from security/sandbox/mac/Sandbox.mm

  (allow file-read*
    ...
    (home-regex \"/Library/Application Support/Firefox/Profiles/[^/]+/extensions/\")
    (home-regex \"/Library/Application Support/Firefox/Profiles/[^/]+/weave/\"))

The profiles are stored in ~/Library/Application Support/Firefox/Profiles so these regex's allow the content process to read from the "extensions" and "weave" subdirectories of _all_ profiles, but instead we should only allow access to these subdirectories in the in-use profile.

2) The profile might not be in ~/Library/Application Support/Firefox/Profiles because the user can choose to store the profile somewhere else using the profile manager. In that case, these regex's won't work.
Assignee

Updated

3 years ago
Summary: content sandbox rules should use actual profile directory, not Profiles/*/ regex's → Content sandbox rules should use actual profile directory, not Profiles/*/ regex's
Assignee

Updated

3 years ago
Assignee: nobody → haftandilian

Updated

3 years ago
Whiteboard: sbmc1
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8781213 - Flags: review?(jmathies)

Comment 2

3 years ago
mozreview-review
Comment on attachment 8781213 [details]
Bug 1290619 - Content sandbox rules should use actual profile directory, not Profiles/*/ regex's;

https://reviewboard.mozilla.org/r/70774/#review69418

::: dom/ipc/ContentProcess.cpp:125
(Diff revision 1)
> +{
> +  bool flag;
> +  nsresult rv =
> +    XRE_GetFileFromPath(aProfile.BeginReading(), getter_AddRefs(mProfileDir));
> +  if (NS_FAILED(rv) ||
> +      NS_FAILED(mProfileDir->Exists(&flag)) || !flag) {

Are we going to run into child process file system read permissions for XRE_GetFileFromPath  and  mProfileDir->Exists?

::: toolkit/xre/nsEmbedFunctions.cpp:621
(Diff revision 1)
> +#endif
> +
>            for (int idx = aArgc; idx > 0; idx--) {
>              if (aArgv[idx] && !strcmp(aArgv[idx], "-appdir")) {
> +              MOZ_ASSERT(!foundAppdir);
> +              if (foundAppdir) {

the extra foundAppdir and foundProfile checks seem pointless. we assert to catch a screwup so I don't think we need the extra checks.
Attachment #8781213 - Flags: review?(jmathies) → review+
Assignee

Comment 3

3 years ago
mozreview-review-reply
Comment on attachment 8781213 [details]
Bug 1290619 - Content sandbox rules should use actual profile directory, not Profiles/*/ regex's;

https://reviewboard.mozilla.org/r/70774/#review69418

> Are we going to run into child process file system read permissions for XRE_GetFileFromPath  and  mProfileDir->Exists?

ContentProcess::SetProfile() is called during process startup, before content sandbox restrictions are turned on. So we should be OK on that front. We turn on the sandbox restrictions in the handling of the SetProcessSandbox message (called out of ContentChild::RecvSetProcessSandbox).

> the extra foundAppdir and foundProfile checks seem pointless. we assert to catch a screwup so I don't think we need the extra checks.

They're not necessary, but they're there so we can break out of the loop once the profileDir and appDir are found, saving a few cycles in cases where there are other command line arguments after -appdir and -profile. If that seems overly complex for the small savings, I'll remove those lines.
Assignee

Comment 4

3 years ago
mozreview-review
Comment on attachment 8781213 [details]
Bug 1290619 - Content sandbox rules should use actual profile directory, not Profiles/*/ regex's;

https://reviewboard.mozilla.org/r/70774/#review69990

::: security/sandbox/mac/Sandbox.mm:374
(Diff revision 1)
>    "  (allow file-read*\n"
>    "      (home-regex \"/Library/Application Support/[^/]+/Extensions/[^/]/\")\n"
>    "      (resolving-regex \"/Library/Application Support/[^/]+/Extensions/[^/]/\")\n"
> -  "      (home-regex \"/Library/Application Support/Firefox/Profiles/[^/]+/extensions/\")\n"
> -  "      (home-regex \"/Library/Application Support/Firefox/Profiles/[^/]+/weave/\"))\n"
> +  "      (profile-subpath \"/extensions\")\n"
> +  "      (profile-subpath \"/weave\"))\n"
>    "\n"
>    "; the following rules should be removed when printing and \n"
>    "; opening a file from disk are brokered through the main process\n"
>    "  (if\n"
>    "    (< sandbox-level 2)\n"
>    "    (allow file*\n"
>    "        (require-not\n"
>    "            (home-subpath \"/Library\")))\n"

I realized there's an issue with the fix as is. Previously, the profile was read/write protected except for extensions/ and weave/. With the change, if the user places their profile outside of ~/Library, the read/write protection will be lost. I'll update the code with the fix.

We need an additional rule to block read/write access to the profile that works when the profile is not in ~/Library.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 7

3 years ago
Posting here as a patch because I butchered the review board review. This is a change (the same as the 2nd diff on reviewboard) that needs review. The change makes sure that read access to the profile dir is blocked when the profile is placed outside of ~/Library.
Attachment #8783651 - Flags: review?(jmathies)
Comment on attachment 8783651 [details] [diff] [review]
Continue to block read access to profile when it's not within ~/Library

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

I see now we embed the profile directory into the filter string which we then feed into the sandbox. I missed that in the previous review.
Attachment #8783651 - Flags: review?(jmathies) → review+
Assignee

Comment 9

3 years ago
The full reviewed patch.

With the patch, the profile dir is passed to the content process as a -profile CLI option so that the correct profile dir can be used in the OS X content sandbox rules. Only enabled on OS X for now. And profile directories will now be read/write protected from the content process (apart from a few profile subdirectories) even when they don't reside in ~/Library. Previously, profiles were read/write protected only if they were in the default location ~/Library/Application Support/Firefox/Profiles.

Try results:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=524f93eb4ce3
Attachment #8781213 - Attachment is obsolete: true
Attachment #8783651 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 10

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b357fab2feb4
Content sandbox rules should use actual profile directory, not Profiles/*/ regex's. r=jimm
Keywords: checkin-needed
I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4e65c70d7a66 because it appears to have caused lots of e10s crashes on OSX:

https://treeherder.mozilla.org/logviewer.html#?job_id=34599497&repo=mozilla-inbound#L2002
Flags: needinfo?(haftandilian)
Assignee

Comment 12

3 years ago
Haven't been able to reproduce this locally, but I think the problem is caused by both ContentProcess and ContentChild having member nsCOMPtr's pointing to the same nsIFile, but in ContentChild, the nsCOMPtr is initialized from a raw pointer. Working on a reproducing the problem.
Flags: needinfo?(haftandilian)
Assignee

Comment 14

3 years ago
Updated patch to address the crashes on inbound. Carrying forward jimm's review. Discussed the problem with jimm.

The problem was that GetProfileDir is called using getter_AddRefs from StartMacOSContentSandbox, but GetProfileDir doesn't add ref before returning. As a result reference counting was off by one resulting in a crash when ContentChild is destructed. The attempted fix in comment #13 was wrong. The updated patch adds

   void GetProfileDir(nsIFile** aProfileDir) const
   {
     *aProfileDir = mProfileDir;
+    NS_IF_ADDREF(*aProfileDir);
   }

New try results:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b41c3bb28f7f

I don't think the debug non-e10s mochitest failures are related to the patch because the changes are e10s-specific.
Attachment #8784112 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 15

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f53bc1a9aea
Content sandbox rules should use actual profile directory, not Profiles/*/ regexes. r=jimm
Keywords: checkin-needed
Assignee

Comment 17

3 years ago
Sorry about that. I'm looking into it.
Flags: needinfo?(haftandilian)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 20

3 years ago
The problem with the previous push was that xpcshell tests cause plugin-container to be invoked without a profile directory and I assumed a profile directory would always be provided. On an xpcshell test, ContentChild::GetProfileDir() returned a NULL profileDir causing StartMacOSContentSandbox to perform a NULL dereference causing the crash.

I've added logic so that a profile directory isn't required and when plugin-container is invoked without a profile, the profile-specific filesystem rules are not added to the content sandbox.
Assignee

Updated

3 years ago
Attachment #8785173 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8781213 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8781213 - Attachment is obsolete: false
Assignee

Updated

3 years ago
Attachment #8781213 - Flags: review+ → review?(jmathies)

Comment 21

3 years ago
mozreview-review
Comment on attachment 8781213 [details]
Bug 1290619 - Content sandbox rules should use actual profile directory, not Profiles/*/ regex's;

https://reviewboard.mozilla.org/r/70774/#review75040

::: dom/ipc/ContentChild.cpp:1328
(Diff revision 5)
>    info.appPath.assign(appPath.get());
>    info.appBinaryPath.assign(appBinaryPath.get());
>    info.appDir.assign(appDir.get());
>    info.appTempDir.assign(tempDirPath.get());
>  
> +  if (profileDir) {

This could be a check on profileDirPath.IsEmpty() here to insure we have a path to provide.
Attachment #8781213 - Flags: review?(jmathies) → review+
Comment hidden (mozreview-request)

Comment 24

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/69b7d6494d13
Content sandbox rules should use actual profile directory, not Profiles/*/ regex's; r=jimm
Keywords: checkin-needed

Comment 25

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/69b7d6494d13
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.