dbus-broker icon indicating copy to clipboard operation
dbus-broker copied to clipboard

Apparmor support

Open sre opened this issue 2 years ago • 11 comments

Hi,

This adds support for AppArmor using libapparmor and the Ubuntu kernel patch requested in #169. For the implementation dbus-daemon has been used as reference. The patchset handles restriction of all common DBus operations (sending, receiving, bus owning and monitoring) - just like dbus-daemon.

Note, that the downstream kernel patch is no longer limited to Ubuntu. It has been applied to some kernels used in the embedded sector. Missing support for AppArmor is the limiting factor to switch from dbus-daemon to dbus-broker on these embedded systems and the reason I wrote this code.

Regarding the status of the kernel patch required for this support: The main reason, that the kernel is still missing support for kernel based af_unix/dbus meditation is a pending code restructuring that got postponed for multiple years. Current expectation is, that this restructuring finally happens in the 5.19 cycle (fingers crossed).

-- Sebastian

sre avatar Mar 18 '22 18:03 sre

Thanks for having a look at it. I will check the things you pointed out next week and also try to figure out a way to split the second patch into smaller chunks.

sre avatar May 06 '22 15:05 sre

@sre Is there a link to the patch review on lore for the revised patch slated for 5.19 yet?

Conan-Kudo avatar May 10 '22 12:05 Conan-Kudo

@sre Is there a link to the patch review on lore for the revised patch slated for 5.19 yet?

No, not that I'm aware of.

sre avatar May 11 '22 17:05 sre

So I pulled in most of the preparatory patches, and I adjusted some bits. The major apparmor.c code is still missing. I am working on it.

Thanks, the changes look fine to me.

Let me know if something is unclear, or if I unintentionally broke something.

I just rebased the pull-request to current HEAD dropping the merged patches (no other changes) and the branch is still working. I will look into the CI issues and the bus_type problem.

sre avatar May 17 '22 17:05 sre

I am quite confused now where you got the transmission checks from. dbus-daemon simply checks with the destination-string and source-unique-id, but your code iterates the set of names on the destination. This will very easily break existing AppArmor rules. I also don't understand why that change was done. Is there any reason not to imitate what dbus-daemon does?

That is, I would make apparmor check whether the sender is allowed to send to the destination used in the message. Then check whether the receiver is allowed to receive from the unique-id of the sender. If either is false, refuse the transmission.

I can implement this, but I want to make sure you agree on that, and that this is indeed the intended behavior?

dvdhrm avatar Jul 13 '22 07:07 dvdhrm

From an AppArmor perspective, this LGTM. We updated our regression tests upstream and they are passing with these patches.

gegarcia avatar Mar 09 '23 13:03 gegarcia

As far as I can tell the CI is currently unaware of AppArmor in the sense that -Dapparmor=true isn't passed to meson anywhere. At least CodeQL analyzes the apparmor fallback only as far as I can see because libapparmor-dev isn't installed there and the "autobuild" job can't figure out what to do. The other CI workflow passes -Daudit=true only. There it would probably make sense to build dbus-broker with both -Dapparmor=true and -Dapparmor=false to make sure it compiles fine with -Werror with and without libapparmor.

(I'm not sure it should be fixed here but I think it should be fixed one way or another).

evverx avatar Jun 16 '23 15:06 evverx

looks like arch is adopting dbus-broker as default. any chance this can be merged?

andrewfader avatar Jan 11 '24 07:01 andrewfader

Arch kernels don't have apparmor dbus mediation patches so included so this is irrelevant.

Maryse47 avatar Jan 13 '24 16:01 Maryse47

Arch kernels don't have apparmor dbus mediation patches so included so this is irrelevant.

I don't see how that can be, but I'm not an expert. Per https://wiki.archlinux.org/title/AppArmor I have it enabled and running, a service loaded, a kernel option enabled, aa-enabled returns true, I see "DENIED" in my logs sometimes, etc.

andrewfader avatar Jan 13 '24 17:01 andrewfader

The required AppArmor features to get D-Bus message security policies working are not part of upstream linux kernels. Basic AppArmor support has been included in upstream kernels, though.

So to get AppArmor working with dbus-broker, you would definitely need downstream patches. Ubuntu applies them to all their kernel builds.

dvdhrm avatar Jan 15 '24 15:01 dvdhrm