1186158 - Receive notifications of sandbox violations in the browser on OS X

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Recently I discovered that this is possible in principle:

sandboxd calls notify_post("com.apple.sandbox.violation.*") every time it sends a sandbox violation report to syslogd.  The notification subsystem to which this call belongs is documented in /usr/include/notify.h.

I'm going to try to write a patch here that does this.

If we had this kind of information, we could (for example) crash the content process or warn the user on sandbox violations.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Proof of concept patch

This patch is pretty minimal -- it just logs sandbox violations to stdout and the system console.  But it shows that we *can* log them, and the limitations of trying to do so.

Just to make sure people don't see these logs accidentally, I've preffed off my patch.  I've also made sure it only "works" on OS X 10.9 and up, and then only in non-release builds.

I was afraid ASL queries wouldn't be very performant, but I was pleasantly surprised -- in my tests they take at most a second or two.  A more serious problem is that the notifications themselves are sometimes sent (by sandboxd) several 10s of seconds after a sandbox violation.  But when this happens, the notification that sandboxd sends to syslogd is also very late.

The bottom line, as best I can tell, is that we'll never receive these notifications synchronously with the actual sandbox violations.

Comment 2

[Stephen A Pohl [:spohl]]

Comment on attachment 8638220 [details] [diff] [review]
Proof of concept patch

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

::: browser/app/profile/firefox.js
@@ +1259,5 @@
>  
> +#if defined(XP_MACOSX) && defined(MOZ_SANDBOX)
> +// If this pref is on, log sandbox violations to stdout and the system console.
> +// This is currently only supported in non-release builds.  See bug 1186158.
> +pref("security.sandbox.mac.track.violations", false);

Can we simply omit this from this file? Having this here will show this pref in about:config, but I don't believe this is user-facing enough to warrant having it there. What do you think?

::: widget/cocoa/nsSandboxViolationSink.mm
@@ +47,5 @@
> +
> +// ViolationHandler() is always called on its own secondary thread.  This
> +// makes it unlikely it will interfere with other browser activity.
> +
> +#define ONLY_LOG_OURS 1

Would it make sense to make this a pref in about:config as well? This would require a restart of Firefox as it stands, but would be easier than a recompile. If we keep this, I'd prefer a name like "ONLY_LOG_OUR_VIOLATIONS".

@@ +70,5 @@
> +  asl_set_query(query, ASL_KEY_REF_PID, query_pid, ASL_QUERY_OP_EQUAL);
> +#endif
> +  aslresponse response = asl_search(nullptr, query);
> +
> +  // Each time ViolationHandler() is called we grab as many of "our" messages

Should this be "[...] as many messages as are available."? "our messages" only applies if we're filtering for our own violations, right?

@@ +73,5 @@
> +
> +  // Each time ViolationHandler() is called we grab as many of "our" messages
> +  // as are available.  Otherwise we might not get them all.
> +  if (response) {
> +    for (;;) {

nit: |while (true)|

@@ +75,5 @@
> +  // as are available.  Otherwise we might not get them all.
> +  if (response) {
> +    for (;;) {
> +      aslmsg found;
> +      const char *id_str;

nit: s/const char *id_str/const char* id_str/

@@ +77,5 @@
> +    for (;;) {
> +      aslmsg found;
> +      const char *id_str;
> +
> +      for (found = nullptr, id_str = nullptr; ;) {

Let's change this to:

  aslmsg hit = nullptr;
  aslmsg found = nullptr;
  const char* id_str = nullptr;

  while (hit = aslresponse_next(response))
  {
    ...
  }

and let's remove the |if (!hit)| check at the beginning of the loop.

@@ +97,5 @@
> +      if (!found) {
> +        break;
> +      }
> +
> +      const char *pid_str = asl_get(found, ASL_KEY_REF_PID);

nit: s/const char *pid_str/const char* pid_str/

@@ +98,5 @@
> +        break;
> +      }
> +
> +      const char *pid_str = asl_get(found, ASL_KEY_REF_PID);
> +      const char *message_str = asl_get(found, ASL_KEY_MSG);

nit: s/const char *message_str/const char* message_str/

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: Proof of concept patch, rev1

OK, then, how's this? :-)

I basically agree with all your suggestions.

Comment 4

[Stephen A Pohl [:spohl]]

Comment on attachment 8638774 [details] [diff] [review]
Proof of concept patch, rev1

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

Looks great, thanks! :-)

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fd14cf6b415

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX Werror bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=12158424&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed1cf289546

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Sigh.  I'll fix it.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

This is all it took:

-+      while (hit = aslresponse_next(response)) {
++      while ((hit = aslresponse_next(response))) {

Stephen has already given me an r+ for this change on IRC.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

But now mozilla-inbound is closed.  I'll wait until it reopens.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63c709f71ce3

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63c709f71ce3