Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

interfaces/builtin: remove the name=org.freedesktop.DBus restriction in cups-control AppArmor rules #11843

Merged
merged 1 commit into from Jun 7, 2022

Conversation

jhenstridge
Copy link
Contributor

While testing the CUPS snap, @tillkamppeter noticed that some D-Bus broadcast signals were being blocked by AppArmor.

On closer inspection, the rules related to the org.cups.cupsd.Notifier D-Bus interface all had a restriction like peer=(name=org.freedesktop.DBus,label=...). This seemed weird to me, as that name is reserved for dbus-daemon, and these messages are not being sent to or received from the bus. Removing the name= restriction allowed the messages to be delivered.

I asked @alexmurray for his opinion, and he couldn't think of a reason the name conditional was included here either, so I've submitted this PR to remove it.

In this case, there isn't a different D-Bus name that could be used in the rule. The messages are sent by a short lived notifier plugin process:

https://github.com/OpenPrinting/cups/blob/master/notifier/dbus.c

It does not try to own a name, and the clients watching for the signal broadcasts don't match on a well known bus name either.

…in cups-control AppArmor rules

This seems to have been included erroneously: the org.freedesktop.DBus
bus name is owned by the bus itself rather than any particular peer
connected to the bus. While it makes sense to use in a rule dealing with
communication with the bus, it seems to block legitimate traffic in this
case.
@mvo5 mvo5 added this to the 2.56 milestone Jun 3, 2022
@mvo5 mvo5 added the Needs security review Can only be merged once security gave a :+1: label Jun 3, 2022
Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks sensible to me

@jhenstridge
Copy link
Contributor Author

I'll also note that there are other interfaces using this peer=(name=org.freedesktop.DBus,label=...) construct for messages not to/from the bus daemon. It's possible that they aren't behaving as expected either.

Copy link
Contributor

@mardy mardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mardy
Copy link
Contributor

mardy commented Jun 3, 2022

I'll also note that there are other interfaces using this peer=(name=org.freedesktop.DBus,label=...) construct for messages not to/from the bus daemon. It's possible that they aren't behaving as expected either.

I had a quick look, and they are all either from Ubuntu Phone obsolete interfaces, or legitimate (because they are about (un)registering D-Bus names or getting the ID of the D-Bus session.

The only exceptions I've found are in the "desktop" interface:

# gmenu
dbus (send)
     bus=session
     interface=org.gtk.Actions
     member=Changed
     peer=(name=org.freedesktop.DBus, label=unconfined),

and similar ones (though not 100% equivalent) in the unity7 interface. I will propose a PR, then we can discuss on it.

Copy link
Collaborator

@alexmurray alexmurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @jhenstridge.

@alexmurray alexmurray removed the Needs security review Can only be merged once security gave a :+1: label Jun 6, 2022
@mvo5 mvo5 merged commit cafc226 into snapcore:master Jun 7, 2022
@tillkamppeter
Copy link

@jhenstridge @alexmurray @mardy @mvo5 Thanks a lot for your quick fix, verification, and merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants