Re: [PATCH 3/4] Save and restore UNIX socket peer credentials

["Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> This saves the uid/gid of the sk_peercred structure in the checkpoint
> stream.  On restart, it uses may_setuid() and may_setgid() to determine
> if the uid/gid from the checkpoint stream may be used.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  include/linux/checkpoint_hdr.h |    2 ++
>  net/unix/checkpoint.c          |   29 ++++++++++++++++-------------
>  2 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
> index 829ff2d..6c6780c 100644
> --- a/include/linux/checkpoint_hdr.h
> +++ b/include/linux/checkpoint_hdr.h
> @@ -523,6 +523,8 @@ struct ckpt_hdr_socket_unix {
>  	struct ckpt_hdr h;
>  	__s32 this;
>  	__s32 peer;
> +	__u32 peercred_uid;
> +	__u32 peercred_gid;
>  	__u32 flags;
>  	__u32 laddr_len;
>  	__u32 raddr_len;
> diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
> index 841d25d..eb19e66 100644
> --- a/net/unix/checkpoint.c
> +++ b/net/unix/checkpoint.c
> @@ -3,6 +3,7 @@
>  #include <linux/fs_struct.h>
>  #include <linux/checkpoint.h>
>  #include <linux/checkpoint_hdr.h>
> +#include <linux/user.h>
>  #include <net/af_unix.h>
>  #include <net/tcp_states.h>
> 
> @@ -98,6 +99,9 @@ int sock_unix_checkpoint(struct ckpt_ctx *ctx,
>  		goto out;
>  	}
> 
> +	un->peercred_uid = socket->sk->sk_peercred.uid;
> +	un->peercred_gid = socket->sk->sk_peercred.gid;
> +
>  	ret = ckpt_write_obj(ctx, (struct ckpt_hdr *) h);
>  	if (ret < 0)
>  		goto out;
> @@ -225,19 +229,6 @@ static int sock_unix_join(struct ckpt_ctx *ctx,
>  	unix_sk(a)->peer = b;
>  	unix_sk(b)->peer = a;
> 
> -	/* TODO:
> -	 * Checkpoint the credentials, restore them here if the values match
> -	 * the restored creds or we may_setuid()
> -	 */
> -
> -	a->sk_peercred.pid = task_tgid_vnr(current);
> -	a->sk_peercred.uid = ctx->realcred->uid;
> -	a->sk_peercred.gid = ctx->realcred->gid;
> -
> -	b->sk_peercred.pid = a->sk_peercred.pid;
> -	b->sk_peercred.uid = a->sk_peercred.uid;
> -	b->sk_peercred.gid = a->sk_peercred.gid;
> -
>  	if (!UNIX_ADDR_EMPTY(un->raddr_len))
>  		addr = sock_unix_makeaddr(&un->raddr, un->raddr_len);
>  	else if (!UNIX_ADDR_EMPTY(un->laddr_len))
> @@ -303,6 +294,18 @@ static int sock_unix_restore_connected(struct ckpt_ctx *ctx,
>  		goto out;
>  	}
> 
> +	this->sk_peercred.pid = task_tgid_vnr(current);
> +
> +	if (may_setuid(ctx->realcred->user->user_ns, un->peercred_uid) &&
> +	    may_setgid(ctx->realcred->group_info, un->peercred_gid)) {
> +		this->sk_peercred.uid = un->peercred_uid;
> +		this->sk_peercred.gid = un->peercred_gid;

It's a real shame that we have this uid and gid with no indication of
which user_ns it belongs in.  But I do think that assuming
ctx->realcred->user->user_ns  is the right one is the best guess you
can make.

So the may_setuid() is right, but may_setgid() should be changed
to
	may_setgid(ctx->realcred->user->user_ns, un->peercred_gid,
			current_cred());

meaning: we want to know whether:
	1. current_cred() has cap_capable to ctx->realcred->user->user_ns
		(which it does if it created it - once that's implemented)
or
	(
	2. current_cred->user->user_ns == ctx->real_cred->user_user_ns
		and
	3. un->peercred_gid is equal to current_cred()->egid or is in
		current_cred->group_info.
	)

Then again, until we add a user_ns to peercred, that will result
in a safety problem with peercred!

/me thinks some more

> +	} else {
> +		ckpt_debug("peercred %i:%i would require setuid",
> +			   un->peercred_uid, un->peercred_gid);
> +		return -1;
> +	}
> +
>  	/* Prime the socket's buffer limit with the maximum.  These will be
>  	 * overwritten with the values in the checkpoint stream in a later
>  	 * phase.
> -- 
> 1.6.0.4
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers