bus1 / dbus-broker

Linux D-Bus Message Broker
https://github.com/bus1/dbus-broker/wiki
Apache License 2.0
685 stars 84 forks source link

AppArmor support #169

Open bitstreamout opened 6 years ago

bitstreamout commented 6 years ago

I see this here in the journal

 dbus-broker-launch[1035]: AppArmor enabled, but not supported. Ignoring.
dvdhrm commented 6 years ago

The AppArmor code is, to a large extent, a downstream patchset carried by Ubuntu in their respective packages. The upstream linux kernel does not have the required AppArmor features to implement AppArmor-support in dbus-broker. In particular, SO_PEERSEC is not sufficiently implemented.

dbus-daemon(1) carries patches against the downstream AppArmor code of Ubuntu. We currently are a bit reluctant to add code that relies on downstream kernel features. However, we did adjust the dbus-broker infrastructure to easily allow downstream patches that add AppArmor support. Meaning, we already parse all the AppArmor related configuration and can easily insert the required AppArmor hooks.

bitstreamout commented 6 years ago

Hmm ... the apparmor patch set is part of our kernel here. Otherwise I would not see this messages I guess. I found https://lkml.org/lkml/2018/5/4/476 ... now let's see if your patchset will reach the kernel

dvdhrm commented 6 years ago

Basic Apparmor support is upstream in the linux kernel. If you now enable apparmor support in your dbus configuration, you will see the message you mentioned. The are two modes to run dbus-AppArmor in: enforcing or informative. Your configuration apparently uses the latter, hence AppArmor is ignored by dbus-broker, as it is informative only. If you selected the enforcing mode, dbus-broker would refuse to start (since it lacks apparmor support).

If anyone wants enforcing behavior of AppArmor in dbus-broker, they have to patch it into dbus-broker manually, or upstream the AppArmor patches so we can work on it.

The patchset you linked to is not related to this issue. It was already merged upstream and is about SO_PEERSEC support on socketpairs.

dvdhrm commented 5 years ago

For documentational purposes:

The downstream Ubuntu patch that currently provides the required kernel functionality is:

https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/cosmic/commit/security/apparmor?id=2d33c63704a49b005dfa4597882be265ce0bdb06

I saved it on github as well:

https://gist.github.com/dvdhrm/0b8c556960957970d646c9bba814692a

In particular, we need upstream kernel sources to include:

+   LSM_HOOK_INIT(unix_stream_connect, apparmor_unix_stream_connect),
+   LSM_HOOK_INIT(unix_may_send, apparmor_unix_may_send),

Or something along those lines. Also, ctx->peer is currently a read-only dummy. This field needs to be written to somewhere to update AF_UNIX connection state.

I will keep this issue open for now, so we can track upstream AppArmor progress.

DemiMarie commented 3 years ago

This is a case where I wish D-Bus clients sent a /proc/self FD at connection. We already know the client’s PID, and can check that it has not been recycled by opening /proc/$PID and checking that the device and inode numbers match. If the launcher passes an FD pointing to /proc, we don’t even need access to /proc ourselves.

dvdhrm commented 3 years ago

This is a case where I wish D-Bus clients sent a /proc/self FD at connection. We already know the client’s PID, and can check that it has not been recycled by opening /proc/$PID and checking that the device and inode numbers match. If the launcher passes an FD pointing to /proc, we don’t even need access to /proc ourselves.

I am not sure how this is related to AppArmor support? This is a tracking issue waiting for AppArmor-patches to be upstreamed to the linux sources.

Nevertheless, I agree that an FD to /proc is a quite elegant solution. But so far the linux ABI kinda expects /proc to be mounted, so in real-world scenarios it makes little difference. This gets more interesting, when we consider open FDs on /proc/<pid> as process handles. It does definitely get rid of pid-recycle-races. However, due to it being O_PATH FDs, it would not solve permission issuey, since it would still not be a proper capability-based model.

The recent pidfd_xyz() syscalls went into the right direction, but then they decided to still perform permission checks for all operations on those FDs, thus completely subverting the idea of using process handles to "own" access to a process.

Is there a particular improvement you have in mind?

DemiMarie commented 3 years ago

This is a case where I wish D-Bus clients sent a /proc/self FD at connection. We already know the client’s PID, and can check that it has not been recycled by opening /proc/$PID and checking that the device and inode numbers match. If the launcher passes an FD pointing to /proc, we don’t even need access to /proc ourselves.

I am not sure how this is related to AppArmor support? This is a tracking issue waiting for AppArmor-patches to be upstreamed to the linux sources.

Nevertheless, I agree that an FD to /proc is a quite elegant solution. But so far the linux ABI kinda expects /proc to be mounted, so in real-world scenarios it makes little difference. This gets more interesting, when we consider open FDs on /proc/<pid> as process handles. It does definitely get rid of pid-recycle-races. However, due to it being O_PATH FDs, it would not solve permission issuey, since it would still not be a proper capability-based model.

The recent pidfd_xyz() syscalls went into the right direction, but then they decided to still perform permission checks for all operations on those FDs, thus completely subverting the idea of using process handles to "own" access to a process.

Is there a particular improvement you have in mind?

Once we have a race-free way to get the client’s /proc/$PID FD, we can obtain their AppArmor context from /proc, without using SO_PEERSEC. For this to work, a client must send a /proc/self FD, so that we can check that their PID hasn’t been reused.

dvdhrm commented 3 years ago

Once we have a race-free way to get the client’s /proc/$PID FD, we can obtain their AppArmor context from /proc, without using SO_PEERSEC. For this to work, a client must send a /proc/self FD, so that we can check that their PID hasn’t been reused.

But is the reduced AppArmor kernel module enough to provide the required level of support for the actual enforcement of the runtime policies? I was under the impression that those patches are needed to implement a meaningful policy.

Is there actual interest in AppArmor support for dbus-broker? So far, there was little interest in the Ubuntu development community to adopt dbus-broker, so we never pushed for AppArmor support.

DemiMarie commented 3 years ago

Once we have a race-free way to get the client’s /proc/$PID FD, we can obtain their AppArmor context from /proc, without using SO_PEERSEC. For this to work, a client must send a /proc/self FD, so that we can check that their PID hasn’t been reused.

But is the reduced AppArmor kernel module enough to provide the required level of support for the actual enforcement of the runtime policies? I was under the impression that those patches are needed to implement a meaningful policy.

Is there actual interest in AppArmor support for dbus-broker? So far, there was little interest in the Ubuntu development community to adopt dbus-broker, so we never pushed for AppArmor support.

I have no idea. I just know that this can be used whenever we need information from a client’s /proc.

dvdhrm commented 3 years ago

Ok!