On 05/18/2010 11:07 PM, Sukadev Bhattiprolu wrote:
> Restore the file-owner information for each 'struct file'. This is
> essentially is like a new fcntl(F_SETOWN) and fcntl(F_SETSIG) calls,
> except that the pid, uid, euid and signum values are read from the
> checkpoint image.
>
> Changelog[v2]:
> - [Matt Helsley, Serge Hallyn]: Don't trust uids in checkpoint image.
> (added CAP_KILL check)
> - Check that signal number read from the checkpoint image is valid.
> (not sure it is required, since its an incomplete check for tampering)
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@xxxxxxxxxxxxxxxxxx>
> ---
[...]
>
> +static int restore_file_owner(struct ckpt_ctx *ctx, struct ckpt_hdr_file *h,
> + struct file *file)
> +{
> + int ret;
> + struct pid *pid;
> + uid_t uid, euid;
> +
> + uid = h->f_owner_uid;
> + euid = h->f_owner_euid;
> +
> + ckpt_debug("restore_file_owner(): uid %u, euid %u, pid %d, type %d\n",
> + uid, euid, h->f_owner_pid, h->f_owner_pid_type);
> + /*
> + * We can't trust the uids in the checkpoint image and normally need
> + * CAP_KILL. But if the uids match our ids, should be fine since we
> + * have access to the file.
> + *
> + * TODO: Move this check to __f_setown() ?
> + */
> + ret = -EACCES;
> + if (!capable(CAP_KILL) &&
> + (uid != current_uid() || euid != current_euid())) {
> + ckpt_err(ctx, ret, "image uids [%d, %d] don't match current "
> + "process uids [%d, %d] and no CAP_KILL\n",
> + uid, euid, current_uid(), current_euid());
> + return ret;
> + }
> +
> + ret = -EINVAL;
> + if (!valid_signal(h->f_owner_signum)) {
> + ckpt_err(ctx, ret, "Invalid signum %d\n", h->f_owner_signum);
> + return ret;
> + }
> + file->f_owner.signum = h->f_owner_signum;
> +
> + rcu_read_lock();
> + pid = find_vpid(h->f_owner_pid);
What if this fails - the pid is invalid/non-existent ?
> + /*
> + * 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 ?
> +
> + if (ret < 0)
> + ckpt_err(ctx, ret, "__fsetown_uid() failed\n");
> +
> + return ret;
> +}
> +
> #define CKPT_SETFL_MASK \
> (O_APPEND | O_NONBLOCK | O_NDELAY | FASYNC | O_DIRECT | O_NOATIME)
>
> @@ -648,6 +699,10 @@ int restore_file_common(struct ckpt_ctx *ctx, struct file *file,
> if (ret < 0)
> return ret;
>
> + ret = restore_file_owner(ctx, h, file);
> + if (ret < 0)
> + return ret;
> +
> /*
> * Normally f_mode is set by open, and modified only via
> * fcntl(), so its value now should match that at checkpoint.
Oren.
--
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
