[PATCH 1/1] cr: use a new capability to authorize c/r

[PATCH 1/1] cr: use a new capability to authorize c/r

[Serge E. Hallyn]

Alexey,

do you object to this patch?  The idea is to not give away any
privilege not otherwise needed.

thanks,
-serge

From b929e0b1c1a27e328ad04e8e651ddd5ec0496ccc Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date: Tue, 12 May 2009 10:53:14 -0400
Subject: [PATCH 1/1] cr: use a new capability to authorize c/r

We're currently using CAP_SYS_ADMIN to allow sys_checkpoint() and
sys_restart().  It makes no sense to lump it in with all the other
things CAP_SYS_ADMIN implies.

Define CAP_CHECKPOINT_RESTART and use that instead.

Note that CAP_SYS_ADMIN will still be necessary for unsharing
namespaces, CAP_SETUID  and CAP_SETGID for setting ids, etc.

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/sys.c           |    4 ++--
 include/linux/capability.h |    4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/checkpoint/sys.c b/checkpoint/sys.c
index 255ebc1..5552478 100644
--- a/checkpoint/sys.c
+++ b/checkpoint/sys.c
@@ -281,7 +281,7 @@ asmlinkage long sys_checkpoint(pid_t pid, int fd, unsigned long flags)
 	if (flags & ~CKPT_USER_FLAGS)
 		return -EINVAL;
 
-	if (!ckpt_unpriv_allowed && !capable(CAP_SYS_ADMIN))
+	if (!ckpt_unpriv_allowed && !capable(CAP_CHECKPOINT_RESTART))
 		return -EPERM;
 
 	if (pid == 0)
@@ -318,7 +318,7 @@ asmlinkage long sys_restart(int crid, int fd, unsigned long flags)
 	if (flags)
 		return -EINVAL;
 
-	if (!ckpt_unpriv_allowed && !capable(CAP_SYS_ADMIN))
+	if (!ckpt_unpriv_allowed && !capable(CAP_CHECKPOINT_RESTART))
 		return -EPERM;
 
 	/* FIXME: for now, we use 'crid' as a pid */
diff --git a/include/linux/capability.h b/include/linux/capability.h
index 572b5a0..a593391 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -357,7 +357,9 @@ struct cpu_vfs_cap_data {
 
 #define CAP_MAC_ADMIN        33
 
-#define CAP_LAST_CAP         CAP_MAC_ADMIN
+#define CAP_CHECKPOINT_RESTART      34
+
+#define CAP_LAST_CAP         CAP_CHECKPOINT_RESTART
 
 #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
 
-- 
1.6.1

Re: [PATCH 1/1] cr: use a new capability to authorize c/r

[Alexey Dobriyan]

On Tue, May 12, 2009 at 10:07:13AM -0500, Serge E. Hallyn wrote:
> do you object to this patch?  The idea is to not give away any
> privilege not otherwise needed.

> --- a/checkpoint/sys.c
> +++ b/checkpoint/sys.c
> @@ -281,7 +281,7 @@ asmlinkage long sys_checkpoint(pid_t pid, int fd, unsigned long flags)
>  	if (flags & ~CKPT_USER_FLAGS)
>  		return -EINVAL;
>  
> -	if (!ckpt_unpriv_allowed && !capable(CAP_SYS_ADMIN))
> +	if (!ckpt_unpriv_allowed && !capable(CAP_CHECKPOINT_RESTART))
>  		return -EPERM;
>  
>  	if (pid == 0)
> @@ -318,7 +318,7 @@ asmlinkage long sys_restart(int crid, int fd, unsigned long flags)
>  	if (flags)
>  		return -EINVAL;
>  
> -	if (!ckpt_unpriv_allowed && !capable(CAP_SYS_ADMIN))
> +	if (!ckpt_unpriv_allowed && !capable(CAP_CHECKPOINT_RESTART))
>  		return -EPERM;
>  
>  	/* FIXME: for now, we use 'crid' as a pid */
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index 572b5a0..a593391 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -357,7 +357,9 @@ struct cpu_vfs_cap_data {
>  
>  #define CAP_MAC_ADMIN        33
>  
> -#define CAP_LAST_CAP         CAP_MAC_ADMIN
> +#define CAP_CHECKPOINT_RESTART      34

I don't know if this is really needed.

If you allow restart(2) for everyone, you can during struct cred
restoration check if, say, capabilities coming from image are more
strict than capabilities of restorer, that aux groups are a subset of
aux groups of restorer and so on.

You still need these checks, otherwise CAP_CHECKPOINT_RESTART is much
more powerful than it suggests.

I'm going to try and see how hard will it be.

Re: [PATCH 1/1] cr: use a new capability to authorize c/r

[Serge E. Hallyn]

Quoting Alexey Dobriyan (adobriyan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org):
> On Tue, May 12, 2009 at 10:07:13AM -0500, Serge E. Hallyn wrote:
> > do you object to this patch?  The idea is to not give away any
> > privilege not otherwise needed.
> 
> > --- a/checkpoint/sys.c
> > +++ b/checkpoint/sys.c
> > @@ -281,7 +281,7 @@ asmlinkage long sys_checkpoint(pid_t pid, int fd, unsigned long flags)
> >  	if (flags & ~CKPT_USER_FLAGS)
> >  		return -EINVAL;
> >  
> > -	if (!ckpt_unpriv_allowed && !capable(CAP_SYS_ADMIN))
> > +	if (!ckpt_unpriv_allowed && !capable(CAP_CHECKPOINT_RESTART))
> >  		return -EPERM;
> >  
> >  	if (pid == 0)
> > @@ -318,7 +318,7 @@ asmlinkage long sys_restart(int crid, int fd, unsigned long flags)
> >  	if (flags)
> >  		return -EINVAL;
> >  
> > -	if (!ckpt_unpriv_allowed && !capable(CAP_SYS_ADMIN))
> > +	if (!ckpt_unpriv_allowed && !capable(CAP_CHECKPOINT_RESTART))
> >  		return -EPERM;
> >  
> >  	/* FIXME: for now, we use 'crid' as a pid */
> > diff --git a/include/linux/capability.h b/include/linux/capability.h
> > index 572b5a0..a593391 100644
> > --- a/include/linux/capability.h
> > +++ b/include/linux/capability.h
> > @@ -357,7 +357,9 @@ struct cpu_vfs_cap_data {
> >  
> >  #define CAP_MAC_ADMIN        33
> >  
> > -#define CAP_LAST_CAP         CAP_MAC_ADMIN
> > +#define CAP_CHECKPOINT_RESTART      34
> 
> I don't know if this is really needed.
> 
> If you allow restart(2) for everyone, you can during struct cred
> restoration check if, say, capabilities coming from image are more
> strict than capabilities of restorer, that aux groups are a subset of
> aux groups of restorer and so on.

Yes, I do all of that in the task credentials restore patchset I sent
yesterday.

> You still need these checks, otherwise CAP_CHECKPOINT_RESTART is much
> more powerful than it suggests.

Absolutely.  This patch I sent here is for Oren's checkpoint/restart
tree, which already makes sure (well aims to make sure) that at every
step of the way the task doing sys_restart() has the appopriate
privilege.

> I'm going to try and see how hard will it be.

-serge