cr: lsm: restore LSM contexts for ipc objects

Here is the next version of the patch implementing checkpoint and restore of LSM contexts. This is just handling IPC objects as a proof of concept. But actually, looking ahead and both files and tasks, I see that selinux stores several sids in the security structs. For instance, for tasks there is the current sid, exec sid, create sid, keycreate_sid, and sockcreate_sid. So I guess I'll have to ask the LSM for how many secids it wants to checkpoint, then checkpoint an array of contexts?

Quoted 9 lines · Serge E. Hallyn

Here is the next version of the patch implementing checkpoint and restore of LSM contexts. This is just handling IPC objects as a proof of concept. But actually, looking ahead and both files and tasks, I see that selinux stores several sids in the security structs. For instance, for tasks there is the current sid, exec sid, create sid, keycreate_sid, and sockcreate_sid. So I guess I'll have to ask the LSM for how many secids it wants to checkpoint, then checkpoint an array of contexts?

Can you please explain exactly what checkpoint/restart is?

Here is the next version of the patch implementing checkpoint and restore of LSM contexts. This is just handling IPC objects as a proof of concept. But actually, looking ahead and both files and tasks, I see that selinux stores several sids in the security structs. For instance, for tasks there is the current sid, exec sid, create sid, keycreate_sid, and sockcreate_sid. So I guess I'll have to ask the LSM for how many secids it wants to checkpoint, then checkpoint an array of contexts?

Quoted 1 line

From 19669b07cdfef4d377f3f188e2421c4124e38708 Mon Sep 17 00:00:00 2001

From: Serge E. Hallyn <[email protected]>
Date: Wed, 17 Jun 2009 12:00:21 -0400
Subject: [PATCH 1/1] cr: lsm: restore LSM contexts for ipc objects

Introduce a cache of secids for checkpoint and restart.

At checkpoint, it takes a secid, stores the corresponding context string, and stores the objref for later use. At restart, read the context from checkpoint image, ask the security module for a secid, and store the secid on the objhash.

The per-object security c/r code will be responsible for getting secid from void*security at checkpoint time, and converting secid to void*security at restore time.

The code to c/r contexts for IPC objects is also in this patch.

For Smack, assign the label of the process doing sys_restart() if !capable(CAP_MAC_ADMIN), otherwise use the checkpointed label.

For SELinux, define a new 'restore' permission for ipc objects. (A corresponding trival policy patch adding 'restore' to the common flask permissions for refpolicy is also needed). The caller of sys_restart() must have the class:restore permission to assign the checkpointed label, else restart will be refused.

Signed-off-by Serge E. Hallyn <[email protected]>

include/linux/checkpoint_hdr.h +0 -0

index e42e0db..e3fb9b3 100644

Quoted 8 lines · Serge E. Hallyn

Here is the next version of the patch implementing checkpoint and restore of LSM contexts. This is just handling IPC objects as a proof of concept. But actually, looking ahead and both files and tasks, I see that selinux stores several sids in the security structs. For instance, for tasks there is the current sid, exec sid, create sid, keycreate_sid, and sockcreate_sid. So I guess I'll have to ask the LSM for how many secids it wants to checkpoint, then checkpoint an array of contexts?

You will need to support checkpointing multiple secids/contexts per object, but what about other state that might live in the security structs, e.g. flags fields, policy seqno, etc.

Quoted 6 lines

Quoted 1 line

From 19669b07cdfef4d377f3f188e2421c4124e38708 Mon Sep 17 00:00:00 2001

From: Serge E. Hallyn <[email protected]>
Date: Wed, 17 Jun 2009 12:00:21 -0400
Subject: [PATCH 1/1] cr: lsm: restore LSM contexts for ipc objects

Introduce a cache of secids for checkpoint and restart.

Not sure you need to cache them in the cr layer (vs. just using the mapping functions provided by the LSM hook interface, and letting the security module handle caching internally).

Quoted 24 lines

At checkpoint, it takes a secid, stores the corresponding context string, and stores the objref for later use. At restart, read the context from checkpoint image, ask the security module for a secid, and store the secid on the objhash.

The per-object security c/r code will be responsible for getting secid from void*security at checkpoint time, and converting secid to void*security at restore time.

The code to c/r contexts for IPC objects is also in this patch.

For Smack, assign the label of the process doing sys_restart() if !capable(CAP_MAC_ADMIN), otherwise use the checkpointed label.

For SELinux, define a new 'restore' permission for ipc objects. (A corresponding trival policy patch adding 'restore' to the common flask permissions for refpolicy is also needed). The caller of sys_restart() must have the class:restore permission to assign the checkpointed label, else restart will be refused.

Signed-off-by Serge E. Hallyn <[email protected]>

include/linux/checkpoint_hdr.h struct ckpt_hdr_ipc_perms { · line 429

Quoted 10 lines

include/linux/checkpoint_hdr.h +1 -1

index e42e0db..e3fb9b3 100644
@@ -418,7 +426,7 @@ struct ckpt_hdr_ipc_perms {
 	__u32 cuid;
 	__u32 cgid;
 	__u32 mode;
-	__u32 _padding;
+	__s32 secref;

Why s32 vs u32? secids are u32 and 0 means they aren't supported by the security module.

Quoted 3 lines · Stephen Smalley

Not sure you need to cache them in the cr layer (vs. just using the mapping functions provided by the LSM hook interface, and letting the security module handle caching internally).

Do I understand correctly that secids are supposed to be consistent across machines and reboots, but not across policy versions?

There are several ways we could handle this. One is to have security_checkpoint_object() spit out an arbitrary void* which describes the *full* security context, with the checkpoint/restart layer completely unaware about the meaning of the contents.

Another is to have security_checkpoint_header() output the policy version, and for each checkpointed object just write out an array of secids. If any file has a seqno which is not the same as the rest of the container's, then refuse the checkpoint?

A third, which is what I was doing here, is to write out textual context representations, and expect the LSM on _restore() to know of a single meaningful interpretation for the context_str.

Maybe adding a security_checkpoint_header() at the top of the checkpoint file would help with that (so that in the general case, if policy version at restart is different from the checkpointed one, we refuse restart, assuming that we can't meaningfully intepret even system_u:object_r:user_file_t).

Quoted 25 lines

Quoted 24 lines

At checkpoint, it takes a secid, stores the corresponding context string, and stores the objref for later use. At restart, read the context from checkpoint image, ask the security module for a secid, and store the secid on the objhash.

The per-object security c/r code will be responsible for getting secid from void*security at checkpoint time, and converting secid to void*security at restore time.

The code to c/r contexts for IPC objects is also in this patch.

For Smack, assign the label of the process doing sys_restart() if !capable(CAP_MAC_ADMIN), otherwise use the checkpointed label.

For SELinux, define a new 'restore' permission for ipc objects. (A corresponding trival policy patch adding 'restore' to the common flask permissions for refpolicy is also needed). The caller of sys_restart() must have the class:restore permission to assign the checkpointed label, else restart will be refused.

Signed-off-by Serge E. Hallyn <[email protected]>

Quoted 13 lines

include/linux/checkpoint_hdr.h struct ckpt_hdr_ipc_perms { · line 429

Quoted 10 lines

include/linux/checkpoint_hdr.h +1 -1

index e42e0db..e3fb9b3 100644
@@ -418,7 +426,7 @@ struct ckpt_hdr_ipc_perms {
 	__u32 cuid;
 	__u32 cgid;
 	__u32 mode;
-	__u32 _padding;
+	__s32 secref;

Why s32 vs u32? secids are u32 and 0 means they aren't supported by the security module.

The secref is a reference to an object in the checkpoint/restart object hashtable, not a secid. Those are signed, with <0 meaning error and 0 being a special case (in this case, 'not applicable')

If we write out the secids directly, which it sounds like you are advocating, and not the secid_to_secctx string, then we can store a u32 numsecs followed by __u32 secid[] which will have numsecs entries.

Quoted 6 lines · Serge E. Hallyn

Quoted 4 lines · Stephen Smalley

Not sure you need to cache them in the cr layer (vs. just using the mapping functions provided by the LSM hook interface, and letting the security module handle caching internally).

Quoted 3 lines

Do I understand correctly that secids are supposed to be consistent across machines and reboots, but not across policy versions?

No, secids are temporal - they are dynamically allocated at runtime like file descriptors. You should only store security contexts in the images.

ipc/checkpoint_msg.c +0 -0

index 51385b0..ca55339 100644

ipc/checkpoint_msg.c +4 -0

@@ -175,11 +183,26 @@ static int load_ipc_msg_hdr(struct ckpt_ctx *ctx,
 			    struct msg_queue *msq)
 {
 	int ret = 0;
+	int secid = 0;
 
 	ret = restore_load_ipc_perms(&h->perms, &msq->q_perm);
 	if (ret < 0)
 		return ret;
 
+	if (h->perms.secref) {
+		struct sec_store *s;
+		s = ckpt_obj_fetch(ctx, h->perms.secref, CKPT_OBJ_SECURITY);

Quoted 4 lines · Serge E. Hallyn

ipc/checkpoint_msg.c +0 -0

index 51385b0..ca55339 100644

<snip>

Quoted 21 lines

ipc/checkpoint_msg.c +12 -0

@@ -175,11 +183,26 @@ static int load_ipc_msg_hdr(struct ckpt_ctx *ctx,
 			    struct msg_queue *msq)
 {
 	int ret = 0;
+	int secid = 0;
 
 	ret = restore_load_ipc_perms(&h->perms, &msq->q_perm);
 	if (ret < 0)
 		return ret;
 
+	if (h->perms.secref) {
+		struct sec_store *s;
+		s = ckpt_obj_fetch(ctx, h->perms.secref, CKPT_OBJ_SECURITY);
+		if (IS_ERR(s))
+			return PTR_ERR(s);
+		secid = s->secid;
+	}
+	ret = security_msg_queue_alloc(msq);
+	if (ret)
+		return ret;
+	ret = security_msg_queue_restore(msq, secid);

ipc/checkpoint_msg.c static int load_ipc_msg_hdr(struct ckpt_ctx *ctx, · line 204

Quoted 2 lines

ipc/checkpoint_msg.c +2 -0

+	if (ret < 0)
+		return ret;

I don't think you want to call security_msg_queue_alloc() here, as that both allocates the security struct and performs the create check. So I would just call the _restore() function, and let it internally call ipc_alloc_security() to allocate the struct but then apply its own distinct restore check. Likewise for the rest of them.

Also, where do we get to veto attempts to checkpoint the task in the first place? If ptrace, I think we'd want it treated as a PTRACE_MODE_ATTACH (also used for /proc/pid/mem) rather than just PTRACE_MODE_READ (reading other /proc/pid info).

Quoted 27 lines · Stephen Smalley

Quoted 4 lines · Serge E. Hallyn

ipc/checkpoint_msg.c +0 -0

index 51385b0..ca55339 100644

<snip>

Quoted 21 lines

ipc/checkpoint_msg.c +12 -0

@@ -175,11 +183,26 @@ static int load_ipc_msg_hdr(struct ckpt_ctx *ctx,
 			    struct msg_queue *msq)
 {
 	int ret = 0;
+	int secid = 0;
 
 	ret = restore_load_ipc_perms(&h->perms, &msq->q_perm);
 	if (ret < 0)
 		return ret;
 
+	if (h->perms.secref) {
+		struct sec_store *s;
+		s = ckpt_obj_fetch(ctx, h->perms.secref, CKPT_OBJ_SECURITY);
+		if (IS_ERR(s))
+			return PTR_ERR(s);
+		secid = s->secid;
+	}
+	ret = security_msg_queue_alloc(msq);
+	if (ret)
+		return ret;
+	ret = security_msg_queue_restore(msq, secid);

Quoted 8 lines

ipc/checkpoint_msg.c static int load_ipc_msg_hdr(struct ckpt_ctx *ctx, · line 204

Quoted 2 lines

ipc/checkpoint_msg.c +2 -0

+	if (ret < 0)
+		return ret;

I don't think you want to call security_msg_queue_alloc() here, as that both allocates the security struct and performs the create check. So I would just call the _restore() function, and let it internally call ipc_alloc_security() to allocate the struct but then apply its own distinct restore check. Likewise for the rest of them.

Ok, will change that.

Quoted 37 lines · Serge E. Hallyn

Quoted 27 lines · Stephen Smalley

Quoted 4 lines · Serge E. Hallyn

ipc/checkpoint_msg.c +0 -0

index 51385b0..ca55339 100644

<snip>

Quoted 21 lines

ipc/checkpoint_msg.c +12 -0

@@ -175,11 +183,26 @@ static int load_ipc_msg_hdr(struct ckpt_ctx *ctx,
 			    struct msg_queue *msq)
 {
 	int ret = 0;
+	int secid = 0;
 
 	ret = restore_load_ipc_perms(&h->perms, &msq->q_perm);
 	if (ret < 0)
 		return ret;
 
+	if (h->perms.secref) {
+		struct sec_store *s;
+		s = ckpt_obj_fetch(ctx, h->perms.secref, CKPT_OBJ_SECURITY);
+		if (IS_ERR(s))
+			return PTR_ERR(s);
+		secid = s->secid;
+	}
+	ret = security_msg_queue_alloc(msq);
+	if (ret)
+		return ret;
+	ret = security_msg_queue_restore(msq, secid);

Quoted 8 lines

ipc/checkpoint_msg.c static int load_ipc_msg_hdr(struct ckpt_ctx *ctx, · line 204

Quoted 2 lines

ipc/checkpoint_msg.c +2 -0

+	if (ret < 0)
+		return ret;

I don't think you want to call security_msg_queue_alloc() here, as that both allocates the security struct and performs the create check. So I would just call the _restore() function, and let it internally call ipc_alloc_security() to allocate the struct but then apply its own distinct restore check. Likewise for the rest of them.

Quoted 1 line

Ok, will change that.

Hmm, but that means that if there is some new LSM which allocates memory in security_msg_queue_alloc(), but which does not define security_msg_queue_restore() (for some stupid reason), it'll end up causing a bug.

It's something we can certainly catch through code review, but do we want to set such a scenario up at all?

Speaking just for SELinux, the security_msg_queue_alloc() hook would return -EPERM only if the task calling sys_restart() wasn't allowed to create a msg queue with its own type, right? Is that something which is often disallowed?

I suppose we could have the default (cap_msg_queue_restore) call security_ops->msg_queue_alloc() - feels frail, but maybe it's ok...

Quoted 45 lines · Serge E. Hallyn

Quoted 37 lines · Serge E. Hallyn

Quoted 27 lines · Stephen Smalley

Quoted 4 lines · Serge E. Hallyn

ipc/checkpoint_msg.c +0 -0

index 51385b0..ca55339 100644

<snip>

Quoted 21 lines

ipc/checkpoint_msg.c +12 -0

@@ -175,11 +183,26 @@ static int load_ipc_msg_hdr(struct ckpt_ctx *ctx,
 			    struct msg_queue *msq)
 {
 	int ret = 0;
+	int secid = 0;
 
 	ret = restore_load_ipc_perms(&h->perms, &msq->q_perm);
 	if (ret < 0)
 		return ret;
 
+	if (h->perms.secref) {
+		struct sec_store *s;
+		s = ckpt_obj_fetch(ctx, h->perms.secref, CKPT_OBJ_SECURITY);
+		if (IS_ERR(s))
+			return PTR_ERR(s);
+		secid = s->secid;
+	}
+	ret = security_msg_queue_alloc(msq);
+	if (ret)
+		return ret;
+	ret = security_msg_queue_restore(msq, secid);

Quoted 8 lines

ipc/checkpoint_msg.c static int load_ipc_msg_hdr(struct ckpt_ctx *ctx, · line 204

Quoted 2 lines

ipc/checkpoint_msg.c +2 -0

+	if (ret < 0)
+		return ret;

I don't think you want to call security_msg_queue_alloc() here, as that both allocates the security struct and performs the create check. So I would just call the _restore() function, and let it internally call ipc_alloc_security() to allocate the struct but then apply its own distinct restore check. Likewise for the rest of them.

Quoted 1 line

Ok, will change that.

Hmm, but that means that if there is some new LSM which allocates memory in security_msg_queue_alloc(), but which does not define security_msg_queue_restore() (for some stupid reason), it'll end up causing a bug.

Quoted 7 lines

It's something we can certainly catch through code review, but do we want to set such a scenario up at all?

Speaking just for SELinux, the security_msg_queue_alloc() hook would return -EPERM only if the task calling sys_restart() wasn't allowed to create a msg queue with its own type, right? Is that something which is often disallowed?

Certainly some program domains lack permission to create ipc objects. The ipc _alloc hooks are unusual in that they combine both allocation and create checking unlike the rest of the object alloc hooks. I think that was discussed at the time, but people didn't want two different hook calls at the same call site.

Quoted 54 lines · Stephen Smalley

Quoted 45 lines · Serge E. Hallyn

Quoted 37 lines · Serge E. Hallyn

Quoted 27 lines · Stephen Smalley

Quoted 4 lines · Serge E. Hallyn

ipc/checkpoint_msg.c +0 -0

index 51385b0..ca55339 100644

<snip>

Quoted 21 lines

ipc/checkpoint_msg.c +12 -0

@@ -175,11 +183,26 @@ static int load_ipc_msg_hdr(struct ckpt_ctx *ctx,
 			    struct msg_queue *msq)
 {
 	int ret = 0;
+	int secid = 0;
 
 	ret = restore_load_ipc_perms(&h->perms, &msq->q_perm);
 	if (ret < 0)
 		return ret;
 
+	if (h->perms.secref) {
+		struct sec_store *s;
+		s = ckpt_obj_fetch(ctx, h->perms.secref, CKPT_OBJ_SECURITY);
+		if (IS_ERR(s))
+			return PTR_ERR(s);
+		secid = s->secid;
+	}
+	ret = security_msg_queue_alloc(msq);
+	if (ret)
+		return ret;
+	ret = security_msg_queue_restore(msq, secid);

Quoted 8 lines

ipc/checkpoint_msg.c static int load_ipc_msg_hdr(struct ckpt_ctx *ctx, · line 204

Quoted 2 lines

ipc/checkpoint_msg.c +2 -0

+	if (ret < 0)
+		return ret;

I don't think you want to call security_msg_queue_alloc() here, as that both allocates the security struct and performs the create check. So I would just call the _restore() function, and let it internally call ipc_alloc_security() to allocate the struct but then apply its own distinct restore check. Likewise for the rest of them.

Quoted 1 line

Ok, will change that.

Hmm, but that means that if there is some new LSM which allocates memory in security_msg_queue_alloc(), but which does not define security_msg_queue_restore() (for some stupid reason), it'll end up causing a bug.

Quoted 7 lines

It's something we can certainly catch through code review, but do we want to set such a scenario up at all?

Speaking just for SELinux, the security_msg_queue_alloc() hook would return -EPERM only if the task calling sys_restart() wasn't allowed to create a msg queue with its own type, right? Is that something which is often disallowed?

Quoted 5 lines

Certainly some program domains lack permission to create ipc objects. The ipc _alloc hooks are unusual in that they combine both allocation and create checking unlike the rest of the object alloc hooks. I think that was discussed at the time, but people didn't want two different hook calls at the same call site.

Oh, no. I wasn't thinking right.

The objects are actually restored through calls to do_shmget() etc, so that security_xyz_alloc() already gets called.

So I think we'll just leave it as is right now, acknowledging that it might become problematic if we want to confine a restart_t domain to be able to restore but not alloc any ipcs. The actual ramifications of that still somewhat escape me, but I do prefer having the common helpers used whenever possible to recreate objects.

Quoted 4 lines · Serge E. Hallyn

Oh, no. I wasn't thinking right.

The objects are actually restored through calls to do_shmget() etc, so that security_xyz_alloc() already gets called.

Does this mean that the objects temporarily exist in the wrong security context and are accessible to other threads during the interval between creation and when they get "restored" to the right security context?

Quoted 9 lines · Stephen Smalley

Quoted 4 lines · Serge E. Hallyn

Oh, no. I wasn't thinking right.

The objects are actually restored through calls to do_shmget() etc, so that security_xyz_alloc() already gets called.

Does this mean that the objects temporarily exist in the wrong security context and are accessible to other threads during the interval between creation and when they get "restored" to the right security context?

They get restored in a private IPC namespace so they aren't accessible to any live tasks. Also, the objects will be created using the default context for the program doing sys_restore(), running as app_restore_t or something, so presumably a policy could ensure that such temporary objects aren't readable by anyone else, just in case something goes wrong before the security_ipcxyz_restore(), right?

Quoted 1 line · Stephen Smalley

On Fri, 2009-06-19 at 20:32 -0500, Serge E. Hallyn wrote:

Quoted 4 lines

Also, where do we get to veto attempts to checkpoint the task in the first place? If ptrace, I think we'd want it treated as a PTRACE_MODE_ATTACH (also used for /proc/pid/mem) rather than just PTRACE_MODE_READ (reading other /proc/pid info).

The checkpointing of ipc objects goes through an ipcperms(perm, S_IROTH) check in ipc/checkpoint (at top of
http://git.ncl.cs.columbia.edu/?p=linux-cr.git;a=blob;f=ipc/checkpoint.c;h=88996e2b7abf328bd1b263400798ed5bd4924f48;hb=HEAD
)

But yes, for the task itself we check PTRACE_MODE_READ (line 280 in
http://git.ncl.cs.columbia.edu/?p=linux-cr.git;a=blob;f=checkpoint/checkpoint.c;h=a6dee4fb1085a47095f24443c48683a7fbc8ac59;hb=HEAD ) I had thought that PTRACE_MODE_ATTACH implied the permission to
actually modify the task. If it also can imply a "very invasive" read
then changing it certainly seems right.