[systemd-devel] [PATCH] Move apparmor code before the namespace setup

[Michael Scherer]

On Mon, Oct 27, 2014 at 11:20:53PM +0100, Lennart Poettering wrote:
> On Mon, 27.10.14 20:16, Michael Scherer (misc at zarb.org) wrote:
> 
> > On Mon, Oct 27, 2014 at 03:38:37PM +0100, Lennart Poettering wrote:
> > > On Sat, 11.10.14 21:57, misc at zarb.org (misc at zarb.org) wrote:
> > > 
> > > > From: Michael Scherer <misc at 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