Closed Bug 1361304 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Security, enhancement)

Product:
Core
Component:
Security
Version:
55 Branch
Platform:
Unspecified
macOS
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: haik, Assigned: haik)

References

(Blocks 1 open bug)

Details

(Whiteboard: sbmc2)

Attachments

(1 file)

Bug 1361304 - Remove /private/var read access from Mac level 3 content sandbox;
59 bytes, text/x-review-board-request
Alex_Gaynor
: review+
Details 
/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: nobody → haftandilian
Whiteboard: sbmc2
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?
(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
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
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 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?
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/26a2a52b1108
Status: NEW → RESOLVED
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: