Closed Bug 1186158 Opened 5 years ago Closed 5 years ago

Receive notifications of sandbox violations in the browser on OS X

Categories

(Core :: Security: Process Sandboxing, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: smichaud, Assigned: smichaud)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
See Also: → 1185084
See Also: → 1186187
Assignee: nobody → smichaud
Attached patch Proof of concept patch (obsolete) — Splinter Review
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.
Attachment #8638220 - Flags: review?(spohl.mozilla.bugs)
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/
Attachment #8638220 - Flags: review?(spohl.mozilla.bugs)
OK, then, how's this? :-)

I basically agree with all your suggestions.
Attachment #8638220 - Attachment is obsolete: true
Attachment #8638774 - Flags: review?(spohl.mozilla.bugs)
Attachment #8638774 - Attachment is patch: true
Comment on attachment 8638774 [details] [diff] [review]
Proof of concept patch, rev1

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

Looks great, thanks! :-)
Attachment #8638774 - Flags: review?(spohl.mozilla.bugs) → review+
Sigh.  I'll fix it.
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.
But now mozilla-inbound is closed.  I'll wait until it reopens.
https://hg.mozilla.org/mozilla-central/rev/63c709f71ce3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.