1376976 - [mac] restrict the sysctl-read access in content

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement)

Description

[Alex Gaynor [:Alex_Gaynor]]

Right now our sandbox policy has a blanket |(allow sysctl-read)|. There's apparently a way to restrict the which sysctls are allowed though! From |com.apple.AnnotationKit.MigratorService.sb| in |/System/Library/Sandbox/Profiles|:


    (allow sysctl-read
	(sysctl-name "hw.physicalcpu_max"))

We should copy this pattern and restrict this only to the sysctls we need! Per :jld, sysctl's can be used to obtain the mac address and other details.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

That file doesn't exist on OS X 10.10, and I don't see anything else in /System/Library/Sandbox/Profiles filtering by sysctl selector, so it's possible that this isn't doable on older OS versions; we should check that.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://opensource.apple.com/source/Libinfo/Libinfo-459.40.1/gen.subproj/getifaddrs.c.auto.html is an example of what we *don't* want to allow; even if that doesn't expose MAC addresses directly, it would show non-privacy-enabled IPv6 addresses (e.g., link-local) which contain the MAC address.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

:haik tested this out on a 10.9 box and it's sadly an error there, it looks fine on try (10.10) so we'll have to make this conditional on a newer OS.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Attachment: sysctl-lockdown.diff

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e06169bf788cc0b8b53c5f3b6f35435266e84e46&group_state=expanded

Just remembered that this still needs the 10.9 logic.

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8883574 [details] [diff] [review]
sysctl-lockdown.diff

Clearing the reviews until I add the 10.9 bits

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

Attachment: sysctl-lockdown.diff

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8883622 [details] [diff] [review]
sysctl-lockdown.diff

Review of attachment 8883622 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a little concerned about the possibility of OS updates introducing more sysctls, especially for CPU features, but I'm not seeing a way to address a sysctl subtree, so this might be the best we can do.  (The CPU info can mostly(?) be obtained from the CPUID instruction, so fine-grained restrictions on those sysctls might not actually be hiding anything.)

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

|sysctl-name-regex| does exist, if we wanted to whitelist all of |hw\.| or another tree. (Haven't checked the exact version support for it). Precise whitelist obviously gives us more control, and fails-closed making it more secure, whitelisting full trees fails-open in the event a privacy-compromising option is added to a tree we trusted (an example of this I ran into was that |hostname| is under the |kern| tree).

I could go either way, so if anyone feels strongly I'd love to hear about it.

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

(Also apparently |sysctl-name-prefix|, which is slightly prettier than -regex but functionally equivalent as far as I can tell)

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Honestly, I'm not sure either.

There's also the problem that the sysctl(8) command doesn't show some things, even with the -o flag to show “opaque” values — the `pcblist` nodes (== `netstat -an`) do show up, but `net.route` shows only a harmless-looking boolean flag `net.route.verbose` even though that's the subtree that getifaddrs() uses.  So that's not a safe way to check what a subtree exposes.

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #10)
> (Also apparently |sysctl-name-prefix|, which is slightly prettier than
> -regex but functionally equivalent as far as I can tell)

That doesn't exist on 10.10, but -regex does (and works).

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

The lack of ability to reliably introspect these things to me is a compelling argument that we should at least _try_ the pure-whitelist, and only switch to prefixes once we learn that it's a terrible idea because everything breaks :-) (Most sysctl callsites in our code have defaults, so depending on your perspective that either means the failures will be silent, or they won't be super disruptive).

Comment 14

[Haik Aftandilian [:haik]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #13)
> The lack of ability to reliably introspect these things to me is a
> compelling argument that we should at least _try_ the pure-whitelist, and
> only switch to prefixes once we learn that it's a terrible idea because
> everything breaks :-) (Most sysctl callsites in our code have defaults, so
> depending on your perspective that either means the failures will be silent,
> or they won't be super disruptive).

I think it's worth trying the whitelist too.

One concern is that new OS versions might introduce low-level code that use a sysctl we block for CPU-specific optimizations. It could silently fallback to a slower path and we wouldn't know apart from messages in Console. But that's the nature of this work.

Comment 15

[Haik Aftandilian [:haik]]

Comment on attachment 8883622 [details] [diff] [review]
sysctl-lockdown.diff

Review of attachment 8883622 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with :jld and think this is a little risky, but well understood. We want the strongest sandbox that allows the browser to work so it's worth trying.

Comment 16

[Haik Aftandilian [:haik]]

Let's add a test for this in security/sandbox/test/browser_content_sandbox_syscalls.js.

Comment 17

[Alex Gaynor [:Alex_Gaynor]]

Attachment: sysctl-lockdown.diff

Comment 18

[Daniel Veditz [:dveditz]]

Alex: this appears to be work on improving our sandbox and not really a vulnerability per se. Does it need to remain hidden?

Comment 19

[Alex Gaynor [:Alex_Gaynor]]

I didn't originally think it did. :jld marked it as a security (maybe because he filed an equivilant Chromium bug as a security bug?). :jld, can you confirm?

Comment 20

[Haik Aftandilian [:haik]]

Comment on attachment 8883996 [details] [diff] [review]
sysctl-lockdown.diff

Why the TODO for net.inet?

Comment 21

[Alex Gaynor [:Alex_Gaynor]]

Doh, meant to delete that |TODO|, I already confirmed it was fine.

Comment 22

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Yes, I hid this bug because I filed https://crbug.com/738129 (linked less readably in this bug's See Also section), which they've opted to leave hidden even though they consider it a “minor privacy issue”.  I also mentioned on that bug that this bug was briefly public.

Comment 23

[Alex Gaynor [:Alex_Gaynor]]

Attachment: sysctl-lockdown.diff

Comment 24

[Haik Aftandilian [:haik]]

Sorry for not noting this earlier, but it just occurred to me that we need to make the test not apply to OS X 10.9. Low priority since we're not running 10.9 on treeherder right now. We might end up doing that later for ESR builds. I don't know the best way to do it, but Google led me to sw_vers which works on 10.9:

  $ sw_vers -productVersion
  10.9.5

Comment 25

[Alex Gaynor [:Alex_Gaynor]]

I think sysctlbyname("kern.osrelease") >= "14.0.0" would work (14.0.0 was the Darwin release corresponding to 10.10). I think we must have an existing internal API for this, so I'll look for that first before doing it the funny way :-)

Comment 26

[Alex Gaynor [:Alex_Gaynor]]

Attachment: sysctl-lockdown.diff

Ok, I think this resolves all the review feedback!

What's the process for landing a change like this?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/beec1017eefaec856040a49e49c6e477582f2a72

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The Chromium bug is still hidden, but this unit test landed and the comment pretty much lets the cat out of the bag (if in fact the cat was ever in the bag): https://chromium.googlesource.com/chromium/src.git/+/9e96523ae3dee27d2af4d48fbfcc12881aa8e721/content/renderer/sandbox_mac_v2_unittest.mm#154

Comment 30

[Alex Gaynor [:Alex_Gaynor]]

Good enough for me. Dan, can you unhidden this?