cr: use a new capability to authorize c/r

PATCH In review Serge E. Hallyn 2 participants 3 messages May 12, 2009 2 messages not in the archive Capabilities

lore

Share & tools

lore

Serge E. Hallyn

author

2009-05-12 15:07 UTC #0 lore raw

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

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/[email protected]>
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/[email protected]>

---
 checkpoint/sys.c           |    4 ++--
 include/linux/capability.h |    4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

checkpoint/sys.c +2 -2

index 255ebc1..5552478 100644
@@ -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 */

include/linux/capability.h +3 -1

index 572b5a0..a593391 100644
@@ -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)

Signature · 1 line

1.6.1

A message that is not in the archive (sent off-list or not yet received): 20090512150713.GA32064-r/[email protected]

Alexey Dobriyan

2009-05-12 20:59 UTC #2 lore raw

Quoted 28 lines · Serge E. Hallyn

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

checkpoint/sys.c +2 -2

@@ -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 */

include/linux/capability.h +0 -0

index 572b5a0..a593391 100644

include/linux/capability.h struct cpu_vfs_cap_data { · line 360

Quoted 5 lines

include/linux/capability.h +1 -1

@@ -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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Serge E. Hallyn

author

2009-05-12 21:17 UTC #4 lore raw

Quoted 37 lines · Alexey Dobriyan

Quoted 28 lines · Serge E. Hallyn

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

checkpoint/sys.c +2 -2

@@ -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 */

include/linux/capability.h +0 -0

index 572b5a0..a593391 100644

include/linux/capability.h struct cpu_vfs_cap_data { · line 360

Quoted 5 lines

include/linux/capability.h +1 -1

@@ -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.

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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

-serge