1083344 - Tighten rules for Mac OS content process sandbox on 10.9 and 10.10

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[André Reinald]

Follow up of bug 1076385 which only implemented the "plumbing" of the content sandbox.

Comment 1

[André Reinald]

Attachment: rules.sb

WIP: Mac sandbox rules.

Comment 2

[André Reinald]

Attachment: read-sandbox-rules-from-file.patch

A dirty hack to read rules from disk so there is no need to recompile FF, only to open a new browser window. Try-error-correction cycles are much shorter.

Comment 3

[André Reinald]

The syntax of rules is really a full language, in the likes of lisp and scheme, but I found no documentation.

The content process accesses far more resources than openh264, some of which are needed only in precise circumstances like:
- playing sound,
- playing video,
- using some fonts,
- using the "developer edition" of Firefox,
- likely many other circumstances I can't think of yet.

Resources used:
http://hints.macworld.com/article.php?story=20100318044558156

Rules files in:
/System/Library/Sandbox/Profiles/

Notably:
system.sb
bsd.sb
application.sb

Comment 4

[André Reinald]

Another very rich source of information is IronFox.app
Among others, they have different rulesets for all versions of Mac OS X they support.
But their scope encompasses Firefox as a whole process, it's a "before e10s" project.

https://www.romab.com/ironfox/

Comment 5

[André Reinald]

Attachment: rules.sb

Updated set of rules.

Comment 6

[André Reinald]

I see in bug 1083284 that on 10.6 the "iokit-open" rule is commented out because of "bad syntax", and is not replaced by any other "iokit" related rule. I guess this implies that "iokit" access is not sandboxed at all on 10.6, and that I should also comment out all "iokit-open" rules for the content process on that system version.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

I don't know.  You need to test on OS X 10.6.  For the time being, I don't have time to do it myself.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

I should have been more precise:

Yes, you probably should comment out rules with exactly the same syntax.  But who knows whether or not your rules need replacements of some kind.  Mine didn't ... but I only found that out through testing.  You *really* need to be able to test on OS X 10.6.

Comment 9

[André Reinald]

Steven, when you have a few spare mn, could you please send me zips of the /System/Library/Sandbox folder for all OS X versions you have (and that we should support)? It would help me figure out differences in .sb files syntax. Thanks.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Attachment: /System/Library/Sandbox on OS X 10.6.8

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Attachment: /System/Library/Sandbox on OS X 10.7.5

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Attachment: /System/Library/Sandbox on OS X 10.8.5

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: /System/Library/Sandbox on OS X 10.9.5

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Attachment: /System/Library/Sandbox on OS X 10.10.1

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Since they're small, I just posted them here.  Note that the number of profiles is much smaller on OS X 10.6.8 than on other versions of OS X.

Comment 16

[André Reinald]

Attachment: rules.sb

This is the rules file, which I escaped to include into the source. This will be easier to check and modify. Using it directly requires patch 8524802 applied.

Comment 17

[André Reinald]

Attachment: bugzilla-1083344-2.patch

Tests passed: try submission c4b59e2d6f3c
This patch is only effective on 10.9 and up covering most users.
Previous systems will be supported in upcoming patches (will open a follow-up).

Comment 18

[André Reinald]

I just discovered that /private/var/folders/XX changes XX from system to system, so rules should be more flexible. Will update this tomorrow.

Comment 19

[André Reinald]

Attachment: bugzilla-1083344-3.patch

Adjusted rules according to comment 18.

Comment 20

[André Reinald]

Attachment: rules.sb

This is the rules file, which I escaped to include into the source. This will be easier to check and modify. Using it directly requires patch 8524802 applied. Adjusted according to comment 18.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8548319 [details] [diff] [review]
bugzilla-1083344-3.patch

Which versions of OS X have you tested this on?  What kinds of tests have you run?  (Please give a few examples.)

Some of the syntax is *very* weird, at least to me.  For example:

(if (< macosMinorVersion 9) ...

Did you find any documentation for the syntax?  Does it resemble one or more computer languages, which might themselves have decent documentation?

Comment 22

[André Reinald]

I mentioned in comment 3 that the language is similar to lisp or scheme. Examples are in system sb profiles. I tested on 10.9. On 10.10 syntax is fine, but I have to test further. For older systems it's allow all. Tests passed on try which is 10.8 I believe. I tried browsing, audio, video and camera and microphone.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

> I tried browsing, audio, video and camera and microphone.

Please provide a few specific examples of each.  I'll test them on OS X 10.9 and 10.10.

I'll also check that you don't get syntax errors on 10.6 and up (passing mochitests is probably not a good enough testcase).

So I may not be able to finish my review until next week.

Comment 24

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

Move process sandboxing bugs to their new, separate component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)

Comment 25

[André Reinald]

Attachment: bugzilla-1083344-4.patch

Added rule for 10.10 video playback.

Comment 26

[André Reinald]

Attachment: rules.sb

Added rules for 10.10 video playback.

Comment 27

[André Reinald]

During my tests, accessing camera and mic was not supported at all, but that is a shortcoming of e10s. As far as I understand, bug 1104615 is about accessing those input devices from the main process, so there should be no incidence on this patch.

Comment 28

[André Reinald]

Follow up opened as bug 1123291 for previous system versions.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

> During my tests, accessing camera and mic was not supported at all

Actually they *are* supported, after a fashion, even in e10s mode:  The testcase from bug 1012949 (http://mozilla.github.io/webrtc-landing/pc_test_h264.html) *does* successfully access your webcam, even in e10s mode -- you see the little green camera light, and what it sees gets displayed in the right-hand pane.  It's just that the testcase doesn't work fully -- you don't also see webcam output in the left-hand pane.

This is with your allow-all content process ruleset, of course.

Comment 30

[André Reinald]

My tests were with talky.io which uses webrtc too, and no denied access showed in the system log. If you figure out why we have different results I'll be curious to know. Anyway camera access from the content process sounds quite scary.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

> talky.io

Is there a simple webcam test on that site?  One that doesn't require you to actually chat with someone? :-)

Comment 32

[Steven Michaud [:smichaud] (Retired)]

I've been making progress on this, but only very slowly.

The first problem I had to solve is find decent references to the Polish notation (aka prefix notation) used by Apple's sandbox rulesets (their *.sb files).  I did find http://en.wikipedia.org/wiki/Polish_notation, but it's nowhere near complete.  So I fell back to looking for Lisp-language references, and found what seems to be a pretty good one:

http://www.lispworks.com/documentation/HyperSpec/Front/index.htm

Particularly valuable is its list of symbols:
http://www.lispworks.com/documentation/HyperSpec/Front/X_AllSym.htm

Comment 33

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8549709 [details] [diff] [review]
bugzilla-1083344-4.patch

Using the above mentioned reference, and by trial and error, I found that the following line is badly wrong:

+  "(if (< macosMinorVersion 9)\n"

It should instead be as follows, which more or less reverses the logic.  (I'll have more to say about this in a subsequent comment.)

+  "(if (>= macosMinorVersion 9)\n"

Even after fixing this there are still problems.  I'll try to address them over the next few days, and suggest an alternative ruleset.  But even the parts of the ruleset that "work" (after the above change) contain some very strange and scary items, for example the following:

+  "    (allow file-read*\n"
+  "        (subpath \"/Applications/TextWrangler.app\")\n"
+  "        (subpath \"/Applications/Xcode.app/Contents/Developer/Applications/iOS Simulator.app\")\n"

Many of these rules (including the ones mentioned above) "just work", but we have no idea why.  And it would take a lot of time (and possibly serious reverse engineering of the OS) to find out why.

Furthermore, the ruleset as a whole has had very little testing (some by André and so far very little by me, though I hope to add more over the next few days).  And because we're largely flying blind (as I mentioned above), I think it's likely that André and I have missed lots of rules, without which all kinds of basic functionality will break.

So I may decide, over the next few days, that we shouldn't land any of this patch now.  It may be better to wait until we have sufficient time to investigate, and particularly until we've had a chance to identify functionality that should be moved from the content process to the chrome process (as per bug 1124817).

Note that I haven't yet made up my mind.  I need to do several more days of testing and playing around.  But I want people to be forewarned how this bug may turn out.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #33)

+  "(if (< macosMinorVersion 9)\n"

As I mentioned above, this is wrong and its logic needs to be reversed, more or less.  Since Lisp syntax (and Polish logic) is new to me, and perhaps also to André, I'll make a detailed argument for this from parts of the Lisp reference that I mentioned above.

Here's what it has to say about the "special operator IF":

http://www.lispworks.com/documentation/HyperSpec/Body/s_if.htm#if

The general syntax rule for it is as follows:

if test-form then-form [else-form] => result*

André's if statement contains test-form and then-form elements, but no else-form element.  Using the same language as this general rule, it looks like this:

if ((then-form)
  else-form
)

Or to substitute (part of) its actual contents:

if ((< macosMinorVersion 9)
  begin(
    ...
  )
)

The "test-form" part of André's if statement uses an arithmetic comparison operator ("<").  The syntax of all such operators is defined here:

http://www.lispworks.com/documentation/HyperSpec/Body/f_eq_sle.htm

I think it's pretty clear from this that "(< macosMinorVersion 9)" should be translated into C/C++ syntax as "(macosMinorVersion < 9)".

But we want these rules to apply on OS X 10.9 and above, so the following is clearly more appropriate:

if ((>= macosMinorVersion 9)
  begin(
    ...
  )
)

Comment 35

[Steven Michaud [:smichaud] (Retired)]

I tested what I say in comment #34 with the following very simple rulesets, on OS X 10.9.  A simple "(deny default)" rule denies *all* access, and causes the content process to crash.  A simple "(allow default)" rule works like current code, which allows all access -- so the content process doesn't crash.

  "(version 1)\n"
  "\n"
  "(define macosMajorVersion %d)\n"
  "(define macosVersionMinor %d)\n"
  "(define appPath \"%s\")\n"
  "(define appBinaryPath \"%s\")\n"
  "(define home-path \"%s\")\n"
  "\n"
  "(if (< macosVersionMinor 9) (allow default) (deny default))\n";

With this ruleset, the content process crashes on OS X 10.9.

  "(version 1)\n"
  "\n"
  "(define macosMajorVersion %d)\n"
  "(define macosVersionMinor %d)\n"
  "(define appPath \"%s\")\n"
  "(define appBinaryPath \"%s\")\n"
  "(define home-path \"%s\")\n"
  "\n"
  "(if (>= macosVersionMinor 9) (allow default) (deny default))\n";

With this ruleset the content process runs fine.

Comment 36

[Steven Michaud [:smichaud] (Retired)]

By the way, André tells me he (at least mostly) tested *without* the incorrect if statement, on OS X 10.9.  So it's incorrectness doesn't invalidate his tests.

Comment 37

[Steven Michaud [:smichaud] (Retired)]

(Correcting comment #34)

> if ((then-form)
>   else-form
> )

This should of course be as follows:

if ((test-form)
  then-form
)

Comment 38

[Steven Michaud [:smichaud] (Retired)]

I'll mention one more thing in passing, since it cost me several hours to find out:

(import "[path]") statements in Apple sandbox rulesets must only be used at the top (global) level -- not for example in if statements.  If you do use an "import" statement incorrectly, Apple's parser stops reading the ruleset at that point ... though it *doesn't* report a syntax error.

Comment 39

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

(In reply to Steven Michaud from comment #32)
> So I fell back to looking for Lisp-language
> references, and found what seems to be a pretty good one:
> 
> http://www.lispworks.com/documentation/HyperSpec/Front/index.htm
> 
> Particularly valuable is its list of symbols:
> http://www.lispworks.com/documentation/HyperSpec/Front/X_AllSym.htm

I don't know if it makes any difference here, but Scheme standards might also be of interest:

[R5RS]: http://www.schemers.org/Documents/Standards/R5RS/HTML/
[R6RS]: http://www.r6rs.org/final/html/r6rs/r6rs-Z-H-2.html

If nothing else, R5RS is a much smaller document than the Common Lisp spec.  As for R6RS, I suspect that none of its additions are relevant to the Mac sandbox language, but it had more of a focus on precise specification than its predecessors, which could potentially be useful.)

Comment 40

[André Reinald]

(In reply to Steven Michaud from comment #33)
> Comment on attachment 8549709 [details] [diff] [review]
> bugzilla-1083344-4.patch
> 
> Using the above mentioned reference, and by trial and error, I found that
> the following line is badly wrong:
> 
> +  "(if (< macosMinorVersion 9)\n"
> 
> It should instead be as follows, which more or less reverses the logic. 
> (I'll have more to say about this in a subsequent comment.)
> 
> +  "(if (>= macosMinorVersion 9)\n"

I keep it that it's the right syntax and that you've even proved it in your own tests.

> Even after fixing this there are still problems.  I'll try to address them
> over the next few days, and suggest an alternative ruleset.  But even the
> parts of the ruleset that "work" (after the above change) contain some very
> strange and scary items, for example the following:
> 
> +  "    (allow file-read*\n"
> +  "        (subpath \"/Applications/TextWrangler.app\")\n"
> +  "        (subpath
> \"/Applications/Xcode.app/Contents/Developer/Applications/iOS
> Simulator.app\")\n"
> 
> Many of these rules (including the ones mentioned above) "just work", but we
> have no idea why.  And it would take a lot of time (and possibly serious
> reverse engineering of the OS) to find out why.

That's right, but may aim was to make it work. I kind of suspect why those are needed, but being able to work without them would require avoiding their calling in the first place, and the sandboxing work did not include this work.

I opened bug 1124817 specifically to address those issues in upcoming bugs, and those very 2 rules were the first ones I was to to add.

> Furthermore, the ruleset as a whole has had very little testing (some by
> André and so far very little by me, though I hope to add more over the next
> few days).  And because we're largely flying blind (as I mentioned above), I
> think it's likely that André and I have missed lots of rules, without which
> all kinds of basic functionality will break.

I agree that we had to fly blind. Reverse engineering our own code to list all possible resources needed (including those accessed by library calls that we depend on) is much beyond the time assigned to this bug, So this was the only way out. And from my understanding of sandboxing work on other platforms this is what is done too: testing and adding rules as needed.

(In reply to Steven Michaud from comment #34)
> (Following up comment #33)
> The general syntax rule for it is as follows:
> 
> if test-form then-form [else-form] => result*
> 
> André's if statement contains test-form and then-form elements, but no
> else-form element.

This is NOT true. My IF statement contains both THEN and ELSE elements. The first is executed when (macosMinorVersion < 9) which lisp wise is written (< macosMinorVersion 9) and is (allow default). We want an allow all rule for macos prior to 10.9.

The second is executed otherwise (ie if (macosMinorVersion >= 9)) and is the "hard" part (begin .....)

(if (< macosMinorVersion 9) ;; condition
  (allow default) ;; THEN
  (begin ;; ELSE
    ...))

And this is exactly what is needed.

> I think it's pretty clear from this that "(< macosMinorVersion 9)" should be
> translated into C/C++ syntax as "(macosMinorVersion < 9)".

That's right as I stated above. Steven just missed that the IF construct in had both a THEN and an ELSE parts.

> But we want these rules to apply on OS X 10.9 and above, so the following is
> clearly more appropriate:
> 
> if ((>= macosMinorVersion 9)
>   begin(
>     ...
>   )
> )

I treat the version < 9 case in the THEN and the >= case in the ELSE.

(In reply to Steven Michaud from comment #35)
> I tested what I say in comment #34 with the following very simple rulesets,
> on OS X 10.9.  A simple "(deny default)" rule denies *all* access, and
> causes the content process to crash.  A simple "(allow default)" rule works
> like current code, which allows all access -- so the content process doesn't
> crash.
> 
>   "(version 1)\n"
>   "\n"
>   "(define macosMajorVersion %d)\n"
>   "(define macosVersionMinor %d)\n"
>   "(define appPath \"%s\")\n"
>   "(define appBinaryPath \"%s\")\n"
>   "(define home-path \"%s\")\n"
>   "\n"
>   "(if (< macosVersionMinor 9) (allow default) (deny default))\n";
> 
> With this ruleset, the content process crashes on OS X 10.9.

Yes, in your tests, macosVersionMinor is 9, so (< macosVersionMinor 9) is false, so the ELSE statement is executed (deny default) and the process crashes.

>   "(version 1)\n"
>   "\n"
>   "(define macosMajorVersion %d)\n"
>   "(define macosVersionMinor %d)\n"
>   "(define appPath \"%s\")\n"
>   "(define appBinaryPath \"%s\")\n"
>   "(define home-path \"%s\")\n"
>   "\n"
>   "(if (>= macosVersionMinor 9) (allow default) (deny default))\n";
> 
> With this ruleset the content process runs fine.

Yes again, in your tests, macosVersionMinor is 9, so (>= macosVersionMinor 9) is true, so the THEN statement is executed (allow default) and the process runs fine.

Comment 41

[André Reinald]

Attachment: rules.sb

Updated rules for accessing the camera.

Comment 42

[André Reinald]

Attachment: bugzilla-1083344-5.patch

Comment 43

[André Reinald]

Comment on attachment 8555216 [details] [diff] [review]
bugzilla-1083344-5.patch

It seems that a patch landed during the past weeks which now allows camera and microphone access in e10s.

This patch adds a few sandbox rules to allow the camera. Microphone access still doesn't work, but there are no relevant traces in the log to determine what is blocked (and needs to be allowed).

Comment 44

[André Reinald]

With sandbox enabled, those likes are written to /var/log/system.log at launch:
Jan 27 14:34:49 lepiote.local sandboxd[118] ([63603]): plugin-container(63603) deny mach-lookup com.apple.coreservices.appleevents

Then when opening apprtc.webrtc.org and going to a room (see the time stamps):
Jan 27 14:35:18 lepiote.local sandboxd[118] ([63603]): plugin-container(63603) deny file-read-data /Users/andre/Programmes/mozilla-central
Jan 27 14:35:18 lepiote.local sandboxd[118] ([63603]): plugin-container(63603) deny file-read-metadata /Users/andre/Programmes/mozilla-central

The above directory is the current directory from which Firefox is launched with the following command:
/Users/andre/Desktop/Nightly-sb.app/Contents/MacOS/firefox --no-remote -p debuguser

Comment 45

[André Reinald]

With just an (allow default) rule, this (very long) line is written to /var/log/system.log at launch:
Jan 27 14:41:31 lepiote.local appleeventsd[92]: <rdar://problem/11489077> A sandboxed application with pid 63619, "Nightly Web Content" checked in with appleeventsd, but its code signature could not be validated ( either because it was corrupt, or could not be read by appleeventsd ) and so it cannot receive AppleEvents targeted by name, bundle id, or signature. Error=ERROR: #100013  { "NSDescription"="SecCodeCopyGuestWithAttributes() returned 100013, -." }  (handleMessage()/appleEventsD.cp #2072) client-reqs-q

With the (allow default) rule, both camera and microphone are "shared", whereas with the current rules, only the camera is "shared".

Comment 46

[André Reinald]

(In reply to Steven Michaud from comment #29)
> > During my tests, accessing camera and mic was not supported at all
> 
> Actually they *are* supported, after a fashion, even in e10s mode:  The
> testcase from bug 1012949
> (http://mozilla.github.io/webrtc-landing/pc_test_h264.html) *does*
> successfully access your webcam, even in e10s mode -- you see the little
> green camera light, and what it sees gets displayed in the right-hand pane. 
> It's just that the testcase doesn't work fully -- you don't also see webcam
> output in the left-hand pane.
> 
> This is with your allow-all content process ruleset, of course.

With my rules applied, the camera is accessed, but the microphone is not.
With (allow default), both camera and microphone are accessed.

Comment 47

[André Reinald]

Comment above is about a build based on today's mozilla-central, with and without my patch applied.

Comment 48

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #40)

> (if (< macosMinorVersion 9) ;; condition
>   (allow default) ;; THEN
>   (begin ;; ELSE
>    ...))

You're right, André -- this is correct.

The following *is* wrong, but isn't in your patch.  I was working from a copy of your patch that I'd modified, forgetting that one of the modifications was to move the "(allow default)" line elsewhere.

> if ((>= macosMinorVersion 9)
>   begin(
>     ...
>   )
> )

Sorry for the confusion :-(

I'll continue testing with your latest patch.

Please do post more comments about the testing you've done.  Please include URLs.

Comment 49

[Steven Michaud [:smichaud] (Retired)]

Another thing, André:  Both of us need to test as a non-admin ("standard") user, to see if this makes any difference, and if so how much.

Comment 50

[André Reinald]

(In reply to Steven Michaud from comment #49)
> Another thing, André:  Both of us need to test as a non-admin ("standard")
> user, to see if this makes any difference, and if so how much.

Using the Mac as an admin opens a few security holes, so it's generally good practice to use a non-admin account for everyday tasks. At least this is what I do.

So *all* my tests are done with non-admin rights.

Comment 51

[André Reinald]

Attachment: bugzilla-1083344-6.patch

This new set of rules addresses the concern expressed in comment 33, about the 2 rules containing paths to specific applications I had installed (TextWrangler and the iOS emulator).

Those 2 paths were obviously found using LaunchServices, so denying access to LaunchServices in the first place (removing the (allow mach-lookup "com.apple.coreservices.launchservicesd") rule) resulted in those paths not being accessed any more, and the corresponding 2 objectionable rules could be removed.

The only regression I'm aware of about this patch is lack of access to the microphone which I'm still investigating. But as I am aware of work being done to proxy access to peripherals, I'm not sure what policy we should adopt here: putting the patch on hold, or land it and put more fire on the peripheral access proxying.

We should keep in mind that as more and more stuff will be proxyied to other processes, we'll be able to remove (allow) rules from this patch, which will result in a tighter sandbox.

Comment 52

[André Reinald]

Attachment: rules.sb

Updated to match bugzilla-1083344-6.patch

Comment 53

[Steven Michaud [:smichaud] (Retired)]

Attachment: Additions to André's latest patch

I've continued testing, but it will be at least a couple more days before I'm finished.

Along the way I discovered a couple of additions to André's '6' patch, which I'm posting here.  They're meant to be applied on top of that patch on current trunk.

1) Added support for granting read access to the "appdir" and its contents.  Without this you get some serious failures of functionality.  For example clicking on the "home" symbol doesn't take you to your home page -- but instead loads an about:newtab.

The "appdir" is the same in the parent (chrome) process and the child (the content process).  It's normally the Contents/Resources/browser subdirectory of the parent.

2) Added a rule to allow reading of *all* file metadata.

I got this from the bsd.sb profile, where it has the following annotation:

;; allow processes to traverse symlinks

Without it I see lots of the following errors in Terminal (when I run Firefox from there):

_CFURLGetResourcePropertyFlags failed because it was passed this URL which has no scheme: ...

Comment 54

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #33)

After talking with Benjamin Smedberg on Tuesday, I'm no longer concerned that our initial content process sandbox rules are somewhat insecure.  As I understand it from him, we'll acknowledge that our sandbox, as we first land it, isn't really secure.  Once e10s itself is stable, we'll gradually tighten it, probably largely by moving code that needs "sensitive" access from the content process to the chrome process (as per bug 1124817).  This process will be easier if we have something concrete to start from -- a specific set of rules, rather than just the empty ruleset we currently have.

I *am* still concerned about showstoppers -- about breaking major pieces of functionality (like the home button mentioned in comment #53).  But I'm not worried about error messages showing up in the system log (viewable using the Console app), unless they indicate serious breakage.

Comment 55

[Steven Michaud [:smichaud] (Retired)]

Another potential showstopper:

Double-clicking on an *.html file while a test build is running results in a blank page getting loaded in the test build.  This doesn't happen without André's patch.

This *may* have something to do with Apple events (since the OS does use Apple events to implement this functionality).  But I don't think the relationship is a direct one.

1) Other Apple event functionality seems to work fine.  For example clicking on a URL in Thunderbird does fully open it in the test build.  And when the test build is in the background, clicking on its Dock icon does bring it to the foreground.  Both of these also use Apple events.

2) The sandbox error message in the Console is "deny file-read-data [filepath]".

I'll investigate further.

Comment 56

[Steven Michaud [:smichaud] (Retired)]

More weirdness, probably not a showstopper:

Opening a new tab and visiting an ftp:// link in it triggers errors like the following:

1/29/15 3:34:07.226 PM sandboxd[98]: ([16201]) plugin-container(16201) deny file-read-data /private/var/folders/1p/6dzwvfx10ss0nyny7q973w240000gn/C/com.apple.IconServices/ISCacheTOC

1/29/15 3:34:07.259 PM sandboxd[98]: ([16201]) plugin-container(16201) deny file-read-data /private/var/folders/1p/6dzwvfx10ss0nyny7q973w240000gn/C/com.apple.IconServices/2C8721FE29F62D80F6A51E2321A27B07.iscachebmp

It might be possible to get rid of these by altering (loosening up) the following rule:

  "    (allow file*\n"
  "        (var-folders2-regex \"/com\\.apple\\.IntlDataCache\\.le$\"))\n"

Probably the reason for these errors is that ftp:// sites don't have icons, and so must use a default icon, for which the OS needs to search.  The errors don't prevent the default icon (a computer symbol) from being used correctly.

Comment 57

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #55)

Interestingly, Save Page As works just fine.  I'd bet the difference is that Save Page As brokers writing the file to the Chrome process.

Comment 58

[Steven Michaud [:smichaud] (Retired)]

Another thing we need to test is whether updates work properly or not.

Robert and Stephen, do you have any suggestions about how to test this using self builds?

I remember that there are some automated tests of updates, but I don't remember where (which ones) they are.  Can either of you remind me? :-)

Comment 59

[Robert Strong (they/them - no direct email)]

The tests are located at
toolkit/mozapps/update/tests/

You can most easily test landing on any of the project branches that provides updates.

Comment 60

[Steven Michaud [:smichaud] (Retired)]

Thanks Robert.  Here's another question:

Do you know if having the Chrome and Content processes running makes any difference to what happens during an update?  If the answer is no, then we probably don't have to test.  Though you've suggested a reasonably easy way to do so.

Comment 61

[Robert Strong (they/them - no direct email)]

My understanding is that "having the Chrome and Content processes running" doesn't make a difference. Having said that I don't know enough about the rules or the affects of the sandbox to say it won't have an adverse affect and the safest path would be to land and test on a project branch if there is a chance it will break updates.

Comment 62

[Steven Michaud [:smichaud] (Retired)]

Another showstopper, worse than the one in comment #55:

Printing is completely broken.

Printers that you've created in the Printers and Scanners system pref panel don't show up in the Print dialog.  All you see is "No Printer Selected".  And choosing "Add Printer" has no effect.

None of the following work:  "Open PDF in Preview", "Save as PDF", "Save as PostScript".

All of these work fine in today's mozilla-central nightly.

Comment 63

[Steven Michaud [:smichaud] (Retired)]

I'm now inclined to land a patch for this bug only as a non-default option, as we discussed at today's sandboxing meeting.  The default option would be what we already have in current code -- a sandbox with an "empty" ruleset (one that allows all access).

I suspect printing will also need to be brokered through the chrome process.

Comment 64

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

(In reply to Steven Michaud from comment #57)
> Interestingly, Save Page As works just fine.  I'd bet the difference is that
> Save Page As brokers writing the file to the Chrome process.

More or less; the UI lives in the parent process and asks the content process about the state of the document.  It also doesn't entirely work yet.  See bug 1058251 and bug 1101100.

(In reply to Steven Michaud from comment #63)
> I suspect printing will also need to be brokered through the chrome process.

Bug 927188 looks as if it might be what's needed here.

Comment 65

[André Reinald]

I've put together a patch, including Steven's additions, and also found how to enable printing, camera and microphone access from the sandbox.

I'll also test the double-clicking of a file in the finder to see what happens in the log and add rules accordingly, it should be easier than the microphone access which gave me no log traces, and this should hopefully remove all show-stoppers.

I hope to get something later in the day or tomorrow.

Comment 66

[André Reinald]

Attachment: rules.sb

Adding mic and printing. Still wip.

Comment 67

[André Reinald]

Attachment: bugzilla-1083344-7.patch

Adding mic and printing. Still wip. Not tested yet, as we have a meeting. But feedback is welcome (though not flagged review?).

Comment 68

[Steven Michaud [:smichaud] (Retired)]

André, I've found a better way to get the appdir.  I'll post it shortly as an addition to your v7 patch.

The change won't effect testing.  But without it we'll have some leaks.

Comment 69

[André Reinald]

Attachment: bugzilla-1083344-8.patch

Forget v7 patch, I had a meeting at 18 and was in a hurry. I applied MY changes in the rules.sb file, and YOUR changes to the patch itself. Now I have ALL changes in both places.

Comment 70

[André Reinald]

Attachment: rules.sb

rules matching patch v8.

Comment 71

[Steven Michaud [:smichaud] (Retired)]

Attachment: Additions to André's v8 patch

Here's the patch I promised in comment #68, which fixes how we get the appdir.

Comment 72

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8557163 [details] [diff] [review]
bugzilla-1083344-8.patch

+  "    (shared-preferences-read \"org.cups.PrintingPrefs\")\n"

Including this in the ruleset makes sandbox_init() return an error, which in turn crashes the content process.  I suppose the syntax is wrong.

Commenting it out stops the crashes.

Comment 73

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #72)

Turns out that the shared-preferences-read procedure is defined in application.sb, which we don't (and apparently can't) import.  So to make it work we need to add a definition for it to the content sandbox ruleset.

I've adapted the following from application.sb:

  "    (define (shared-preferences-read . domains)\n"
  "      (for-each\n"
  "        (lambda (domain)\n"
  "          (begin\n"
  "            (if (defined? `user-preference-read)\n"
  "              (allow user-preference-read (preference-domain domain)))\n"
  "            (allow file-read*\n"
  "                   (home-literal\n"
  "                     (string-append \"/Library/Preferences/\" domain \".plist\"))\n"
  "                   (home-regex\n"
  "                     (string-append\n"
  "                       \"/Library/Preferences/ByHost/\"\n"
  "                       (regex-quote domain)\n"
  "                       \"\\..*\\.plist$\")))))\n"
  "      domains))\n"

Adding it stops the crashes.

Comment 74

[Steven Michaud [:smichaud] (Retired)]

But printing is still completely broken.  I still have exactly the same symptoms as I reported in comment #62.  It may make a difference that I have a networked printer.

Comment 75

[Steven Michaud [:smichaud] (Retired)]

I do now have access to the microphone (in addition to the webcam).  But that's the only thing that's been fixed.

Printing is still completely broken (as mentioned in comment #74).  And I'm still seeing the problems I mentioned in comment #55 and comment #56.

Comment 76

[André Reinald]

I changed the rules but still have those limitations:

1. Loading a file from disk doesn't work neither from the Finder nor from Firefox itself (File/Open, or typing the file:// URL) and I always get the same message:

  deny file-read-data |path-to-the-file-to-open|

I removed the appleEvents rule, there was no difference, neither in behavior nor in the logs. So blocking AppleEvents was not the cause.

This is a clear indication that the content process tries to read the file from disk, which should not happen. Allowing the content process to read any file from disk would completely void the purpose of the sandbox.


2. Printing to PDF is broken for the same reason as 1: the content process tries to write a file to disk and the log has:

  deny file-write-create |path-to-the-pdf-file-to-save|

Allowing the content process to write any file to disk would completely void the purpose of the sandbox.

Comment 77

[André Reinald]

Things that don't work in e10s and that are UNRELATED to sandboxing:

3. Saving a file doesn't work, the progress is in "endless state". Surprisingly, when quitting and relaunching Firefox the files ARE saved!

4. Printing misses some images in the output. Tried to print the nightly startup page, or a random news page. Only small images were printed for an unknown reason.

I guess I'll have to create bugs about 3 and 4 if they have not yet been reported.

Comment 78

[André Reinald]

Attachment: rules.sb

Patch to come soon, but rules corresponding to the 2 above comments are here.

Comment 79

[André Reinald]

(In reply to André Reinald from comment #78)
> Created attachment 8557518 [details]
> rules.sb

Line 12 should of course be (if (< macosMinorVersion 9)

But to distinguish if e10s or sandboxing is involved in a misbehavior (comment 77), switching between 9 and 19 allowed to quickly enable/disable the sandbox.

Comment 80

[André Reinald]

(In reply to Steven Michaud from comment #57)
> (Following up comment #55)
> 
> Interestingly, Save Page As works just fine.  I'd bet the difference is that
> Save Page As brokers writing the file to the Chrome process.

My own tests shows that with both (allow default) and current rules, Save Page As results in "undertermined" progress bar in downloads lasting forever. So this is dependent on e10s.

Page is really saved when I quit and relaunch Firefox, which is odd.

Comment 81

[André Reinald]

(In reply to Steven Michaud from comment #63)
> I'm now inclined to land a patch for this bug only as a non-default option,
> as we discussed at today's sandboxing meeting.  The default option would be
> what we already have in current code -- a sandbox with an "empty" ruleset
> (one that allows all access).
> 
> I suspect printing will also need to be brokered through the chrome process.

Rules to allow printing AND print preview (preview requires a lot more rules) have been added separately so they will be easy to remove when the functionality is moved in the chrome process.

Comment 82

[André Reinald]

(In reply to Steven Michaud from comment #56)
> More weirdness, probably not a showstopper:
> 
> Opening a new tab and visiting an ftp:// link in it triggers errors like the
> following:
> 
> 1/29/15 3:34:07.226 PM sandboxd[98]: ([16201]) plugin-container(16201) deny
> file-read-data
> /private/var/folders/1p/6dzwvfx10ss0nyny7q973w240000gn/C/com.apple.
> IconServices/ISCacheTOC
> 
> 1/29/15 3:34:07.259 PM sandboxd[98]: ([16201]) plugin-container(16201) deny
> file-read-data
> /private/var/folders/1p/6dzwvfx10ss0nyny7q973w240000gn/C/com.apple.
> IconServices/2C8721FE29F62D80F6A51E2321A27B07.iscachebmp
> 
> It might be possible to get rid of these by altering (loosening up) the
> following rule:
> 
>   "    (allow file*\n"
>   "        (var-folders2-regex \"/com\\.apple\\.IntlDataCache\\.le$\"))\n"
> 
> Probably the reason for these errors is that ftp:// sites don't have icons,
> and so must use a default icon, for which the OS needs to search.  The
> errors don't prevent the default icon (a computer symbol) from being used
> correctly.

Fixed in an additional rule, as we only need read access to Icon files:
    (allow file-read* (var-folders2-regex "/com\.apple\.IconServices/"))

Comment 83

[André Reinald]

(In reply to Steven Michaud from comment #55)
> Another potential showstopper:
> 
> Double-clicking on an *.html file while a test build is running results in a
> blank page getting loaded in the test build.  This doesn't happen without
> André's patch.
> 
> This *may* have something to do with Apple events (since the OS does use
> Apple events to implement this functionality).  But I don't think the
> relationship is a direct one.

I don't think so, because:
• I tried allowing and denying AppleEvents from the content process (I don't think we should allow AppleEvents in a final e10s implementation anyway) and the misbehavior still shows up.
• As I detail further, the issue is related to network vs file access, not to AppleEvents.

> 1) Other Apple event functionality seems to work fine.  For example clicking
> on a URL in Thunderbird does fully open it in the test build.  And when the
> test build is in the background, clicking on its Dock icon does bring it to
> the foreground.  Both of these also use Apple events.

If the link in TB refers to http:// or ftp:// (which I more plausible since sending file:// links through email makes no sense) accessing them is brokered through the main process, the sandbox is not involved in this case.

> 2) The sandbox error message in the Console is "deny file-read-data
> [filepath]".

This indicates that the content process tries to access directly the file system instead of brokering it through the main process. This is BAD!

Comment 84

[André Reinald]

Checked ftp:// addresses, they work fine, and surprisingly downloading a file actually works too (which is not the case with http:// pages or images).

The "Save Link As" doesn't work, with or without sandbox.

Tested on:
  ftp://ftp.mozilla.org/pub/chimera/releases/

Comment 85

[André Reinald]

Attachment: rules.sb

Comment 86

[André Reinald]

Attachment: bugzilla-1083344-9.patch

Taking into account additions from Steven regarding appDir retrieval.

Loading a file from disk and saving PDFs are still blocked by the sandbox because allowing file read and write in user specified locations would completely void the purpose of sandbox.

If the 2 above features are considered show-stoppers, then we should delay landing this patch until e10s takes them out of the content process.

Fixed access to default icon files.
Fixed sandbox for allowing printing, and printing preview.

With e10s but independent of the sandbox:
- printing and printing preview often produce strange results (missing images or css),
- saving http:// pages results in infinite progress bars (but they are saved on browser restart), ftp:// works,
- right-clicking and "Save Link As" command brings no file open dialog.

Comment 87

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8557929 [details] [diff] [review]
bugzilla-1083344-9.patch

This is getting closer, but there's still some unexplained weirdness:

> - right-clicking and "Save Link As" command brings no file open dialog.

Actually this isn't true for current m-c nightlies (with e10s on), though you do have to move the mouse before the native filepicker appears.  That's definitely a bug, but not the same bug as effects current trunk code with this patch -- there the filepicker never appears, even after moving the mouse.

(In reply to comment #80, wrt Save Page As)

I see this behavior too, and in today's m-c nightly.  But it's a recent regression -- I don't see it in in the 2015-01-29 m-c nightly.  And like I said I didn't see it earlier with your patch.

Since this new bug seriously messes up our testing, we'll need to test with trunk code that doesn't have it.  Later I'll check if a bug has been opened on it, and if not I'll open one myself.

Printing

I, too, find that printing and print preview work with your current patch on current trunk code.  That's a significant advance.  Thanks!

Save as PDF and PostScript still don't work.  But to allow this to work properly in the content process as it now stands (in a content process that doesn't broker all file access through the chrome process), we'd basically need to let the content process write files anywhere the current user's account can write.  And (as you say) this would entirely defeat the purpose of having a sandbox.

File : Open broken, also double-clicking on local files

I see this too, and agree with your assessment that it has nothing to do with Apple events.  And just as for Save As PDF, it's probably best to fix this by fixing bug 1124817.

Sandbox level setting

I don't think we can break Save As PDF and File : Open, even on m-c (at least with its default settings).  So I don't think the current sandbox rules can become the default until 1124817 is fixed.  But I hope we can land them (with perhaps a few more changes) on trunk behind a user-settable Gecko preference -- like the sandbox-level setting we've been discussing at the sandbox meetings.

Further testing

So far I've basically only tested on OS X 10.9.5.  I'll also need to test on 10.10.2, and on versions earlier than 10.9.  I hope to post my results later today.

So far all my tests have been with an admin account.  I understand that all yours, André, have been with a standard account.  But I suspect most of our users are running on their "own" machines and using admin accounts.  So I'm going to continue to concentrate on testing with admin accounts (though I hope to have time to also test with standard accounts).

Testing of updates

I suspect it'll be best to wait to test this until some version of André's patch has landed on the trunk.  Though it won't be on by default, we can test its effects on updates by turning it on and then trying an update.

Comment 88

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8557929 [details] [diff] [review]
bugzilla-1083344-9.patch

+  "        (appdir-regex \"(/XUL)|(\\.(js|jsm|css|xml|properties|ent|dtd|png|svg|gif|dylib))$\")\n"

Why don't you just allow read access to all of the appdir (as my own patch did)?  Remember that the appdir is passed (in a commandline parameter) from the chrome process to the content process, which I think means that the content process is meant to have read access to everything in it.

Comment 89

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #87)

>> - right-clicking and "Save Link As" command brings no file open dialog.
>
> Actually this isn't true for current m-c nightlies (with e10s on),
> though you do have to move the mouse before the native filepicker
> appears.  That's definitely a bug, but not the same bug as effects
> current trunk code with this patch -- there the filepicker never
> appears, even after moving the mouse.

Actually I *do* see this without your patch -- in a self-build made
from current trunk code pulled earlier today.  So this appears to be
yet another recent regression, which confuses our test results.

Comment 90

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8557929 [details] [diff] [review]
bugzilla-1083344-9.patch

I've now tested this on OS X 10.6.8, 10.7.5 and 10.8.5.  On all those versions of OS X it behaved as expected -- it appears to (correctly) be using a single (allow default) rule.

Among other things, I tried Print and then Save As PDF -- this always worked correctly.

Comment 91

[Steven Michaud [:smichaud] (Retired)]

I've now also tested on OS X 10.10.2.  My results were like on OS X 10.9.5, but with one additional problem:

Neither printing nor print preview work on OS X 10.10.2.

If you're able to test on 10.10.2, André, it might be worth your while to try to find new rules to add.  Otherwise, since we're already going to have the content sandbox rules off by default until bug 1124817 can be fixed, it's probably not worth the trouble right now.

Comment 92

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #87)

> (In reply to comment #80, wrt Save Page As)
>
> I see this behavior too, and in today's m-c nightly.  But it's a
> recent regression -- I don't see it in in the 2015-01-29 m-c
> nightly.  And like I said I didn't see it earlier with your patch.
>
> Since this new bug seriously messes up our testing, we'll need to
> test with trunk code that doesn't have it.  Later I'll check if a
> bug has been opened on it, and if not I'll open one myself.

This is bug 1127927.

Comment 93

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #89)

> - right-clicking and "Save Link As" command brings no file open dialog.

Actually this is bug 1128712, and has been around for a while (a few days).  And it appears to be a dup of bug 1127927.

For some reason I'm much better able to reproduce it on OS X 10.7.5 (than on OS X 10.9.5 and 10.10.2 where I was testing yesterday).

Comment 94

[André Reinald]

Following discussion with bsmedberg about the "print to PDF" and "load file" issues while we wait for file read and printing to be moved into the main process, I propose as a tradeoff between usability and security that I add rules allowing read and write only in $HOME (and subdirs), minus $HOME/Library (and subdirs).

Steven, do you think this tradeoff would make the patch landable in m-c?

Comment 95

[André Reinald]

(In reply to Steven Michaud from comment #88)
> Comment on attachment 8557929 [details] [diff] [review]
> bugzilla-1083344-9.patch
> 
> +  "        (appdir-regex
> \"(/XUL)|(\\.(js|jsm|css|xml|properties|ent|dtd|png|svg|gif|dylib))$\")\n"
> 
> Why don't you just allow read access to all of the appdir (as my own patch
> did)?  Remember that the appdir is passed (in a commandline parameter) from
> the chrome process to the content process, which I think means that the
> content process is meant to have read access to everything in it.

I figured out that only XUL plus those file types were needed, so I restricted the rule accordingly.
If you believe we should loosen it to allow any file in appDir, that's fine with me.

Comment 96

[Steven Michaud [:smichaud] (Retired)]

> If you believe we should loosen it to allow any file in appDir, that's fine with me.

Yes, I think we should.

By the way, XUL isn't even *in* the appdir.  It's currently the Contents/Resources/browser directory of the chrome process (e.g. FirefoxNightly.app/Contents/Resources/browser).

Comment 97

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #94)

Let me think about this.

We also need to test denying read and write access to the profile directory, to see what this might break.

Comment 98

[Steven Michaud [:smichaud] (Retired)]

> We also need to test denying read and write access to the profile directory

Here's the rule I'm going to test with (added to the end of André's ruleset):

  "    (deny file*\n"
  "        (home-subpath \"Library\Application Support\Firefox\Profiles\")\n"
  "        (home-subpath \"Library\Caches\Firefox\Profiles\"))\n"

It doesn't cover all cases:  Sometimes (for example with mochitests) the profile directories can be in different locations.  But it should be good enough for manual testing.

Comment 99

[Steven Michaud [:smichaud] (Retired)]

> Here's the rule I'm going to test with (added to the end of André's ruleset):
>
>  "    (deny file*\n"
>  "        (home-subpath \"Library\Application Support\Firefox\Profiles\")\n"
>  "        (home-subpath \"Library\Caches\Firefox\Profiles\"))\n"

Err, oops.  That should be this:

  "    (deny file*\n"
  "        (home-subpath \"/Library/Application Support/Firefox/Profiles\")\n"
  "        (home-subpath \"/Library/Caches/Firefox/Profiles\"))\n"

Comment 100

[André Reinald]

(In reply to Steven Michaud from comment #98)
> > We also need to test denying read and write access to the profile directory
> 
> Here's the rule I'm going to test with (added to the end of André's ruleset):
> 
>   "    (deny file*\n"
>   "        (home-subpath \"Library\Application Support\Firefox\Profiles\")\n"
>   "        (home-subpath \"Library\Caches\Firefox\Profiles\"))\n"
> 
> It doesn't cover all cases:  Sometimes (for example with mochitests) the
> profile directories can be in different locations.  But it should be good
> enough for manual testing.

The rules in the patch deny by default access to all files, so adding 2 *deny* rules should not change anything.

It's more about adding *allow* rules to address the "printing to pdf" and "loading from file" issues.

In rules language, my suggestion from comment 94 was to add those 2 rules at the *beginning* of the "else" block of the ruleset, but after the definition of "home-subpath":

  (allow file* home-subpath "/")
  (deny file* home-subpath "/Library/")

Comment 101

[André Reinald]

Following chat with Steven, and as a note to remember tomorrow, deny after allow don't seem to work, so an alternative rule would be:

  (allow file*
    (and
      (home-subpath "/")
      (not
        (home-subpath "/Library/"))))

Will test tomorrow.

Comment 102

[Steven Michaud [:smichaud] (Retired)]

Like André said, I discovered that a (deny ...) rule doesn't work in the general case after an (allow ...).  You'd *think* that the last rule in a set would override any rules that came before it, and there do seem to be some cases of this in Apple's own rulesets (under /System/Library/Sandbox/Profiles/).  But when I added the following rule to the end of André's existing rulset (after (allow appleevent-send ... )), it appeared to have no effect:

(deny file*)

It *should* have denied all access to all files, and thereby caused crashes.

So the test I described in comment #99 is (apparently) invalid.

But, like André also said, it doesn't seem that this matters.  As best I can tell, his current ruleset already denies access to the files in those two directories, and therefore to everything in the current user profile in "ordinary" usage (i.e. when not running mochitests).

So denying the content process direct access to files in the current profile appears to have no ill effect.  Neither André nor I has noticed them in earlier tests, and explicitly changing a few preferences (in Firefox : Preferences and about:config) worked as expected.  Apparently all preference reading and writing is already brokered through the chrome process.

Comment 103

[Steven Michaud [:smichaud] (Retired)]

>  (allow file* home-subpath "/")
>  (deny file* home-subpath "/Library/")

>  (allow file*
>    (and
>      (home-subpath "/")
>      (not
>        (home-subpath "/Library/"))))

I'm fine with things like this, in principle.  And yes, it should allow most ordinary cases of File : Open and Print : Save to PDF to work correctly with this patch's ruleset.  This likewise should allow us to turn this patch's ruleset on by default.

We can't set this commitment in stone:  Others' testing (once this bug's patch lands on the trunk) may turn up other important breakage we can't fix quickly/easily.  And we also need the tests of updating to go well (after this bug has landed on trunk).

But for the first time (since discovering my showstoppers) I'm reasonably comfortable with having a non-trivial set of content sandbox rules on by default.

I still think we should have a preference (possibly the sandbox level preference) to allow users to turn these rules off in about:config (and go back to the (allow default) ruleset that's currently on trunk).

Comment 104

[Steven Michaud [:smichaud] (Retired)]

Attachment: Console errors doing Print : OpenPDF in Preview on OS X 10.10.2

At your request, André, here are the errors I see trying to do Print : Open PDF In Preview on OS X 10.10.2, since you're not currently able to test there.

Comment 105

[André Reinald]

Attachment: bugzilla-1083344-10.patch

Added file access to home folder (except home/Library) as per comment 103. Fixes load from file and save as PDF.
Allows any file read from appDir instead of filtering through regex as per comment 96.
Added allow rules matching logs from comment 104. Should fix printing on Mac OS 10.10.

Comment 106

[André Reinald]

Attachment: rules.sb

Added file access to home folder (except home/Library) as per comment 103. Fixes load from file and save as PDF.
Allows any file read from appDir instead of filtering through regex as per comment 96.
Added allow rules matching logs from comment 104. Should fix printing on Mac OS 10.10.

Comment 107

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8561574 [details]
rules.sb

I'm going to be very busy with bug 1110888 for the rest of this week, so I won't have much time to spend on this bug until next week.

Comment 108

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8561570 [details] [diff] [review]
bugzilla-1083344-10.patch

I won't have a chance to do a real review of this patch (or its successors) until next week.  But like I said above (particularly in comment #54 and comment #103), I agree with the general approach, which I'd describe as follows:

1) We need a set of rules that, while not yet really secure, can be used as a starting point later on (when we have the time) for another set of rules that can do what's normally expected of a sandbox.

2) We need to have something that can be turned on by default on the trunk (at least), so that extension vendors can be alerted to the fact that they need to start adjusting *now* to the new environment.  Ideally, even our first set of rules should include all the restrictions that might effect extensions (e.g. denial of direct (unmediated) access to the profile directory), so that we break extensions only once.

3) Our default rules must not break major functionality -- there must not be any "showstoppers".

4) We should have a way for users to relax our default sandbox rules in about:config (going back to the empty ruleset that's currently on trunk).  So the empty ruleset would (say) be "level 0", André's ruleset for this bug would be "level 1", and "level 1" would be the default.

Comment 109

[André Reinald]

Attachment: rules.sb

Previous rules had a misspelling bug in the "else" block, but as I added the preference setting and forgot to set it to 1 while testing, it went unnoticed.

So remember to set "security.sandbox.macos.content.moreStrict" to 1 !

Comment 110

[André Reinald]

Attachment: bugzilla-1083344-11.patch

Previous rules had a misspelling bug in the "else" block, but as I added the preference setting and forgot to set it to 1 while testing, it went unnoticed.

So remember to set "security.sandbox.macos.content.moreStrict" to 1 !

PS: I keep in mind that you can't review till next week, and I have other stuff to work on till then.

Comment 111

[André Reinald]

Attachment: bugzilla-1083344-12.patch

Rebased patch v11 to current m-c -> v12
No other changes were made.

Comment 112

[André Reinald]

Yesterday, I finished setting up a multi-system Mac, so I'll test myself that critical features work well on 10.10 (notably "printing" and "open pdf in preview". So Steven, don't rush to test/review the patch.

Comment 113

[Steven Michaud [:smichaud] (Retired)]

> I finished setting up a multi-system Mac

Glad to hear it!

> So Steven, don't rush to test/review the patch.

I won't.  Bug 1110888 has spilled over to this week, and I think I'll need a couple more days of hard work to finish it.

Comment 114

[André Reinald]

I have split the patch in 2 parts:
- the 1st changes the c++ part but still uses the "(allow default)" rule,
- the 2nd changes the "(allow default)" rule with the real rules.

This split allows to apply the "read-sandbox-rules-from-file" patch instead of the 2nd one, and shorten the trial-error cycle, as it removes the need to recompile and even to restart Firefox in order to test new rules.

Comment 115

[André Reinald]

Attachment: bugzilla-1083344-core.patch

Core patch (see comment above), rebased on latest m-c, but rules are still "(allow default)", so this patch should land safely.

Comment 116

[André Reinald]

Attachment: bugzilla-1083344-13.patch

This patch is the "rules" part after the split introduced above.
It introduces new "allow" rules, which finally allow printing on MacOS 10.10.

Only known issue: "open pdf in preview" still doesn't work on 10.10 (it works on 10.9). This bug may take more time to fix than we'd like as no relevant message appears in system logs. Which reminds me of the microphone access issue also discussed above (c51 and c65).

However, this may not be a showstopper as:
1. there is a simple workaround: "save as pdf" then open the pdf from the Finder or from Preview,

2. one aim of activating the sandbox now is to cause crashes of incompatible add-ons, in order to bring their developers to revisit them with sandboxing in mind,

3. printing should not happen the content process in the first place, so we can consider this misbehavior as a message to our own devs: "please move printing away from the content process",

4. we already punched a huge hole in the sandbox by allowing read/write access to the home directory, in order to allow "save as pdf" and "open file" to work, plus a long list of rules to allow plain printing (c94, c103 and up).

Comment 117

[André Reinald]

(In reply to André Reinald from comment #115)
> Created attachment 8565964 [details] [diff] [review]
> bugzilla-1083344-core.patch
> 
> Core patch (see comment above), rebased on latest m-c, but rules are still
> "(allow default)", so this patch should land safely.

Additionally this patch adds setting the default value of "security.sandbox.macos.content.moreStrict" to 1.

Comment 118

[André Reinald]

Attachment: read-sandbox-rules-from-file.patch

Rebased this patch after c115, i.e. to be applied after:
bugzilla-1083344-core.patch

Comment 119

[André Reinald]

Attachment: rules.sb

As usual, updated separate rules.sb file, which is easier to read and maintain than the C quoted string.

Comment 120

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8565964 [details] [diff] [review]
bugzilla-1083344-core.patch

This looks fine to me.

> +pref("security.sandbox.macos.content.moreStrict", 1);

In the sandboxing meetings we've been talking about a "level" setting, so the name might get changed.  But this seems fine for now.

Comment 121

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8565964 [details] [diff] [review]
bugzilla-1083344-core.patch

(Following up comment #120)

Something I forgot to mention:

For bug 1110911 I think we'll have to remove all XUL dependencies from Mac sandboxing code (what's in security/sandbox/mac), which will mean that we can no longer query settings from there.  It shouldn't be hard to work around this -- for example we could pass the relevant setting in the MacSandboxInfo structure.  And we can cross that bridge when we come to it.

Comment 122

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8566004 [details] [diff] [review]
bugzilla-1083344-13.patch

This is *almost* there ... but not quite.

I discovered the following weirdness, on OS X 10.10.2, while trying to "Save as PDF" with the sandbox on (with security.sandbox.macos.content.moreStrict set to its default value of '1'):  The filepicker that shows up when you do this doesn't list any existing files/directories in its central window.

I agree that "Open PDF in Preview" not working on OS X 10.10 isn't a showstopper.  But that together with what I just found may be one.

I'll post whatever errors I see in a later comment.

Note that I once saw "Save Page As" disabled (grayed out) for every tab (with the sandbox on).  I haven't been able to reproduce this, so it may be a fluke.  But if I do figure out how to reproduce it, and it's easy to reproduce, this would also be a showstopper all by itself.

Comment 123

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8566004 [details] [diff] [review]
bugzilla-1083344-13.patch

Mostly my tests went just fine.

For example I tested that setting security.sandbox.macos.content.moreStrict to '0' and restarting really does turn off your "more strict" rules -- it does.

I also tested that, with the sandbox on, it's not possible to open a file in ~/Library -- it isn't (which is the correct behavior).

Comment 124

[Steven Michaud [:smichaud] (Retired)]

Here's what I see in the Console app when I try to "Print : Save As PDF":

Lots of the following error:
2/19/15 6:28:49.000 PM kernel[0]: Sandbox: plugin-container(2768) deny iokit-open nvSharedUserClient

Then:
2/19/15 6:28:59.182 PM com.apple.xpc.launchd[1]: (com.apple.DesktopServicesHelper.417928D3-59B5-4932-8C60-9E6306486CE3[2802]) Service exited due to signal: Abort trap: 6
2/19/15 6:28:59.190 PM ReportCrash[2801]: Saved crash report for DesktopServicesHelper[2802] version 784.2.3 to /Library/Logs/DiagnosticReports/DesktopServicesHelper_2015-02-19-182859_Retina.crash

It's conceivable that DesktopServicesHelper crashes could sometimes cause "Save Page As" to get greyed out.

Comment 125

[André Reinald]

Attachment: bugzilla-1083344-14.patch

I could not reproduce the bug of c#124 on my test 10.10.2 system, but I added an allow rule corresponding to the log above anyway. Plus I changed the pref name from "moreStrict" to "level" as per c#120.

Comment 126

[André Reinald]

Attachment: rules.sb

Updated rules.sb file reflecting last changes.

Comment 127

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8567168 [details] [diff] [review]
bugzilla-1083344-14.patch

> I added an allow rule corresponding to the log above

>  "        (iokit-user-client-class \"nvSharedUserClient\")\n"

This wasn't enough by itself.  But the problem I reported in comment #122 goes away if I also add the following two lines to your "allow iokit-open" rule:

>  "        (iokit-user-client-class \"IGAccelGLContext\")\n"
>  "        (iokit-user-client-class \"nvFermiGLContext\")\n"

In other words, with these three lines I no longer fail to see files/directories in the central panel of the filepicker that opens up when you try to do "Save As PDF" on OS X 10.10.2.  The DesktopServicesHelper still crashes (as it doesn't when the sandbox level is '0'), but I guess we'll just have to live with that for the time being.

With the addition of these two lines, I'm fine with you landing this path on trunk (mozilla-central).  I don't pretend to understand most of the rules in your ruleset ... and I doubt anyone else does, either (at least outside of Apple).  But I hope they'll make a starting point for a "real" (reasonably secure) content-process sandbox a few months down the line.

I've tried my best to find showstoppers, and so far you've fixed (worked around) all the ones I've found.  But that's no guarantee that others won't be found.  You will be the pointman, André, on any regressions from this patch.  You'll need to "fix" them (i.e. work around them), or argue that we should live with them until we can do content process sandboxing properly (by proxying file access, printing, and perhaps other things through the main process).

Congratulations, I think :-)

Comment 128

[Steven Michaud [:smichaud] (Retired)]

> Note that I once saw "Save Page As" disabled (grayed out) for every
> tab (with the sandbox on).  I haven't been able to reproduce this,
> so it may be a fluke.

I figured out how to reproduce this, at least on OS X 10.10.2.  But it
also happens with your new ruleset off (with "level" set to '0'), so
it's unrelated.

1) Use cmd-o to open a file on the Desktop.
2) Notice that Save Page As is now greyed out for all tabs in the
   browser window.

If I can't find an existing bug report, I'll open one myself.

Comment 129

[André Reinald]

Attachment: bugzilla-1083344-15.patch

(In reply to Steven Michaud from comment #127)
> >  "        (iokit-user-client-class \"IGAccelGLContext\")\n"
> >  "        (iokit-user-client-class \"nvFermiGLContext\")\n"

I added the 2 above rules to my patch, now at version 15!

I'm carrying over your r+ and flag this bug as "checkin-needed" for the 2 relevant patches (core + 15).

> Congratulations, I think :-)

It was a long ride... thanks for your assistance all the way!

Comment 130

[André Reinald]

Attachment: rules.sb

Set of rules matching patch 15.

Comment 131

[André Reinald]

Need checkin of:
attachment 8565964 [details] [diff] [review] - bugzilla-1083344-core.patch
attachment 8567485 [details] [diff] [review] - bugzilla-1083344-15.patch

Comment 132

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3195c37d04
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c11efebc455

Comment 133

[Steven Michaud [:smichaud] (Retired)]

In addition to file access and printing, we'll also need to proxy the creation of at least GMP plugins through the main process, as per bug 1057908.

See bug 1135326.  Note that this it's *not* a regression from this bug.  It was already happening with the allow-all content process sandbox.

Comment 134

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d3195c37d04
https://hg.mozilla.org/mozilla-central/rev/8c11efebc455

Comment 135

[Steven Michaud [:smichaud] (Retired)]

If worst comes to worst, and the test failures prove too difficult to work around, we *might* be able to fall back to a very simple ruleset that does nothing more than deny access to the ~/Library directory.  This would deny direct access to the browser cache, and thereby break misbehaving extensions.  So we'd still be able to break those extensions only once (if this is the only way in which we expect them to misbehave).

Comment 136

[Steven Michaud [:smichaud] (Retired)]

> This would deny direct access to the browser cache

And to the browser profile

Comment 137

[André Reinald]

Basically, the content process sandbox *should* be much tighter than it is now. So if even those quite loose rules break tests, loosening it further may not be the wise way to go.

As a compromise, I propose that we do the same as e10s: keep the patch applied and have it ride the trains with default level=1 for aurora and beta, but level=0 for final releases (when it comes there).

This should give exposure to real life testing, and also allow time to either punch additional holes, or discipline the content process properly to play well into the sandbox.

Comment 138

[André Reinald]

(In reply to Steven Michaud from comment #135)
> We *might* be able to fall back to a very simple ruleset that does nothing more than deny access to the ~/Library directory.

You're right Steven, that would be plan B. But that way, none will ever consider bug 1124817.

My understanding is that the sandbox is designed to break things in order to point out flaws in both add-ons and our current e10s architecture.

Of course, we can tone it down to just point out "add-on is trying to access the $HOME/Library". But is this what we really want?

Comment 139

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I disabled the pref in bug 1136407:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ed19dfc6443

Comment 140

[André Reinald]

(In reply to André Reinald from comment #138)
> (In reply to Steven Michaud from comment #135)
> > We *might* be able to fall back to a very simple ruleset that does nothing more than
> > deny access to the ~/Library directory.
> 
> You're right Steven, that would be plan B. But that way, none will ever consider bug 1124817.

Correction: bug 1136836 is a blocker even for "plan B".

Comment 141

[Steven Michaud [:smichaud] (Retired)]

> Correction: bug 1136836 is a blocker even for "plan B".

Yes.  Also see bug 1136407 comment #32.

Comment 142

[Steven Michaud [:smichaud] (Retired)]

For the record, here's my current list of things that will need be brokered through the main process, before we can have a "real" sandbox:

1) The native filepicker dialog (which among other things accesses files directly).
2) The native print and page setup dialogs (which access files and lots of other services).
3) Several other kinds of file access (bug 1124817).
4) Loading chrome URLs (bug 1136836).
5) All extension access to anything in the browser profile or cache.

Comment 143

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #142)

Oops, forgot to add the following:

6) Camera and microphone access (bug 1104615).

Comment 144

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #142)

Some more things I forgot:

7) Loading GMP plugins (bug 1057908).
8) Loading NPAPI plugins?  This probably won't be feasible until we get async plugin initialization working reliably.  It might also not be desirable, if this means that plugin initialization code will run unsandboxed in the main process.

Comment 145

[Steven Michaud [:smichaud] (Retired)]

Revised list of things that will need to be proxied through the main process (from the content process).  I dropped item 5 because I now realize that extensions actually run in the main process.  I dropped item 8 because I'm not entirely sure it's desirable to proxy the loading of NPAPI plugins through the main process.

1) The native filepicker dialog (which among other things accesses files directly).
2) The native print and page setup dialogs (which access files and lots of other services).
3) Several other kinds of file access (bug 1124817).
4) Loading chrome URLs (bug 1136836).
5) Camera and microphone access (bug 1104615).
6) Loading GMP plugins (bug 1057908).

Comment 146

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Just wanted to note that NPAPI plugins are already loaded by the main process. I don't think there are any security issues there since the plugin code runs in a separate process that can be sandboxed.

Comment 147

[Steven Michaud [:smichaud] (Retired)]

> 1) The native filepicker dialog (which among other things accesses files directly).

Billm and evilpie raise the possibility (at bug 1136407) that the Mac filepicker already runs from the main process in e10s mode ... at least most of the time.  I'm going to check (when I have time).  But at the very least we'll need to make sure that the filepicker never runs from the content process.

Comment 148

[Steven Michaud [:smichaud] (Retired)]

> Just wanted to note that NPAPI plugins are already loaded by the main process.

That's another thing I'll need to check on the Mac, when I have time.  Bug 1130435's symptoms seem to indicate that Flash is being launched from the content process.

Comment 149

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #147)

Also see bug 910384.

Comment 150

[Steven Michaud [:smichaud] (Retired)]

I wrote a handy interpose library to check ... and I found out that *both* the filepicker and the "Page Setup" dialog run from the main process.  Only the Print dialog doesn't (at least in normal usage).  In all cases I opened these dialogs using a menu command or menu shortcut.  This was in a fairly recent m-c nightly (and in e10s mode, of course).

I stand corrected ... and also relieved that more work has already been done than I first realized.

Comment 151

[Steven Michaud [:smichaud] (Retired)]

I extended my interpose library to hook calls to posix_spawnp() (which we use on the Mac to launch all our child processes), and I found I was also wrong about NPAPI plugins.  Those, too, are launched from the main process.

Comment 152

[Steven Michaud [:smichaud] (Retired)]

Attachment: Interpose library for comments #150 and #151

Comment 153

[Steven Michaud [:smichaud] (Retired)]

List of things that need to be proxied through the main process, revised yet again:

1) The native filepicker dialog.

This mostly already is (when the dialog is triggered from the menu).  But it isn't (yet) when triggered by <input type=file>.  See bug 910384 and bug 1101100.

2) The native print dialog.

By sheer chance the native page setup dialog is already launched from the main process.

3) Several other kinds of file access (bug 1124817).

4) Loading chrome URLs (bug 1136836).

5) Camera and microphone access (bug 1104615).

6) Loading GMP plugins (bug 1057908).

NPAPI plugins are already loaded from the main process.

Comment 154

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Steven Michaud from comment #153)
> List of things that need to be proxied through the main process, revised yet
> again:
> 
> 1) The native filepicker dialog.
> 
> This mostly already is (when the dialog is triggered from the menu).  But it
> isn't (yet) when triggered by <input type=file>.  See bug 910384 and bug
> 1101100.

I think you might have misunderstood. In the <input type=file> case, the file picker *is* proxied to the parent process. In other cases, the content process never even attempts to open a file picker, so no proxying is needed.

Comment 155

[Nils Ohlmeier [:drno]]

Just to make sure everyone understands why I linked bug 1136347 to this ticket here: looks like you landing broke getUserMedia() camera access on Mac OSX if e10s is turned on.

Comment 156

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

On OSX 10.10, not 10.9.5 it seems.

Comment 157

[André Reinald]

Attachment: bugzilla-1083344-rules-2.patch

Though posted in this bug, this patch adds 6 rules to address complaints listed in bugs 1136407 (which has been closed by switching the default sandbox level to 0), 1136347, and talked on #e10s yesterday. It needs to be applied on top of the previous patches, which landed in m-c on Feb 24.

It may make sense to keep this bug "open" as we'll likely have to go back to it. Hopefully to progressively tighten the sandbox. But I'm not sure of the policy here.

Comment 158

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8569914 [details] [diff] [review]
bugzilla-1083344-rules-2.patch

Let's try this and see what happens.

Comment 159

[Dave Townsend [:mossop]]

Comment on attachment 8569914 [details] [diff] [review]
bugzilla-1083344-rules-2.patch

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

::: security/sandbox/mac/Sandbox.mm
@@ +222,4 @@
>    "\n"
>    "    (allow file-write* (var-folders2-regex \"/org\\.chromium\\.[a-zA-Z0-9]*$\"))\n"
> +  "    (allow file-read*\n"
> +  "        (home-regex \"/Library/Application Support/Firefox/Profiles/[^/]+/extensions/[^/]+\\.xpi$\"))\n"

Not all extensions are in xpis files, some are unpacked directories. I think we should allow reads of these as well.. It's unlikely but there are also a few other locations where extensions can be installed: https://developer.mozilla.org/en-US/Add-ons/Installing_extensions

Comment 160

[Dave Townsend [:mossop]]

Please also see bug 1135953 where it seems the sandbox broke loading file urls in e10s

Comment 161

[Steven Michaud [:smichaud] (Retired)]

Attachment: Interpose library for comments #150 and #151

Comment 162

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Just to reiterate what Dave said, this patch is not ready to land, for several reasons:

1. Profiles can be stored anywhere in the file system.
2. Add-ons need not be stored in the profile.
3. file: URLs access the file system directly right now.

I think the only reasonable thing to land right now is a patch that allows read access to the entire file system.

Also, although we may not understand why certain rules are needed, there should at least be comments why they were added. Something like "Added to avoid breaking gUM" would be fine.

Comment 163

[Steven Michaud [:smichaud] (Retired)]

> I think the only reasonable thing to land right now is a patch that
> allows read access to the entire file system.

This rather defeats the purpose of having a sandbox at all. 

Instead of doing this, I suggest sticking with the allow-all sandbox
as the default, for the time being (what we get when
security.sandbox.macos.content.level is set to '0', as it currently
is).

I also suggest postponing work on a "real" sandbox until all the
issues on my list from comment #153 have been dealt with.  Item #1 may
already have been taken care of.  But I strongly suspect the list is
incomplete -- that we'll discover more functionality that need to be
brokered from the content process to the main process.

Until we finish brokering things to the main process, we'll inevitably
find ourselves either with 1) a mass of detailed and largely
inexplicable rules that we (mostly) arrive at by trial and error or 2)
a set of rules that in effect allows everything.

This doesn't mean we should stop working altogether on a more
restrictive set of rules.  But for the time being our main purpose in
doing this should be to find more "services" that we need to broker
from the content process to the main process.

Comment 164

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #159)

> Not all extensions are in xpis files, some are unpacked directories.

Dave, is it possible to come up with a reasonably comprehensive list (expressed in regular expressions) on where extensions are most likely to be?  If need be, it can just be "all files whose names match [blah]".

As I said above, we don't have to completely abandon our game of whack a mole ... as long as it doesn't get too out of hand, and we try to use it to find which services need to be proxied to the main process.

Comment 165

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Steven Michaud from comment #163)
> > I think the only reasonable thing to land right now is a patch that
> > allows read access to the entire file system.
> 
> This rather defeats the purpose of having a sandbox at all. 

Well, it prevents people from writing to or deleting your files. I think that has a lot of value. It also ensures that no one accidentally adds code that writes to files from the content process. That also has value.

The problem with waiting until everything is done before we enable the sandbox is that more things break while you're fixing the known problems.

Comment 166

[Dave Townsend [:mossop]]

(In reply to Steven Michaud from comment #164)
> (In reply to comment #159)
> 
> > Not all extensions are in xpis files, some are unpacked directories.
> 
> Dave, is it possible to come up with a reasonably comprehensive list
> (expressed in regular expressions) on where extensions are most likely to
> be?  If need be, it can just be "all files whose names match [blah]".
> 
> As I said above, we don't have to completely abandon our game of whack a
> mole ... as long as it doesn't get too out of hand, and we try to use it to
> find which services need to be proxied to the main process.

As Bill noted extensions can actually be installed anywhere, developers often make use of a feature that allows you to "point" Firefox to your extension's source directory during development to save the hassle of reinstalling constantly. For those cases a hard-coded regexp isn't possible. If there is an option to do this at runtime then we can just query the extension manager for the locations of all installed extensions, though that can change with restartless extensions.

For users on release and beta where we will soon be requiring signed extensions I think we'd be safe to restrict to just the fixed install locations that we know about.

Comment 167

[Steven Michaud [:smichaud] (Retired)]

OK, then, how about we do this:

Make the current ruleset (and continued work on it) level 2.

Create a level 1 ruleset that basically has the following rules, and is the default:
a) Read everything, everywhere.
b) Allow writes to everywhere in the home directory outside the ~/Library directory.
c) Allow all "non-file" operations.

Keep our current level 0 "allow everything" ruleset.

Without b) and c), the level 1 ruleset couldn't be the default -- too many things would break, including "save as" and downloads.

What do you think, André?  Have I missed something?

Comment 168

[Steven Michaud [:smichaud] (Retired)]

> Without b) and c), the level 1 ruleset couldn't be the default --
> too many things would break, including "save as" and downloads.

Actually, with b) "save as" and downloads might still work.  We'd
definitely need to test without it, though, and also make sure there
aren't any other showstoppers.

There's no way we can avoid c), though.

Comment 169

[Steven Michaud [:smichaud] (Retired)]

> with b)

without b)

Comment 170

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

(In reply to Bill McCloskey (:billm) from comment #165)
> It also ensures that no one accidentally adds code
> that writes to files from the content process. That also has value.
> 
> The problem with waiting until everything is done before we enable the
> sandbox is that more things break while you're fixing the known problems.

Agreed; this has been an occasional problem on Linux and B2G.  OS X is definitely in a stronger position than Linux as far as being able to apply fine-grained policies to system resource access (without needing to implement the filtering/brokering functionality from scratch), so it would be nice if that could be leveraged to reduce sandboxability regressions in cross-platform Firefox/Gecko code.

Comment 171

[Steven Michaud [:smichaud] (Retired)]

> The problem with waiting until everything is done before we enable the
> sandbox is that more things break while you're fixing the known problems.

I'm not suggesting we "wait until everything is done".  I do think we need to (mostly) postpone fixing individual access violations until we've proxied everything to the main process that we already know needs proxying.  As you can see from André's current ruleset, most of the rules (and especially the non-file ones) are (basically) inexplicable, and have been arrived at through pure trial and error.  And I think most (if not all) of these violations happen because we're doing something from the content process that we need to do from the main process.  Tracking down this kind of violation takes a lot of time and may ultimately be fruitless -- we may never find them all.

Tracking this kind of violation only makes sense as part of an effort to find what needs to be proxied to the main process.  If we keep tracking them without proxying everything that needs proxying, we'll be wasting our time.  We won't be able to know whether newly found violations are the result of known problems (things we already know need proxying) or unknown ones.

Comment 172

[Nils Ohlmeier [:drno]]

Comment on attachment 8569914 [details] [diff] [review]
bugzilla-1083344-rules-2.patch

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

Just confirming that this fixes the camera issue reported in bug 1136347

Comment 173

[Steven Michaud [:smichaud] (Retired)]

For my suggestion in comment #167.  (Note corrections in following two comments.)

Comment 174

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I like the proposal in comment 167.

Comment 175

[André Reinald]

I believe a "level 1" as proposed in comment 167 is useless, as it basically duplicates what default filesystem permissions are doing (minus write in the ~/Library folder).

As I understood it,

- level 0 is disabling the sandbox,

- level 1 should draw a line around what Firefox currently needs in a *normal usage*, plus of course not break tests,

- level 2 should be the ideal sandbox, where we want to go, but which would break lots of functionality right now.

I believe the current level 1 ruleset needs few iterations to be acceptable as the default by most users. I'm willing to address the concerns expressed in the latest comments (159 and later).

Comment 176

[André Reinald]

(In reply to Dave Townsend [:mossop] from comment #159)
> Comment on attachment 8569914 [details] [diff] [review]
> bugzilla-1083344-rules-2.patch
> 
> Review of attachment 8569914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/sandbox/mac/Sandbox.mm
> @@ +222,4 @@
> >    "\n"
> >    "    (allow file-write* (var-folders2-regex \"/org\\.chromium\\.[a-zA-Z0-9]*$\"))\n"
> > +  "    (allow file-read*\n"
> > +  "        (home-regex \"/Library/Application Support/Firefox/Profiles/[^/]+/extensions/[^/]+\\.xpi$\"))\n"
> 
> Not all extensions are in xpis files, some are unpacked directories. I think
> we should allow reads of these as well.. It's unlikely but there are also a
> few other locations where extensions can be installed:
> https://developer.mozilla.org/en-US/Add-ons/Installing_extensions

On Mac, it seems they are either in ~/Library/... or in /Library/... I can deal with both.
Please also note that except ~/Library/... the whole ~/... is read/write accessible.

Comment 177

[André Reinald]

(In reply to Steven Michaud from comment #141)
> > Correction: bug 1136836 is a blocker even for "plan B".
> 
> Yes.  Also see bug 1136407 comment #32.

That one has been addressed in the latest patch, based on Brad's own syslogs.
Together with reading any .xpi from the user's profile.

Comment 178

[André Reinald]

(In reply to Dave Townsend [:mossop] from comment #160)
> Please also see bug 1135953 where it seems the sandbox broke loading file
> urls in e10s

This also has been addressed: reading anything from ~/... is allowed, which means anything downloaded by the user. If you think a significant number of (e10s) users will need to read files from outside the homedir, I suggest we list allowable extensions. That is until file:// access is brokered.

Comment 179

[André Reinald]

(In reply to Bill McCloskey (:billm) from comment #162)
> Just to reiterate what Dave said, this patch is not ready to land, for
> several reasons:
> 
> 1. Profiles can be stored anywhere in the file system.

As long as they are in the homedir, it's ok. If they are not in the homedir (which I believe is very uncommon), there will be problems anyway with filesystem permissions, as writes would not be allowed, and the chrome process needs write access there.

> 2. Add-ons need not be stored in the profile.

According to https://developer.mozilla.org/en-US/Add-ons/Installing_extensions they may be stored in their "shared" counterpart in /Library, which I can allow.

> 3. file: URLs access the file system directly right now.

I allowed homedir reading as a compromise. If that's not enough I can also allow select file extensions.

> I think the only reasonable thing to land right now is a patch that allows
> read access to the entire file system.

And I think it's far too permissive. The current rules may break 1% use cases, only for nightly users with both e10s and sandboxing enabled.

> Also, although we may not understand why certain rules are needed, there
> should at least be comments why they were added. Something like "Added to
> avoid breaking gUM" would be fine.

I commented a few (I did for printing and print preview for instance), and can comment a few others. But not all. Without some basic subset, the content process simply crashes at launch, before even attempting to load anything.

Comment 180

[André Reinald]

(In reply to Dave Townsend [:mossop] from comment #166)
> As Bill noted extensions can actually be installed anywhere, developers
> often make use of a feature that allows you to "point" Firefox to your
> extension's source directory during development to save the hassle of
> reinstalling constantly.

Having their extensions anywhere in their homedir will be fine for now.
For file:// access I had to allow reading from homedir anyway.

Comment 181

[André Reinald]

To summarize my approach:

1. Loosen "level 1" rules to make sure they don't break tests and *normal* usage.

2. In a few short iterations, have them satisfying enough that we can make "level 1" the default.

3. "level 1" becomes a new limit which we should respect.

4. Open "todo" bugs and suggest temporary workarounds.

5. When a "todo" bug is fixed, tighten the sandbox accordingly.

6. If something new breaks, consider that thing should be fixed.

7. Keep in mind the purpose of this bug, which is aimed at staying open.

Comment 182

[André Reinald]

Attachment: bugzilla-1083344-rules-3.patch

Minor changes. Carrying on r+ from previous patch.
Taking into account comments from Dave and info from:
https://developer.mozilla.org/en-US/Add-ons/Installing_extensions

Comment 183

[André Reinald]

Checkin needed for Attachment 8570520 [details] [diff]

Comment 184

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to André Reinald from comment #179)
> And I think it's far too permissive. The current rules may break 1% use
> cases, only for nightly users with both e10s and sandboxing enabled.

No opinion on this, but isn't e10s (and sandboxing?) on by default in Nightly now?

Comment 185

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #181)

As I said in comment #163 and comment #171, I've come to think our current approach to the Mac content sandbox is misguided.  (By "current approach" I mean continuing to track individual access violations before we've brokered services to the main process that we already know need brokering.)  But as an institution we've committed ourselves to having a content process sandbox right away (while acknowledging that it's not perfect), and André's put in a lot of work to accomplish this goal.

So as long as the current approach doesn't get totally stuck in the mud, I'm willing to go along with it.

Yes, it's a messy compromise.  In a lot of cases it doesn't fix problems completely -- only in cases that effect the vast majority of users.  And (as I've already mentioned above), André's ruleset contains lots of rules that we don't understand, and can only justify on the grounds of trial and error.  But I think it's good enough for now.  And (practically speaking) it's the only way we're going to be able to meet the goal we've set ourselves -- of having a Mac content process sandbox "right away".

I'm pretty sure that somewhere down the line (after we've brokered everything that needs brokering) we'll need to start over again from scratch.  Hopefully we'll then be able to eliminate all the rules that are now inexplicable, or spend the time necessary to track down why we need them (probably after serious reverse engineering of the OS).  But let's postpone worrying about that until the time comes.

Comment 186

[Steven Michaud [:smichaud] (Retired)]

> isn't e10s (and sandboxing?) on by default in Nightly now?

It has been since December 2014.  But until this bug's patch landed (comment #134) it used an empty ("allow all") ruleset.  As of the patch for bug 1136407 (and the 2015-02-25 m-c nightly) we're back to using the empty ruleset (which is now what we get when we set security.sandbox.macos.content.level to '0').  But hopefully, over the next few days/weeks André will be able to make the changes necessary to restore the default level to '1'.

Note that e10s isn't on by default in any other distros than mozilla-central nightlies, so the content process sandbox isn't either.

Comment 187

[Dave Townsend [:mossop]]

(In reply to André Reinald from comment #178)
> (In reply to Dave Townsend [:mossop] from comment #160)
> > Please also see bug 1135953 where it seems the sandbox broke loading file
> > urls in e10s
> 
> This also has been addressed: reading anything from ~/... is allowed, which
> means anything downloaded by the user. If you think a significant number of
> (e10s) users will need to read files from outside the homedir, I suggest we
> list allowable extensions. That is until file:// access is brokered.

Note that in the case of that bug at least the reporter was trying to load pages from /tmp and so will still be blocked here.

Comment 188

[André Reinald]

(In reply to Dave Townsend [:mossop] from comment #187)
> Note that in the case of that bug at least the reporter was trying to load
> pages from /tmp and so will still be blocked here.

Then should we:
- allow extensions html|png|jpg|txt|js|css (to be defined) from anywhere ?
- accept the restriction "into homedir" until file:// urls are brokered ?

Comment 189

[Dave Townsend [:mossop]]

(In reply to André Reinald from comment #188)
> (In reply to Dave Townsend [:mossop] from comment #187)
> > Note that in the case of that bug at least the reporter was trying to load
> > pages from /tmp and so will still be blocked here.
> 
> Then should we:
> - allow extensions html|png|jpg|txt|js|css (to be defined) from anywhere ?
> - accept the restriction "into homedir" until file:// urls are brokered ?

My opinion matches Bill's, until we have brokered file:// urls we should allow reads everywhere. But I don't know what the prevalence is here. How far off is file: brokering?

Comment 190

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4670e746db40

Comment 191

[Steven Michaud [:smichaud] (Retired)]

Attachment: Interpose library for investigating what needs brokering

I've continued to add functionality to my interpose library (mentioned above).  It now logs all calls to open(), plus all calls to write() made on "regular files", made from the content process.  These are used by our own low-level NSPR methods.  But the OS also uses them for at least some file operations.

This should be useful trying to find new "services" to broker, and also to find legitimate cases of file access that we will need to allow.  Among other things, I notice that fonts are read from the content process -- but I don't know whether this is something we should broker to the main process.  I suspect probably not (since we'll likely need to use them directly from the content process).

Comment 192

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

(In reply to Steven Michaud from comment #191)
> Among other things, I notice that fonts are read from the content process -- but I
> don't know whether this is something we should broker to the main process. 

I filed bug 1026063, but at least in the short term on B2G and desktop Linux, I intend to let content processes successfully call open() on font files.

For avoidance of confusion about the term "broker": following the example set by Chromium, I've been using it to refer to the technique of intercepting system calls and simulating them by communicating with an unsandboxed process, transparently to the offending code, in order to get finer-grained filtering than the OS natively provides; e.g., what the Mac can do with (allow file-read* (subpath "/System/Library/Fonts")) requires a broker on Linux.  This is not the sense in which it's used above, so it's possible there's been some miscommunication here.

Comment 193

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4670e746db40

Comment 194

[André Reinald]

Attachment: bugzilla-1083344-rules-4.patch

To be applied on top of current m-c.
1. Make level 1 allow "read everywhere", but keep exception about $HOME/Library
2. The current level 1 becomes the new level 2.
3. Raise the default level to 1.

Comment 195

[André Reinald]

Steven, I expect that though we both touch Sandbox.mm (in this bug and in bug 1110911), there won't be conflicts.

Comment 196

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8576741 [details] [diff] [review]
bugzilla-1083344-rules-4.patch

This looks fine to me.  But let's not land it until after tomorrow's meeting.  (It might need some minor rebasing after my patch for bug 1110911 lands, but I expect only the offsets will change.)

Then, of course, you'll need to see what it breaks (if anything), and figure out how to deal with that.

Comment 197

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8576741 [details] [diff] [review]
bugzilla-1083344-rules-4.patch

Sorry, but I'm having second thoughts about this.

Shouldn't level 1 allow file reads everywhere, including in the ~/Library directory?

I think it's fine, though, to prevent file writes to the ~/Library directory.

Comment 198

[Steven Michaud [:smichaud] (Retired)]

I imagine this is one of the questions we'll need to resolve at tomorrow's meeting.

Comment 199

[André Reinald]

As per this comment, it looks like 10.10 tests are now available on try:
https://bugzilla.mozilla.org/show_bug.cgi?id=1136407#c53

So pushed to try, asking for tests on 10.10:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4704aaec10fd

Comment 200

[André Reinald]

Tests all green with sandbox level=1.
Removed "leave-open".
Should close this bug.

Comment 201

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9af90dce2c83

Comment 202

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9af90dce2c83

Comment 203

[Steven Michaud [:smichaud] (Retired)]

Attachment: Interpose library for investigating what needs brokering

Here's my latest revision.  It tracks file opens/reads/writes in our content process or other apps' (e.g. Chromes's and Safari's) background processes.  It also tracks Apple events as they're being processed in Foundation framework code.

I have some good news about Apple events:  It seems the ones we care about are all sent to the main/chrome process (as they should be).  I was concerned the OS might be sending some of them to the content process.  (That's not the end of the story -- both André and I have seen sandbox access violations concerning Apple events, which we'll eventually have to figure out.  But dealing with that seems less urgent than it once did.)

Comment 204

[Steven Michaud [:smichaud] (Retired)]

I've opened bug 1149706 (a meta bug) on issues WRT Mac content process sandboxing that need to be resolved in future work in other bugs, based on the foundation that André has laid here.