Re: [systemd-devel] [PATCH] Move apparmor code before the namespace setup
On Mon, Oct 27, 2014 at 11:20:53PM +0100, Lennart Poettering wrote: On Mon, 27.10.14 20:16, Michael Scherer (m...@zarb.org) wrote: On Mon, Oct 27, 2014 at 03:38:37PM +0100, Lennart Poettering wrote: On Sat, 11.10.14 21:57, m...@zarb.org (m...@zarb.org) wrote: From: Michael Scherer m...@zarb.org Since apparmor need to access /proc to communicate with the kernel, any unit setting / as readonly will be unable to also use the AppArmorProfile setting, as found on debian bug 760526. A unit that sets /proc to read-only is broken anyway, I don't think we should work around that. or am I missing something here? When a unit set / as readonly, /proc seems to become readonly too. Yes, it ReadOnlyDirectories= is recursive. People doing that should use ReadWriteDirectories=/proc to open up /proc again. Note that ReadOnlyDirectories= and ReadWriteDirectories= are low-level functionality. If you use it you really should know what you do. This is different from ProtectSystem= which is a lot more high-level and doesn't require you to think about all the details. Of course, but that do not seems a reason to be forced to have a workaround in every unit doing that. And I would count setting /proc as readonly ( or unreadable ) as a hardening measure to reduce the attack surface. Well, people can do whatever they want, but write access to /proc is part of the Linux API, there's ton of functionality that processes need access to that is only available via writes to /proc. You cannot really take this away, except for trivial programs. systemd is really not the place to push for read-only /proc/self/... The APIs in /proc are generally useful APIs, you cannot just declare them unnecessary, take them away and assume things to still work. They are useful, but in the context of the original bug report on Debian, the goal is to secure tor and reduce potential information leaks on a explictely hardened distribution ( tails ) whose aim is to increase privacy. So that would be a explicit decision of the downstream to restrict it using systemd. If that's not done with systemd, that would be with selinux/apparmor anyway, but it is better to have a defense in depth, in case of a apparmor policy oversight or anything similar. So in order to make it maintainable and secure, the easiest way is to start by restricting everything, and then whitelisting, like we do for firewalling and selinux policy. No one want to assume things will just work, but on the other hand, if we can make it just work at the systemd level, that's IMHO better. So I do not really understand your concern. If the concern is that fixing the bug do not change anything because this is broken anyway, this is something that will be fixed with finer grained whitelisting and/or fixed in the daemon if possible. While not all daemons will work, far from it, I am quite sure some will without any trouble. On the patch itself, I do not really see a problem : - it doesn't change anything besides the location of the code coming from a patch I submitted 9 months ago. It would surely have been accepted if I did it right away. So I do not see any increased maintainance nor migration headaches. - it solve a corner case, which is not documented, nor really expected, and hard to debug to a less expert developper. So if the problem is that the reason of the patch to be merged aren't sound, I see: - there is a demand for it ( cf bug ) - if the patch is not merged, that mean that we will : - have to had 1 work around in the unit ( as said in the initial bug already ) - still restrict it dpwnstream with apparmor - have apparmor policy to do the restriction anyway. I think we both prefer to favor having the right fix at the right place rather than a work around everywhere, and I think that patch is that. -- Michael Scherer ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Move apparmor code before the namespace setup
On Sat, 11.10.14 21:57, m...@zarb.org (m...@zarb.org) wrote: From: Michael Scherer m...@zarb.org Since apparmor need to access /proc to communicate with the kernel, any unit setting / as readonly will be unable to also use the AppArmorProfile setting, as found on debian bug 760526. A unit that sets /proc to read-only is broken anyway, I don't think we should work around that. or am I missing something here? If you apply the apparmor profile before setting up the namespace stuff you need to whitelist all the namespace operations in the apparmor profile. That might be quite a lot, hence: is this really desirable? Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Move apparmor code before the namespace setup
On Mon, Oct 27, 2014 at 03:38:37PM +0100, Lennart Poettering wrote: On Sat, 11.10.14 21:57, m...@zarb.org (m...@zarb.org) wrote: From: Michael Scherer m...@zarb.org Since apparmor need to access /proc to communicate with the kernel, any unit setting / as readonly will be unable to also use the AppArmorProfile setting, as found on debian bug 760526. A unit that sets /proc to read-only is broken anyway, I don't think we should work around that. or am I missing something here? When a unit set / as readonly, /proc seems to become readonly too. And I would count setting /proc as readonly ( or unreadable ) as a hardening measure to reduce the attack surface. For example : CVE-2012-0056 local root exploit, requires to open /proc/$PARENT/mem to work http://git.zx2c4.com/CVE-2012-0056/tree/mempodipper.c CVE-2011-2495 /proc/ information leak https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2011-2495 CVE-2011-1593 local root exploit, requires to read /proc to be exploited http://xorl.wordpress.com/2011/04/25/cve-2011-1593-linux-kernel-proc-next_pidmap-invalid-memory-access/ ) CVE-2011-1020 race condition on /proc permitting DOS https://access.redhat.com/security/cve/CVE-2011-1020 I also wonder how it would be broken, since /proc access is very restricted inside openshift due to selinux and most if not all softwares work fine here. If you apply the apparmor profile before setting up the namespace stuff you need to whitelist all the namespace operations in the apparmor profile. That might be quite a lot, hence: is this really desirable? Apparmor profile is applied on next exec with aa_change_onexec, so we are not restricted until we switch to the daemon, no need to whitelist anything. ( unless we start to use system/fork/exec in the exec_child function but I think we want to avoid that ). -- Michael Scherer ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Move apparmor code before the namespace setup
On Mon, 27.10.14 20:16, Michael Scherer (m...@zarb.org) wrote: On Mon, Oct 27, 2014 at 03:38:37PM +0100, Lennart Poettering wrote: On Sat, 11.10.14 21:57, m...@zarb.org (m...@zarb.org) wrote: From: Michael Scherer m...@zarb.org Since apparmor need to access /proc to communicate with the kernel, any unit setting / as readonly will be unable to also use the AppArmorProfile setting, as found on debian bug 760526. A unit that sets /proc to read-only is broken anyway, I don't think we should work around that. or am I missing something here? When a unit set / as readonly, /proc seems to become readonly too. Yes, it ReadOnlyDirectories= is recursive. People doing that should use ReadWriteDirectories=/proc to open up /proc again. Note that ReadOnlyDirectories= and ReadWriteDirectories= are low-level functionality. If you use it you really should know what you do. This is different from ProtectSystem= which is a lot more high-level and doesn't require you to think about all the details. And I would count setting /proc as readonly ( or unreadable ) as a hardening measure to reduce the attack surface. Well, people can do whatever they want, but write access to /proc is part of the Linux API, there's ton of functionality that processes need access to that is only available via writes to /proc. You cannot really take this away, except for trivial programs. systemd is really not the place to push for read-only /proc/self/... The APIs in /proc are generally useful APIs, you cannot just declare them unnecessary, take them away and assume things to still work. Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Move apparmor code before the namespace setup
Am 27.10.2014 um 23:20 schrieb Lennart Poettering: On Mon, 27.10.14 20:16, Michael Scherer (m...@zarb.org) wrote: On Mon, Oct 27, 2014 at 03:38:37PM +0100, Lennart Poettering wrote: On Sat, 11.10.14 21:57, m...@zarb.org (m...@zarb.org) wrote: From: Michael Scherer m...@zarb.org Since apparmor need to access /proc to communicate with the kernel, any unit setting / as readonly will be unable to also use the AppArmorProfile setting, as found on debian bug 760526. A unit that sets /proc to read-only is broken anyway, I don't think we should work around that. or am I missing something here? When a unit set / as readonly, /proc seems to become readonly too. Yes, it ReadOnlyDirectories= is recursive. People doing that should use ReadWriteDirectories=/proc to open up /proc again. Note that ReadOnlyDirectories= and ReadWriteDirectories= are low-level functionality. If you use it you really should know what you do. This is different from ProtectSystem= which is a lot more high-level and doesn't require you to think about all the details. And I would count setting /proc as readonly ( or unreadable ) as a hardening measure to reduce the attack surface. Well, people can do whatever they want, but write access to /proc is part of the Linux API, there's ton of functionality that processes need access to that is only available via writes to /proc. You cannot really take this away, except for trivial programs. systemd is really not the place to push for read-only /proc/self/... The APIs in /proc are generally useful APIs, you cannot just declare them unnecessary, take them away and assume things to still work in fact you can even set InaccessibleDirectories=/proc InaccessibleDirectories=/sys and httpd, trafficserver, dovecot, postfix, spamassassin, clamd and what not else just works without any single issue signature.asc Description: OpenPGP digital signature ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] Move apparmor code before the namespace setup
From: Michael Scherer m...@zarb.org
Since apparmor need to access /proc to communicate with the kernel,
any unit setting / as readonly will be unable to also use the
AppArmorProfile setting, as found on debian bug 760526.
---
src/core/execute.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/core/execute.c b/src/core/execute.c
index b165b33..1f2da74 100644
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -1501,6 +1501,16 @@ static int exec_child(ExecCommand *command,
}
#endif
+#ifdef HAVE_APPARMOR
+if (params-apply_permissions context-apparmor_profile
use_apparmor()) {
+err = aa_change_onexec(context-apparmor_profile);
+if (err 0 !context-apparmor_profile_ignore) {
+*error = EXIT_APPARMOR_PROFILE;
+return -errno;
+}
+}
+#endif
+
if (context-private_network runtime
runtime-netns_storage_socket[0] = 0) {
err = setup_netns(runtime-netns_storage_socket);
if (err 0) {
@@ -1693,15 +1703,6 @@ static int exec_child(ExecCommand *command,
}
#endif
-#ifdef HAVE_APPARMOR
-if (context-apparmor_profile use_apparmor()) {
-err = aa_change_onexec(context-apparmor_profile);
-if (err 0 !context-apparmor_profile_ignore) {
-*error = EXIT_APPARMOR_PROFILE;
-return -errno;
-}
-}
-#endif
}
err = build_environment(context, n_fds, params-watchdog_usec, home,
username, shell, our_env);
--
1.8.3.1
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
