Re: [RFC 3/3] ima: make the integrity inode cache per namespace

[Christian Brauner <christian.brauner@ubuntu.com>]

On Mon, Nov 29, 2021 at 05:16:56PM +0100, Christian Brauner wrote:
> On Mon, Nov 29, 2021 at 09:35:39AM -0600, Serge Hallyn wrote:
> > On Mon, Nov 29, 2021 at 09:46:55AM -0500, James Bottomley wrote:
> > > On Mon, 2021-11-29 at 15:22 +0100, Christian Brauner wrote:
> > > > On Mon, Nov 29, 2021 at 09:10:29AM -0500, James Bottomley wrote:
> > > > > On Mon, 2021-11-29 at 08:53 -0500, Stefan Berger wrote:
> > > > > > On 11/29/21 07:50, James Bottomley wrote:
> > > > > > > On Sun, 2021-11-28 at 22:58 -0600, Serge E. Hallyn wrote:
> > > > > > > > On Sat, Nov 27, 2021 at 04:45:49PM +0000, James Bottomley
> > > > > > > > wrote:
> > > > > > > > > Currently we get one entry in the IMA log per unique file
> > > > > > > > > event.  So, if you have a measurement policy and it
> > > > > > > > > measures a particular binary it will not get measured again
> > > > > > > > > if it is subsequently executed. For Namespaced IMA, the
> > > > > > > > > correct behaviour seems to be to log once per inode per
> > > > > > > > > namespace (so every unique execution in a namespace gets a
> > > > > > > > > separate log entry).  Since logging once per inode per
> > > > > > > > > namespace is
> > > > > > > > I suspect I'll need to do a more in depth reading of the
> > > > > > > > existing code, but I'll ask the lazy question anyway (since
> > > > > > > > you say "the correct behavior seems to be") - is it actually
> > > > > > > > important that files which were appraised under a parent
> > > > > > > > namespace's policy already should be logged again?
> > > > > > > I think so.  For a couple of reasons, assuming the namespace
> > > > > > > eventually gets its own log entries, which the next incremental
> > > > > > > patch proposed to do by virtualizing the securityfs
> > > > > > > entries.  If you don't do this:
> > > > > > 
> > > > > > To avoid duplicate efforts, an implementation of a virtualized 
> > > > > > securityfs is in this series here:
> > > > > > 
> > > > > > https://github.com/stefanberger/linux-ima-namespaces/commits/v5.15%2Bimans.20211119.v3
> > > > > > 
> > > > > > It starts with 'securityfs: Prefix global variables with
> > > > > > secruityfs_'
> > > > > 
> > > > > That's quite a big patch series.  I already actually implemented
> > > > > this as part of the RFC for getting the per namespace measurement
> > > > > log.  The attached is basically what I did.
> > > > > 
> > > > > Most of the time we don't require namespacing the actual virtualfs
> > > > > file, because it's world readable.  IMA has a special requirement
> > > > > in this regard because the IMA files should be readable (and
> > > > > writeable when we get around to policy updates) by the admin of the
> > > > > namespace but their protection is 0640 or 0440.  I thought the
> > > > > simplest solution would be to make an additional flag that coped
> > > > > with the permissions and a per-inode flag way of making the file as
> > > > > "accessible by userns admin".  Doing something simple like this
> > > > > gives a much smaller diffstat:
> > > > 
> > > > That's a NAK from me. Stefan's series might be bigger but it does
> > > > things correctly. I appreciate the keep it simple attitude but no. I
> > > > won't speciale-case securityfs or similar stuff in core vfs helpers.
> > > 
> > > Well, there's a reason it's an unpublished patch.  However, the more
> > > important point is that namespacing IMA requires discussion of certain
> > > points that we never seem to drive to a conclusion.  Using the akpm
> > > method, I propose simple patches that drive the discussion.  I think
> > > the points are:
> > > 
> > >    1. Should IMA be its own namespace or tied to the user namespace?  The
> > >       previous patches all took the separate Namespace approach, but I
> > >       think that should be reconsidered now keyrings are in the user
> > >       namespace.
> > 
> > Well that purely depends on the needed scope.
> > 
> > The audit container identifier is a neat thing.  But it absolutely must
> > be settable, so seems to conflict with your needs.
> > 
> > Your patch puts an identifier on the user_namespace.  I'm not quite sure,
> > does that satisfy Stefan's needs?  A new ima ns if and only if there is a
> > new user ns?
> 
> I kept thinking about this question while I was out running and while I
> admittedly have reacted poorly to CLONE_NEWIMA patches before it feels
> to me that this is the right approach after all. Making it part of
> userns at least in this form isn't clean.
> 
> I think attaching a uuid to a userns alone for the sake of IMA is wrong.
> Additionally, I think a uuid only for the userns is too limited. This is
> similar to the problem of the audit container id. If we have some sort
> of uuid for ima it will automatically evolve into something like a
> container id (I'm not even arguing that this is necessarily wrong.).
> We also have the issue that we then have the container audit id thing -
> if this ever lands and the ima userns uuid. All that makes it quite
> messy.
> 
> I think CLONE_NEWIMA is ultimately nicer and allows the implementation
> to be decoupled from the userns and self-contained as possible. This
> also means that ima ns works for privileged containers which sure is a
> valid use-case.
> It will also make securityfs namespacing easier as it can be a keyed
> super where the key is the ima ns (similar to how we deal with e.g.
> mqueue).

s/ima ns/userns/g

which is what Stefan already did in the link he shared.