Closed Bug 1290619 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect)

50 Branch
Unspecified
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: haik, Assigned: haik)

Details

(Whiteboard: sbmc1)

Attachments

(1 file, 3 obsolete files)

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.
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: nobody → haftandilian
Whiteboard: sbmc1
Attachment #8781213 - Flags: review?(jmathies)
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+
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.
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.
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+
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
Keywords: checkin-needed
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
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)
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
Keywords: checkin-needed
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
Sorry about that. I'm looking into it.
Flags: needinfo?(haftandilian)
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.
Attachment #8785173 - Attachment is obsolete: true
Attachment #8781213 - Attachment is obsolete: true
Attachment #8781213 - Attachment is obsolete: false
Attachment #8781213 - Flags: review+ → review?(jmathies)
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+
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
https://hg.mozilla.org/mozilla-central/rev/69b7d6494d13
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.