diff mbox series

[v6,15/17] ima: Use mac_admin_ns_capable() to check corresponding capability

Message ID 20211210194736.1538863-16-stefanb@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series ima: Namespace IMA with audit support in IMA-ns | expand

Commit Message

Stefan Berger Dec. 10, 2021, 7:47 p.m. UTC 
Use mac_admin_ns_capable() to check corresponding capability to allow
read/write IMA policy without CAP_SYS_ADMIN but with CAP_MAC_ADMIN.

Signed-off-by: Denis Semakin <denis.semakin@huawei.com>
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 include/linux/capability.h      | 6 ++++++
 security/integrity/ima/ima_fs.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Serge E. Hallyn Dec. 11, 2021, 3:29 p.m. UTC | #1 
On Fri, Dec 10, 2021 at 02:47:34PM -0500, Stefan Berger wrote:
> Use mac_admin_ns_capable() to check corresponding capability to allow
> read/write IMA policy without CAP_SYS_ADMIN but with CAP_MAC_ADMIN.
> 
> Signed-off-by: Denis Semakin <denis.semakin@huawei.com>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  include/linux/capability.h      | 6 ++++++
>  security/integrity/ima/ima_fs.c | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index 65efb74c3585..991579178f32 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -270,6 +270,12 @@ static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns)
>  		ns_capable(ns, CAP_SYS_ADMIN);
>  }
>  
> +static inline bool mac_admin_ns_capable(struct user_namespace *ns)
> +{
> +	return ns_capable(ns, CAP_MAC_ADMIN) ||
> +		ns_capable(ns, CAP_SYS_ADMIN);
> +}
> +
>  /* audit system wants to get cap info from files as well */
>  int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
>  			   const struct dentry *dentry,
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index a136d14f29ec..090ee85bfa3a 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -440,7 +440,7 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
>  #else
>  		if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
>  			return -EACCES;
> -		if (!capable(CAP_SYS_ADMIN))
> +		if (!mac_admin_ns_capable(user_ns))

Sorry if I'm missing something.  But I'm looking at your tree's
version of ima_update_policy() and failing to see where it adds
extra capability checks.  Note that any unprivileged user can
unshare a user namespace, map its hostuid to nsuid 0, and pass
ns_capable(CAP_MAC_ADMIN).

Likewise, a host uid 0 process which does not have CAP_MAC_ADMIN
can create a new user namespace, map hostuid 0 to nsuid 0, and
have CAP_MAC_ADMIN against the new userns.

Somewhere you need to be checking for privilege against either
the parent ns or the init_user_ns.  I'm not seeing where that's
being done.  Can you point me to it?


>  			return -EPERM;
>  		return ima_seq_open(filp, &ima_policy_seqops);
>  #endif
> -- 
> 2.31.1
James Bottomley Dec. 11, 2021, 4:05 p.m. UTC | #2 
On Sat, 2021-12-11 at 09:29 -0600, Serge E. Hallyn wrote:
> On Fri, Dec 10, 2021 at 02:47:34PM -0500, Stefan Berger wrote:
> > Use mac_admin_ns_capable() to check corresponding capability to
> > allow
> > read/write IMA policy without CAP_SYS_ADMIN but with CAP_MAC_ADMIN.
> > 
> > Signed-off-by: Denis Semakin <denis.semakin@huawei.com>
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >  include/linux/capability.h      | 6 ++++++
> >  security/integrity/ima/ima_fs.c | 2 +-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/capability.h
> > b/include/linux/capability.h
> > index 65efb74c3585..991579178f32 100644
> > --- a/include/linux/capability.h
> > +++ b/include/linux/capability.h
> > @@ -270,6 +270,12 @@ static inline bool
> > checkpoint_restore_ns_capable(struct user_namespace *ns)
> >  		ns_capable(ns, CAP_SYS_ADMIN);
> >  }
> >  
> > +static inline bool mac_admin_ns_capable(struct user_namespace *ns)
> > +{
> > +	return ns_capable(ns, CAP_MAC_ADMIN) ||
> > +		ns_capable(ns, CAP_SYS_ADMIN);
> > +}
> > +
> >  /* audit system wants to get cap info from files as well */
> >  int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
> >  			   const struct dentry *dentry,
> > diff --git a/security/integrity/ima/ima_fs.c
> > b/security/integrity/ima/ima_fs.c
> > index a136d14f29ec..090ee85bfa3a 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -440,7 +440,7 @@ static int ima_open_policy(struct inode *inode,
> > struct file *filp)
> >  #else
> >  		if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
> >  			return -EACCES;
> > -		if (!capable(CAP_SYS_ADMIN))
> > +		if (!mac_admin_ns_capable(user_ns))
> 
> Sorry if I'm missing something.  But I'm looking at your tree's
> version of ima_update_policy() and failing to see where it adds
> extra capability checks.  Note that any unprivileged user can
> unshare a user namespace, map its hostuid to nsuid 0, and pass
> ns_capable(CAP_MAC_ADMIN).
> 
> Likewise, a host uid 0 process which does not have CAP_MAC_ADMIN
> can create a new user namespace, map hostuid 0 to nsuid 0, and
> have CAP_MAC_ADMIN against the new userns.
> 
> Somewhere you need to be checking for privilege against either
> the parent ns or the init_user_ns.  I'm not seeing where that's
> being done.  Can you point me to it?

I think you're thinking of my patches, which used a single ima log for
everything.  With Stefan's patches, each namespace gets its own log and
starts out with and empty "do nothing" policy.  However, whatever
policy was active in the parent namespace is still run for activities
in  the child, so any activity the parent requires to be
measured/audited/appraised etc still happens, but all entries are made
in the parent.

James
Serge E. Hallyn Dec. 11, 2021, 7:22 p.m. UTC | #3 
On Sat, Dec 11, 2021 at 11:05:10AM -0500, James Bottomley wrote:
> On Sat, 2021-12-11 at 09:29 -0600, Serge E. Hallyn wrote:
> > On Fri, Dec 10, 2021 at 02:47:34PM -0500, Stefan Berger wrote:
> > > Use mac_admin_ns_capable() to check corresponding capability to
> > > allow
> > > read/write IMA policy without CAP_SYS_ADMIN but with CAP_MAC_ADMIN.
> > > 
> > > Signed-off-by: Denis Semakin <denis.semakin@huawei.com>
> > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > ---
> > >  include/linux/capability.h      | 6 ++++++
> > >  security/integrity/ima/ima_fs.c | 2 +-
> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/capability.h
> > > b/include/linux/capability.h
> > > index 65efb74c3585..991579178f32 100644
> > > --- a/include/linux/capability.h
> > > +++ b/include/linux/capability.h
> > > @@ -270,6 +270,12 @@ static inline bool
> > > checkpoint_restore_ns_capable(struct user_namespace *ns)
> > >  		ns_capable(ns, CAP_SYS_ADMIN);
> > >  }
> > >  
> > > +static inline bool mac_admin_ns_capable(struct user_namespace *ns)
> > > +{
> > > +	return ns_capable(ns, CAP_MAC_ADMIN) ||
> > > +		ns_capable(ns, CAP_SYS_ADMIN);
> > > +}
> > > +
> > >  /* audit system wants to get cap info from files as well */
> > >  int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
> > >  			   const struct dentry *dentry,
> > > diff --git a/security/integrity/ima/ima_fs.c
> > > b/security/integrity/ima/ima_fs.c
> > > index a136d14f29ec..090ee85bfa3a 100644
> > > --- a/security/integrity/ima/ima_fs.c
> > > +++ b/security/integrity/ima/ima_fs.c
> > > @@ -440,7 +440,7 @@ static int ima_open_policy(struct inode *inode,
> > > struct file *filp)
> > >  #else
> > >  		if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
> > >  			return -EACCES;
> > > -		if (!capable(CAP_SYS_ADMIN))
> > > +		if (!mac_admin_ns_capable(user_ns))
> > 
> > Sorry if I'm missing something.  But I'm looking at your tree's
> > version of ima_update_policy() and failing to see where it adds
> > extra capability checks.  Note that any unprivileged user can
> > unshare a user namespace, map its hostuid to nsuid 0, and pass
> > ns_capable(CAP_MAC_ADMIN).
> > 
> > Likewise, a host uid 0 process which does not have CAP_MAC_ADMIN
> > can create a new user namespace, map hostuid 0 to nsuid 0, and
> > have CAP_MAC_ADMIN against the new userns.
> > 
> > Somewhere you need to be checking for privilege against either
> > the parent ns or the init_user_ns.  I'm not seeing where that's
> > being done.  Can you point me to it?
> 
> I think you're thinking of my patches, which used a single ima log for
> everything.  With Stefan's patches, each namespace gets its own log and
> starts out with and empty "do nothing" policy.  However, whatever
> policy was active in the parent namespace is still run for activities
> in  the child, so any activity the parent requires to be
> measured/audited/appraised etc still happens, but all entries are made
> in the parent.

Oh.  Stefan had previously said that one motivation was to do less logging
in the hostns, so I assumed that a child ns policy could make it so the
parent ns policy was no longer applied.  If that's not the case, and not
planned, then that makes this a lot simpler.
James Bottomley Dec. 11, 2021, 8:12 p.m. UTC | #4 
On Sat, 2021-12-11 at 13:22 -0600, Serge E. Hallyn wrote:
> On Sat, Dec 11, 2021 at 11:05:10AM -0500, James Bottomley wrote:
> > On Sat, 2021-12-11 at 09:29 -0600, Serge E. Hallyn wrote:
> > > On Fri, Dec 10, 2021 at 02:47:34PM -0500, Stefan Berger wrote:
> > > > Use mac_admin_ns_capable() to check corresponding capability to
> > > > allow
> > > > read/write IMA policy without CAP_SYS_ADMIN but with
> > > > CAP_MAC_ADMIN.
> > > > 
> > > > Signed-off-by: Denis Semakin <denis.semakin@huawei.com>
> > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > ---
> > > >  include/linux/capability.h      | 6 ++++++
> > > >  security/integrity/ima/ima_fs.c | 2 +-
> > > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/linux/capability.h
> > > > b/include/linux/capability.h
> > > > index 65efb74c3585..991579178f32 100644
> > > > --- a/include/linux/capability.h
> > > > +++ b/include/linux/capability.h
> > > > @@ -270,6 +270,12 @@ static inline bool
> > > > checkpoint_restore_ns_capable(struct user_namespace *ns)
> > > >  		ns_capable(ns, CAP_SYS_ADMIN);
> > > >  }
> > > >  
> > > > +static inline bool mac_admin_ns_capable(struct user_namespace
> > > > *ns)
> > > > +{
> > > > +	return ns_capable(ns, CAP_MAC_ADMIN) ||
> > > > +		ns_capable(ns, CAP_SYS_ADMIN);
> > > > +}
> > > > +
> > > >  /* audit system wants to get cap info from files as well */
> > > >  int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
> > > >  			   const struct dentry *dentry,
> > > > diff --git a/security/integrity/ima/ima_fs.c
> > > > b/security/integrity/ima/ima_fs.c
> > > > index a136d14f29ec..090ee85bfa3a 100644
> > > > --- a/security/integrity/ima/ima_fs.c
> > > > +++ b/security/integrity/ima/ima_fs.c
> > > > @@ -440,7 +440,7 @@ static int ima_open_policy(struct inode
> > > > *inode,
> > > > struct file *filp)
> > > >  #else
> > > >  		if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
> > > >  			return -EACCES;
> > > > -		if (!capable(CAP_SYS_ADMIN))
> > > > +		if (!mac_admin_ns_capable(user_ns))
> > > 
> > > Sorry if I'm missing something.  But I'm looking at your tree's
> > > version of ima_update_policy() and failing to see where it adds
> > > extra capability checks.  Note that any unprivileged user can
> > > unshare a user namespace, map its hostuid to nsuid 0, and pass
> > > ns_capable(CAP_MAC_ADMIN).
> > > 
> > > Likewise, a host uid 0 process which does not have CAP_MAC_ADMIN
> > > can create a new user namespace, map hostuid 0 to nsuid 0, and
> > > have CAP_MAC_ADMIN against the new userns.
> > > 
> > > Somewhere you need to be checking for privilege against either
> > > the parent ns or the init_user_ns.  I'm not seeing where that's
> > > being done.  Can you point me to it?
> > 
> > I think you're thinking of my patches, which used a single ima log
> > for everything.  With Stefan's patches, each namespace gets its own
> > log and starts out with and empty "do nothing" policy.  However,
> > whatever policy was active in the parent namespace is still run for
> > activities in  the child, so any activity the parent requires to be
> > measured/audited/appraised etc still happens, but all entries are
> > made in the parent.
> 
> Oh.  Stefan had previously said that one motivation was to do less
> logging in the hostns, so I assumed that a child ns policy could make
> it so the parent ns policy was no longer applied.

Well, the default policy even for the init_user_ns is don't log
anything, so you can keep that if you don't care.  However, if you do
care you can log all the containers.  If the containers install a
logging policy, that log will disappear when the namespace is reaped,
so if the hostns has a no log policy, all the log space the child used
will be freed.

James


>   If that's not the case, and not planned, then that makes this a lot
> simpler.
>
diff mbox series

Patch

diff --git a/include/linux/capability.h b/include/linux/capability.h
index 65efb74c3585..991579178f32 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -270,6 +270,12 @@  static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns)
 		ns_capable(ns, CAP_SYS_ADMIN);
 }
 
+static inline bool mac_admin_ns_capable(struct user_namespace *ns)
+{
+	return ns_capable(ns, CAP_MAC_ADMIN) ||
+		ns_capable(ns, CAP_SYS_ADMIN);
+}
+
 /* audit system wants to get cap info from files as well */
 int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
 			   const struct dentry *dentry,
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index a136d14f29ec..090ee85bfa3a 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -440,7 +440,7 @@  static int ima_open_policy(struct inode *inode, struct file *filp)
 #else
 		if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
 			return -EACCES;
-		if (!capable(CAP_SYS_ADMIN))
+		if (!mac_admin_ns_capable(user_ns))
 			return -EPERM;
 		return ima_seq_open(filp, &ima_policy_seqops);
 #endif