[Devel] Re: [PATCH 4/9][cr][v2]: Restore file_owner info

[Matt Helsley]

On Wed, Jul 28, 2010 at 12:25:03PM -0700, Sukadev Bhattiprolu wrote:
> Oren Laadan [orenl at cs.columbia.edu] wrote:
> | > +
> | > +	rcu_read_lock();
> | > +	pid = find_vpid(h->f_owner_pid);
> | 
> | What if this fails - the pid is invalid/non-existent ?
> 
> Good point. ->f_owner_pid can be 0 (in the normal case) and __fsetown()
> below will set the owner to NULL pid. But if ->f_owner_pid is non-zero,
> we should ensure we found a valid pid - added a check for this.
> 
> | 
> | > +	/*
> | > +	 * TODO: Do we need to force==1 or can it be 0 ? 'force' is used to
> | > +	 * 	 modify the owner, if one is already set. Can it be set when
> | > +	 * 	 we restart an application ?
> | > +	 */
> | > +	ret = __f_setown(file, pid, h->f_owner_pid_type, uid, euid, 1);
> | > +	rcu_read_unlock();
> | 
> | I wonder if this would be a problem in terms of security on a
> | non-container restart (e.g. not in a new pid-ns): one could set
> | any pid as owner and any signal to be sent, and cause an arbitrary
> | signal to be sent to an arbitrary process ?
> 
> Yes, Matt and Serge pointed it out and for now we need CAP_KILL
> capability to restore an application that has file-leases.

I looked at this some.

The pid and the signal are looked up when the signal is generated. Then
permissions are checked. See sigio_perm() called from send_sigio_to_task().

Thus I think the main thing we need to be certain of are the uid and euid
to pass in to own the file. Those are going to require some userns bits
I think. Then the signal number and recipient will be checked as normal
-- we don't need to do anything special during restart.

For reference, here's sigio_perm():

static inline int sigio_perm(struct task_struct *p,
                             struct fown_struct *fown, int sig)
{ 
        const struct cred *cred;
        int ret;

        rcu_read_lock();
        cred = __task_cred(p);
        ret = ((fown->euid == 0 ||
                fown->euid == cred->suid || fown->euid == cred->uid ||
                fown->uid  == cred->suid || fown->uid  == cred->uid) &&
               !security_file_send_sigiotask(p, fown, sig));
        rcu_read_unlock();
        return ret;
}

[ My Notes: unlike check_kill_permission() it does not check CAP_KILL.
Also check_kill_permission() calls audit as if the signal is about to be
delivered but sigio_perm() does not. ]

Cheers,
	-Matt Helsley
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers