1428055 - Deny access to some properties from the mac sandbox which are included in the default permissions

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

[Alex Gaynor [:Alex_Gaynor]]

Our policies all use |(deny default)| but it turns out even with this there are still some other permissions that can be denied by the process!

We can restrict some of these further.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1428055 - Further lockdown the macOS content sandbox policy by restricting some allowed-by-default privileges;

Review commit: https://reviewboard.mozilla.org/r/210178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210178/

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8939863 [details]
Bug 1428055 - Further lockdown the macOS content sandbox policy by restricting some allowed-by-default privileges;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210178/diff/1-2/

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Patch covers our content process, we should do the same exercise for the plugin process (I'll file a follow up bug).

I'm not 100% confident in the file-map-executable rules, do we ever need to dlopen things besides system libraries or our own libraries?

And the iokit-get-properties list was done by random testing and see what warnings triggered, so that list may need to grow.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8a4d514bef4461d43e62c547b6388f25059180d&group_state=expanded clean try run, but would probably be good to get some testing on 10.9 (it's frustrating that we support 10.9 but don't have it in CI)

Comment 5

[Haik Aftandilian [:haik]]

Comment on attachment 8939863 [details]
Bug 1428055 - Further lockdown the macOS content sandbox policy by restricting some allowed-by-default privileges;

https://reviewboard.mozilla.org/r/210178/#review215942

::: security/sandbox/mac/SandboxPolicies.h:11
(Diff revision 2)
>  #ifndef mozilla_SandboxPolicies_h
>  #define mozilla_SandboxPolicies_h
>  
>  namespace mozilla {
>  
>  static const char pluginSandboxRules[] = R"(

These should be beneficial to the plugin process too. But that could be another bug.

::: security/sandbox/mac/SandboxPolicies.h:69
(Diff revision 2)
>  
>    (if (string=? should-log "TRUE")
>      (deny default)
>      (deny default (with no-log)))
>    (debug deny)
> +  ; These are not included in the (deny default) for whatever reason.

Could you remove the "for whatever reason"?

::: security/sandbox/mac/SandboxPolicies.h:72
(Diff revision 2)
> +  (if (defined? 'nvram*)
> +    (deny nvram*))

Do we think this is the same functionality offered by the nvram(8) command?

::: security/sandbox/mac/SandboxPolicies.h:75
(Diff revision 2)
> +  (deny file-map-executable iokit-get-properties process-info*)
> +  ; This isn't available in some older macOS releases.
> +  (if (defined? 'nvram*)
> +    (deny nvram*))
> +
> +  (allow file-map-executable

Could we combine the file-read allow rules with the file-map-executable so that we only list each path once? I think that might me clearer. For example have a section

(allow file-read* file-map-executable 
  (subpath /System)
  (subpath /usr/lib)
  ...
)

Assuming it doesn't turns out to be too ugly/verbose.

::: security/sandbox/mac/SandboxPolicies.h:85
(Diff revision 2)
>    ; Allow read access to standard system paths.
>    (allow file-read*
>      (require-all (file-mode #o0004)
>        (require-any
>          (subpath "/Library/Filesystems/NetFSPlugins")
>          (subpath "/Library/GPUBundles")

This might need file-map-executable. I'll try with the Nvidia downloaded driver.

::: security/sandbox/mac/SandboxPolicies.h:113
(Diff revision 2)
>    (allow file-read*
>      file-write-data
>      file-ioctl
>      (literal "/dev/dtracehelper"))
>  
> +  (allow process-info-pidinfo process-info-setcontrol (target self))

Could you provide an explanation either as a comment here or more of an explanation on the bug report?

Comment 6

[Haik Aftandilian [:haik]]

Comment on attachment 8939863 [details]
Bug 1428055 - Further lockdown the macOS content sandbox policy by restricting some allowed-by-default privileges;

https://reviewboard.mozilla.org/r/210178/#review215942

I accidentally hit publish too soon.

In general, could you document on the bug what we think the effect of the additions are and how you figured that out?

Otherwise, these seem like nice defense-in-depth improvements.

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

For future reference, here's my understanding of what each of these knobs does:

* file-map-executable: dlopen or mmap with fd and PROT_EXEC. This is relatively intuitive from the name, and confirmed by the fact that without the additional (allow file-map-executable) rules I added, we were failing with things that rely on lazy-loading a DLL, e.g. webcrypto and some AV stuff.
* iokit-get-properties: Don't know :-) From the name I'd guess it allows reading constants related to iokits, but I couldn't say more than that. All the ones I allow were determined by test browsing and seeing what I got denied messages in console from.
* process-info*: The full list of APIs this covers are: process-info-codesignature, process-info-dirtycontrol, process-info-listpids, process-info-rusage, process-info-pidinfo, process-info-pidfdinfo, process-info-pidfileportinfo, process-info-setcontrol. I don't have a full understanding of what each does, but in general this blocks the ability of a content process to interact with other processes on the system, we allow interacting with the current process of course.
* nvram*: The equivalent of /usr/sbin/nvram. Confirmed with `sandbox-exec -p '(version 1) (allow default) (deny nvram*)' /usr/sbin/nvram -p`. /usr/sbin/nvram is obviously a setuid binary, so I'm not sure how much you can do with nvram from a regular user anyways, but better safe than sorry!

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8939863 [details]
Bug 1428055 - Further lockdown the macOS content sandbox policy by restricting some allowed-by-default privileges;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210178/diff/2-3/

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8939863 [details]
Bug 1428055 - Further lockdown the macOS content sandbox policy by restricting some allowed-by-default privileges;

https://reviewboard.mozilla.org/r/210178/#review215942

> These should be beneficial to the plugin process too. But that could be another bug.

Bug 1428361 filed.

> Do we think this is the same functionality offered by the nvram(8) command?

Yup, summary is on the bug.

> This might need file-map-executable. I'll try with the Nvidia downloaded driver.

Let me know what you find; I fear there's going to be a bit of trial and error here :-/

Comment 10

[Haik Aftandilian [:haik]]

Comment on attachment 8939863 [details]
Bug 1428055 - Further lockdown the macOS content sandbox policy by restricting some allowed-by-default privileges;

https://reviewboard.mozilla.org/r/210178/#review215942

> Let me know what you find; I fear there's going to be a bit of trial and error here :-/

Sorry, I didn't realize the Nvidia Mac I have available is too old to use the downloadable drivers. It seems safe to put this in the file-map-executable list given that it's going to contain libraries we need.

I worry this will get to release without getting enough coverage on different Mac hardware, but if we can get some extra test coverage and evaluate before we go to release that would satisfy my concern.

Comment 11

[Haik Aftandilian [:haik]]

Comment on attachment 8939863 [details]
Bug 1428055 - Further lockdown the macOS content sandbox policy by restricting some allowed-by-default privileges;

https://reviewboard.mozilla.org/r/210178/#review216228

Thanks. Looks good. See comments on testing.

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

Haik and I agreed to defer landing this until after the next branch point so that we get a full 6 weeks on nightly for testing.

Comment 13

[Haik Aftandilian [:haik]]

I ran some tests on OS X 10.9.5 and it turns out that the file-map-executable and iokit-get-properties restrictions aren't supported there. With those entries removed, browsing worked and I didn't see any new sandboxing errors/warnings logged in Console.app. This was on a MacBook 13-inch Aluminum, Late 2008.

Comment 14

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8939863 [details]
Bug 1428055 - Further lockdown the macOS content sandbox policy by restricting some allowed-by-default privileges;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210178/diff/3-4/

Comment 15

[Alex Gaynor [:Alex_Gaynor]]

I've updated the patch to make the file-map-executable and iokit-get-properties rules conditional on them being available. It makes it a bit messier, but hopefully some day we'll get to drop 10.9 :-)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a678eec13c6e
Further lockdown the macOS content sandbox policy by restricting some allowed-by-default privileges; r=haik

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a678eec13c6e