On Wed, 2009-06-24 at 17:07 -0500, Serge E. Hallyn wrote:
> Quoting Stephen Smalley (sds@xxxxxxxxxxxxxx):
> > On Tue, 2009-06-23 at 14:57 -0500, Serge E. Hallyn wrote:
> > > Quoting Serge E. Hallyn (serue@xxxxxxxxxx):
> > > > Quoting Stephen Smalley (sds@xxxxxxxxxxxxxx):
> > > > > On Fri, 2009-06-19 at 20:32 -0500, Serge E. Hallyn wrote:
> > > > > > diff --git a/ipc/checkpoint_msg.c b/ipc/checkpoint_msg.c
> > > > > > index 51385b0..ca55339 100644
> > > > > > --- a/ipc/checkpoint_msg.c
> > > > > > +++ b/ipc/checkpoint_msg.c
> > > > > <snip>
> > > > > > @@ -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);
> > > > > > + 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.
> > >
> > > 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?
> >
> > 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.
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?
> 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.
--
Stephen Smalley
National Security Agency
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
