On Wed, Jul 28, 2010 at 12:25:03PM -0700, Sukadev Bhattiprolu wrote:
> Oren Laadan [orenl@xxxxxxxxxxxxxxx] 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
