1215303 - Add a 'Permissive' mode to the file broker

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Julian Hector [:tedd] [:jhector]]

Currently, the file broker implemented in Bug 930258 only opens a file if it is listed in the whitelist of the policy factory[1]. The file descriptor will then be passed back to the content process that requested it via IPC.
If the content process doesn't have access to the requested file, it will simply return a failure message.

It would be nice to have a 'permissive' mode similar to SELinux, where the file broker is running, but instead of denying access to the requested file, it should grant the access (by opening the file and return the file descriptor) but additionally make a log entry that states that this request would have been denied if the policies were enforced.

This would allow to use the device and collect the error messages after a while and use that information to expand the whitelist of the policy factory.

[1] http://mxr.mozilla.org/mozilla-central/source/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp#45

Comment 1

[Julian Hector [:tedd] [:jhector]]

I would like to work on this, and propose to make it only available in an engineering build and use an environment variable tho switch the broker into permissive mode. Which can then be used to start b2g.sh, something like this:

> MOZ_BROKER_PERMISSIVE=1 b2g.sh

what do you think :jld?

Comment 2

[Julian Hector [:tedd] [:jhector]]

Attachment: DO NOT COMMIT - enable broker by default

Comment 3

[Julian Hector [:tedd] [:jhector]]

Attachment: WIP Add permissive mode to file broker

Ok this is what I have so far. The patch adds the option to enable permissive mode via the environment variable: MOZ_PERMISSIVE_CONTENT_SANDBOX.

I also added a method for the logging which is also used by the broker to log any denied operations (regardless of permissive mode).

The log message indicates whether or not it was logged because of permissive mode.

Could you give me some feedback please :jld and also maybe :kang?

I will also attach some sample output from my Aries device.

Comment 4

[Julian Hector [:tedd] [:jhector]]

Attachment: Sample output from file broker logging

Here is some sample output, when using permissive mode on my Aries device.

Comment 5

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

Comment on attachment 8674910 [details] [diff] [review]
WIP Add permissive mode to file broker

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

This seems reasonable, with some comments about the logging.  Sorry for taking so long to get to this.

::: security/sandbox/linux/broker/SandboxBroker.cpp
@@ +397,4 @@
>        }
>      } else {
>        MOZ_ASSERT(perms == 0);
> +      AuditDenial(req.mOp, req.mFlags, pathBuf, permissive);

I'd been avoiding logging denials by default because some of them are expected (files that don't exist, or files that don't actually need to be opened) and having error messages about them might confuse people reading the log.

@@ +414,5 @@
>  
> +void
> +SandboxBroker::AuditDenial(int aOp, int aFlags, const char* aPath,
> +                           bool aPermissive)
> +{

Something that could be done here (because it's in the broker instead of the client) is to lstat the path to see if it actually exists, and mention that in the log message.  I was trying to avoid doing that in normal enforcing mode, because the path could be hostile (even though lstat should be relatively side-effect-free… in theory), but if it's only in permissive mode then that's not a concern.

@@ +415,5 @@
> +void
> +SandboxBroker::AuditDenial(int aOp, int aFlags, const char* aPath,
> +                           bool aPermissive)
> +{
> +  SANDBOX_LOG_ERROR("SandboxBroker: denied op=%d rflags=%d path=%s for pid=%d " \

Nit: I'd been printing out the flags with %o so that they're easier to look up in <bits/fcntl-linux.h> when it matters.

Comment 6

[Julian Hector [:tedd] [:jhector]]

Thank you very much :jld and :kang.

Just so that I understand that correctly, you want to remove the default logging of denials (because the path might not exists) and add a lstat check in the AuditDenail function which checks if the file exists.

Should the file not exist, do you want to still print the denial but with an additional value stating that it exists? Or rather not log it at all?

Thanks, once that is clear I will update the patch.

Comment 7

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

(In reply to Julian Hector [:tedd] [:jhector] from comment #6)
> Just so that I understand that correctly, you want to remove the default
> logging of denials (because the path might not exists) and add a lstat check
> in the AuditDenail function which checks if the file exists.

Yes.  It might be worth release-asserting that the permissive flag is set before doing the lstat, but that might be more paranoia than it needs.

> Should the file not exist, do you want to still print the denial but with an
> additional value stating that it exists? Or rather not log it at all?

Logging it with some indication that it doesn't exist seems like the most useful thing to do in that case.

Comment 8

[Julian Hector [:tedd] [:jhector]]

Thanks :jld, I will work it into the patch and update it.

Comment 9

[Julian Hector [:tedd] [:jhector]]

Attachment: WIP Add permissive mode to file broker [v2]

Ok here is a new patch where I addressed the feedback in Comment 5, :jld about the lstat() check, in this patch I set the default to exists = false; and only when lstat() succeeds it will be set to true.

(I also mentioned that in a comment in the code) As far as I know lstat() can not only fail because of ENOENT (file doesn't exist) but also because of insufficient permissions.

For now b2g is running as root so we shouldn't get any permission errors, but in the future this might not be the case, so do you want the log to distinguish between ENOENT and EACCES?

Comment 10

[Julian Hector [:tedd] [:jhector]]

Attachment: Permissive file broker log with exists indication

New logging output which contains the indication whether or not the requested file exists.

Comment 11

[Julian Hector [:tedd] [:jhector]]

Oh I forgot to mention, the log file is from my Z3C device.

Comment 12

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

Comment on attachment 8683294 [details] [diff] [review]
WIP Add permissive mode to file broker [v2]

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

::: security/sandbox/linux/broker/SandboxBroker.cpp
@@ +412,5 @@
>  }
>  
> +void
> +SandboxBroker::AuditDenial(int aOp, int aFlags, const char* aPath,
> +                           bool aPermissive)

aPermissive is always true now.

@@ +420,5 @@
> +  // Assume the path doesn't exist
> +  bool exists = false;
> +
> +  struct stat statBuf;
> +  memset(&statBuf, 0, sizeof(statBuf));

If there's a reason this needs to be zeroed, a comment would help.

@@ +426,5 @@
> +  if (lstat(aPath, &statBuf) == 0) {
> +    // XXX: If lstat succeeded, then the file/symlink definitely exists.
> +    // But other errors than ENOENT may lead to the failure of lstat.
> +    // For instance EACCES means insufficient permissions, it may be worth
> +    // considering logging such a failure instead of assuming it doesn't exist.

Might be a good idea to include the errno, even on B2G.  This could just set errno=0 on success and use strerror(errno) in the log message, because strerror(0) is "Success".  (I didn't use strerror in the client because it's not async signal safe.)

Comment 13

[Julian Hector [:tedd] [:jhector]]

Attachment: WIP Add permissive mode to file broker [v3]

(In reply to Jed Davis [:jld] from comment #12)
> Comment on attachment 8683294 [details] [diff] [review]
> WIP Add permissive mode to file broker [v2]
> 
> Review of attachment 8683294 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/sandbox/linux/broker/SandboxBroker.cpp
> @@ +412,5 @@
> >  }
> >  
> > +void
> > +SandboxBroker::AuditDenial(int aOp, int aFlags, const char* aPath,
> > +                           bool aPermissive)
> 
> aPermissive is always true now.

True, so AuditDenial is technically only called in permissive mode, but I left the permissive indication in the log message anyways, do you want to have it removed? (talking about the 'permissive=1' part of the log)

> @@ +420,5 @@
> > +  // Assume the path doesn't exist
> > +  bool exists = false;
> > +
> > +  struct stat statBuf;
> > +  memset(&statBuf, 0, sizeof(statBuf));
> 
> If there's a reason this needs to be zeroed, a comment would help.

It is a habit of mine to work with initialized variables, especially on the stack, but in this case it doesn't really do any good since the buffer isn't used after the lstat() call.

> Might be a good idea to include the errno, even on B2G.  This could just set
> errno=0 on success and use strerror(errno) in the log message, because
> strerror(0) is "Success".  (I didn't use strerror in the client because it's
> not async signal safe.)

Renamed the 'exists' parameter in the log message to 'error', and in case the file exists it says for example: '... error="Success"'

Comment 14

[Julian Hector [:tedd] [:jhector]]

Attachment: Permissive log messages

Comment 15

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

Comment on attachment 8684530 [details] [diff] [review]
WIP Add permissive mode to file broker [v3]

Looks good.  I guess the remaining part is enabling the broker on all devices if it's permissive?  (Which will break WebGL on flame-kk via bug 1199866 until that's taken care of, but this is an opt-in dev feature so that's okay.)

Comment 16

[Julian Hector [:tedd] [:jhector]]

Attachment: Part 1: add permissive mode

Comment 17

[Julian Hector [:tedd] [:jhector]]

Attachment: Part 2: automatically enable broker when in permissive mode

Comment 18

[Julian Hector [:tedd] [:jhector]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae140f6b290e

I added a patch that enables the broker when running in permissive mode.

Comment 19

[Julian Hector [:tedd] [:jhector]]

Thanks :jld, I saw some failures in the previous try push, so before I requested checkin-needed, I wanted to take a look at them.

Now I couldn't see anything leading to the conclusion that it is caused by the patches, so I did another push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22cbeec2ae6d

Now I got some failures again, but completely different then the ones from the test before, I can't see anything caused by my code, but just to be sure, can you give it a quick look :jld?

Comment 20

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

Yes, the failures look unrelated.  (And I've adjusted the title on bug 1169726 — it's an intermittent IndexedDB bug, but when it was first reported it tended to appear on runs where an unrelated sandboxing bug had crashed a child process; it seems to have started appearing on its own in the past few weeks.)

Comment 21

[Julian Hector [:tedd] [:jhector]]

Thanks :), I guess we are good to go then for checkin-needed.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97e94e27af52
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b6ab130caa

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/97e94e27af52
https://hg.mozilla.org/mozilla-central/rev/d8b6ab130caa