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

Fix AppArmor 3.0 support (closes #3659) #3660

Merged
merged 1 commit into from
Oct 10, 2020
Merged

Conversation

kris7t
Copy link
Collaborator

@kris7t kris7t commented Oct 10, 2020

AppArmor introduces the @{run} variable, which is used in
<abstractions/dbus-strict> and <abstractions/dbus-session-strict> among
other places. Thus, we must #include <tunables/run> to be able to call
these abstractions.

Standard profiles rely on <tunables/global> to include <tunables/run>, since it exists in previous AppArmor versions, too.

As an attempt at backwards compatibility, we #include if exists instead
of #include, since there is no <tunables/run> in AppArmor 2.x.

However, if exists is a relatively new feature, see e.g.
https://phabricator.kde.org/D14526
(However, do note that if exists does not appear as a new feature in
https://gitlab.com/apparmor/apparmor/-/wikis/Release_Notes_2.13).
Therefore, this commit restricts our compatibility to relatively new
(<10 months) old AppArmor releases only.

As an alternative, we could detect the AppArmor version at configure
time, and emit a firejail-default profile based on that.

Here, I opted for the simpler approach, as distributions likely to ship >10 months old AppArmor (Debian) support Firejail on their own via backports (according to https://github.com/netblue30/firejail/security/policy), anyways.

@reinerh
Copy link
Collaborator

reinerh commented Oct 10, 2020

AppArmor introduces the @{run} variable, which is used in
<abstractions/dbus-strict> and <abstractions/dbus-session-strict> among
other places. Thus, we must #include <tunables/run> to be able to call
these abstractions.

If the abstractions are using the @{run} variable, isn't that a bug in these abstractions and would be better addressed there?
When they start using the tunable or drop the variable completely in the future, we have an unnecessary import in our profile.

As an attempt at backwards compatibility, we #include if exists instead
of #include, since there is no <tunables/run> in AppArmor 2.x.

However, if exists is a relatively new feature, see e.g.
https://phabricator.kde.org/D14526
(However, do note that if exists does not appear as a new feature in
https://gitlab.com/apparmor/apparmor/-/wikis/Release_Notes_2.13).
Therefore, this commit restricts our compatibility to relatively new
(<10 months) old AppArmor releases only.

Ubuntu 18.04 LTS still ships with 2.12. Though I could patch it when uploading to the PPA.

@kris7t
Copy link
Collaborator Author

kris7t commented Oct 10, 2020

@reinerh The two examples of <abstraction/dbus> use I could find in the built-in profiles were usr.sbin.avahi-daemon and usr.sbin.dnsmasq. Both of these include <tunables/global> before including <abstractions/dbus>, which in turn includes <tunables/run>.

So most likely <abstractions/dbus-strict> is not broken, we just have to follow convention by including <tunables/global>. Luckily, <tunables/global> is pretty old, so we don't need if exists when including it. This should clear up the backwards compatibility issue. I'll update my patch accordingly.

AppArmor introduces the @{run} variable, which is used in
<abstractions/dbus-strict> and <abstractions/dbus-session-strict> among
other places. Thus, we follow suit of the built-in profiles and #include
<tunables/global>, which includes <tunables/run> in AppArmor 3.0,
defining the variable.

As <tunables/global> exists in previous versions of AppArmor, too, this
patch does not introduce a backward-compatibility issue with Apparmor
2.x.
@kris7t kris7t merged commit 9bf6e0e into netblue30:master Oct 10, 2020
@kris7t kris7t deleted the apparmor-run branch October 10, 2020 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants