All users were logged out of Bugzilla on October 13th, 2018

[Mac] Remove /private/var read access from level 3 Content Sandbox

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: haik, Assigned: haik)

Tracking

(Blocks: 1 bug)

55 Branch
mozilla55
Unspecified
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: sbmc2)

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
/private/var, which is linked to from /var on OS X, contains user files so blocking read access to this directory from content processes would be beneficial.
(Assignee)

Updated

2 years ago
Assignee: nobody → haftandilian
Whiteboard: sbmc2
Blocks: 1359559
I started looking at this, by looking at this one: https://dxr.mozilla.org/mozilla-central/source/security/sandbox/mac/SandboxPolicies.h#332-334 it's debug only, but I figured it'd be a good starting place. While cutting off this permission doesn't affect users really (since it's debug only), it's useful for devs to get a consistent experience and things not to break when they move to non-debug builds.

The thing this appears to be needed for is writing to |XPCOM_MEM_BLOAT_LOG| when the process intentionally crashes: https://dxr.mozilla.org/mozilla-central/source/xpcom/base/IntentionalCrash.h#24-56

|XPCOM_MEM_BLOAT_LOG| is located at: /var/folders/21/3kp8v0nx0ds4xpp12th8vydh0000gn/T/tmpi1A8ZY.mozrunner/runtests_leaks.log (which is inside the profile dir, but that's an implementation detail, as far as I can tell).

Right now the only directories we allow writes to are the content temp directory and |/org\.chromium\.[a-zA-Z0-9]*$|.

So our options are:

a) Move the file to be in one of these directories -- I'm not sure how the lifecycle for the content temp directory works, would mach be able to read from it after the process exits?
b) Special case the |XPCOM_MEM_BLOAT_LOG| env var in the sandbox policy, this seems crumby, though easy.
c) Remote the write, if (a) isn't possible this one makes the most sense to me.

Thoughts?
(Assignee)

Comment 2

2 years ago
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #1)
> I started looking at this, by looking at this one:
> https://dxr.mozilla.org/mozilla-central/source/security/sandbox/mac/
> SandboxPolicies.h#332-334 it's debug only, but I figured it'd be a good
> starting place. While cutting off this permission doesn't affect users
> really (since it's debug only), it's useful for devs to get a consistent
> experience and things not to break when they move to non-debug builds.

Sounds good, but lets use another bug for this.

> The thing this appears to be needed for is writing to |XPCOM_MEM_BLOAT_LOG|
> when the process intentionally crashes:
> https://dxr.mozilla.org/mozilla-central/source/xpcom/base/IntentionalCrash.
> h#24-56

The rule was added for the leaktest which is DEBUG-only. IIRC, we were doing the same special-casing on Windows so it would be great if the solution could apply there to. So I wonder if XPCOM_MEM_BLOAT_LOG is the same file or something different. See

  https://bugzilla.mozilla.org/show_bug.cgi?id=1284588#c13

> |XPCOM_MEM_BLOAT_LOG| is located at:
> /var/folders/21/3kp8v0nx0ds4xpp12th8vydh0000gn/T/tmpi1A8ZY.mozrunner/
> runtests_leaks.log (which is inside the profile dir, but that's an
> implementation detail, as far as I can tell).
> 
> Right now the only directories we allow writes to are the content temp
> directory and |/org\.chromium\.[a-zA-Z0-9]*$|.
> 
> So our options are:
> 
> a) Move the file to be in one of these directories -- I'm not sure how the
> lifecycle for the content temp directory works, would mach be able to read
> from it after the process exits?

It gets auto-deleted by the parent and is meant to be for temporary things that don't need to outlive the content process so it would better to use a different directory.

> b) Special case the |XPCOM_MEM_BLOAT_LOG| env var in the sandbox policy,
> this seems crumby, though easy.

How is the path of the log file communicated to the content process now? If it's by environment variable, I think it's fine to take the path of least resistance and add a single rule to allow writes to that particular file in the content policy. It would be enabled on DEBUG-builds only.

> c) Remote the write, if (a) isn't possible this one makes the most sense to
> me.

I think this is worth looking into to see how much we write to the file(s). i.e., if we're writing to the file at a high frequency, the extra I/O might be problematic. And if we're writing to the file in places where it would be awkward to do async I/O, that could also be problematic.
Depends on: 1361733
(Assignee)

Comment 3

2 years ago
We don't depend on 1361733 for this bug because 1361733 applies to writing to a file in /private/var in DEBUG only.
No longer depends on: 1361733
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
The posted diffs remove read access to /private/var but continue to allow file-read-metadata. The profile or the ContentTempDir (which is a directory the content process can read and write to) can reside in /private/var and read access to those is still permitted.

Allowing file-read-metadata makes it possible to resolve symlinks that target files in the allowed subdirectories of /private/var.

I've tried to clean things up a bit by removing macros we no longer need and grouping some rules together.

For /private/var, I've added

    (allow file-read-metadata
      (literal "/private/var")
      (literal "/private/var/folders")
      (regex "^/private/var/folders/[^/][^/]/"))

It doesn't get us much extra security, but it prevents file-read-metadata in directories such as /private/var/log.

Comment 6

2 years ago
mozreview-review
Comment on attachment 8864656 [details]
Bug 1361304 - Remove /private/var read access from Mac level 3 content sandbox;

https://reviewboard.mozilla.org/r/135958/#review139666

Overall looks like a great improvement! A few small comments below.

::: security/sandbox/mac/SandboxPolicies.h:225
(Diff revision 1)
> +    (allow file-read-metadata (home-subpath "/Library"))
> +
> +    (allow file-read-metadata
> +      (literal "/private/var")
> +      (literal "/private/var/folders")
> +      (regex "^/private/var/folders/[^/][^/]/"))

Can this just be simplified to `(subpath "/private/var/folders")`? Is there anything anywhere else in `/private/var/folders`?

::: security/sandbox/mac/SandboxPolicies.h:278
(Diff revision 1)
>                    (profile-subpath "/chrome")))
>              ; we don't have a profile dir
>              (allow file-read* (require-not (home-subpath "/Library")))))))
>  
>    ; level 3: global read access permitted, no global write access,
>    ;          no read access to the home directory,

This comment needs to be updated.

::: security/sandbox/test/browser_content_sandbox_fs.js:55
(Diff revision 1)
> -    return {ok: true, numEntries: numEntries};
> +    return {ok: true, numEntries: numEntries, error: false};
>    }).catch(function () {
> -    return {ok: false, numEntries: numEntries};
> +    return {ok: false, numEntries: numEntries, error: true};

`.error` is always `!.ok`.

::: security/sandbox/test/browser_content_sandbox_fs.js:439
(Diff revision 1)
> +      ok(result.error,
> +        `encountered error listing directory (${test.file.path})`);

This should already be handled by the `ok(result.ok == test.ok)`, shouldn't it?

::: security/sandbox/test/browser_content_sandbox_utils.js:6
(Diff revision 1)
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  const uuidGenerator = Cc["@mozilla.org/uuid-generator;1"]
>                        .getService(Ci.nsIUUIDGenerator);
> +const environment = Components.classes["@mozilla.org/process/environment;1"]

nit: Use `Cc`/`Ci` for consistency?
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8864656 [details]
Bug 1361304 - Remove /private/var read access from Mac level 3 content sandbox;

https://reviewboard.mozilla.org/r/135958/#review139666

Thanks. Will post new version shortly. I realized I hadn't included the new /private/var rules in the level 3 section for when we don't have a profile. That happens with some xpcshell tests. I've added that and will re-run on try with level 3.

> Can this just be simplified to `(subpath "/private/var/folders")`? Is there anything anywhere else in `/private/var/folders`?

I've changed it to subpath in the new version. I don't have a strong argument for carrying it forward (from the older regexes.) On my systems, everyting in /private/var/folders is a 2-character directory. On one hand it is good because it prevents metadata discovery of unexpected things that show up there, but on the other hand, it makes the code more brittle. I think it's very like anything sensitive would end up in the 2-character dirs anyway.

> `.error` is always `!.ok`.

Removed the .error member.

> This should already be handled by the `ok(result.ok == test.ok)`, shouldn't it?

Yes! Thanks. So these changes were unnecessary.
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8864656 [details]
Bug 1361304 - Remove /private/var read access from Mac level 3 content sandbox;

https://reviewboard.mozilla.org/r/135958/#review139710

Assuming try is green, this LGTM.
Attachment #8864656 - Flags: review?(agaynor) → review+
(Assignee)

Comment 10

2 years ago
Try push with level3 enabled and remoted extensions patch:

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

Comment 11

2 years ago
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/26a2a52b1108
Remove /private/var read access from Mac level 3 content sandbox; r=Alex_Gaynor

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/26a2a52b1108
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.