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

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: Alex_Gaynor, Assigned: Alex_Gaynor)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Unspecified
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed)

Details

(Whiteboard: sb+)

Attachments

(1 attachment)

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.
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.
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)
Blocks: 1359559
Whiteboard: sb+
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 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.
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!
Blocks: 1428361
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 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 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.
Attachment #8939863 - Flags: review?(haftandilian) → review+
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.
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.
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 :-)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a678eec13c6e
Further lockdown the macOS content sandbox policy by restricting some allowed-by-default privileges; r=haik
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a678eec13c6e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1458553
You need to log in before you can comment on or make changes to this bug.