linux-security-module.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christian Brauner <christian.brauner@ubuntu.com>
To: Matt Helsley <mhelsley@vmware.com>
Cc: "Adrian Reber" <areber@redhat.com>,
	"Eric Biederman" <ebiederm@xmission.com>,
	"Pavel Emelyanov" <ovzxemul@gmail.com>,
	"Oleg Nesterov" <oleg@redhat.com>,
	"Dmitry Safonov" <0x7f454c46@gmail.com>,
	"Andrei Vagin" <avagin@gmail.com>,
	"Nicolas Viennot" <Nicolas.Viennot@twosigma.com>,
	"Michał Cłapiński" <mclapinski@google.com>,
	"Kamil Yurtsever" <kyurtsever@google.com>,
	"Dirk Petersen" <dipeit@gmail.com>,
	"Christine Flood" <chf@redhat.com>,
	"Mike Rapoport" <rppt@linux.ibm.com>,
	"Radostin Stoyanov" <rstoyanov1@gmail.com>,
	"Cyrill Gorcunov" <gorcunov@openvz.org>,
	"Serge Hallyn" <serge@hallyn.com>,
	"Stephen Smalley" <stephen.smalley.work@gmail.com>,
	"Sargun Dhillon" <sargun@sargun.me>,
	"Arnd Bergmann" <arnd@arndb.de>,
	"Aaron Goidel" <acgoide@tycho.nsa.gov>,
	linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org, selinux@vger.kernel.org,
	"Eric Paris" <eparis@parisplace.org>,
	"Jann Horn" <jannh@google.com>
Subject: Re: [PATCH] capabilities: Introduce CAP_RESTORE
Date: Fri, 12 Jun 2020 16:39:26 +0200	[thread overview]
Message-ID: <20200612143926.ingimdxctgpffkm6@wittgenstein> (raw)
In-Reply-To: <20200612001709.GA25598@rlwimi.vmware.com>

On Thu, Jun 11, 2020 at 05:17:09PM -0700, Matt Helsley wrote:
> On Fri, May 22, 2020 at 07:53:50AM +0200, Adrian Reber wrote:
> > This enables CRIU to checkpoint and restore a process as non-root.
> > 
> > Over the last years CRIU upstream has been asked a couple of time if it
> > is possible to checkpoint and restore a process as non-root. The answer
> > usually was: 'almost'.
> > 
> > The main blocker to restore a process was that selecting the PID of the
> > restored process, which is necessary for CRIU, is guarded by CAP_SYS_ADMIN.
> > 
> > In the last two years the questions about checkpoint/restore as non-root
> > have increased and especially in the last few months we have seen
> > multiple people inventing workarounds.
> > 
> > The use-cases so far and their workarounds:
> > 
> >  * Checkpoint/Restore in an HPC environment in combination with
> >    a resource manager distributing jobs. Users are always running
> >    as non root, but there was the desire to provide a way to
> >    checkpoint and restore long running jobs.
> >    Workaround: setuid wrapper to start CRIU as root as non-root
> >    https://github.com/FredHutch/slurm-examples/blob/master/checkpointer/lib/checkpointer/checkpointer-suid.c
> >  * Another use case to checkpoint/restore processes as non-root
> >    uses as workaround a non privileged process which cycles through
> >    PIDs by calling fork() as fast as possible with a rate of
> >    100,000 pids/s instead of writing to ns_last_pid
> >    https://github.com/twosigma/set_ns_last_pid
> >  * Fast Java startup using checkpoint/restore.
> >    We have been in contact with JVM developers who are integrating
> >    CRIU into a JVM to decrease the startup time.
> >    Workaround so far: patch out CAP_SYS_ADMIN checks in the kernel
> >  * Container migration as non root. There are people already
> >    using CRIU to migrate containers as non-root. The solution
> >    there is to run it in a user namespace. So if you are able
> >    to carefully setup your environment with the namespaces
> >    it is already possible to restore a container/process as non-root.
> >    Unfortunately it is not always possible to setup an environment
> >    in such a way and for easier access to non-root based container
> >    migration this patch is also required.
> > 
> > There are probably a few more things guarded by CAP_SYS_ADMIN required
> > to run checkpoint/restore as non-root, but by applying this patch I can
> > already checkpoint and restore processes as non-root. As there are
> > already multiple workarounds I would prefer to do it correctly in the
> > kernel to avoid that CRIU users are starting to invent more workarounds.
> > 
> > I have used the following tests to verify that this change works as
> > expected by setting the new capability CAP_RESTORE on the two resulting
> > test binaries:
> > 
> > $ cat ns_last_pid.c
> >  // http://efiop-notes.blogspot.com/2014/06/how-to-set-pid-using-nslastpid.html
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> >  #include <sys/file.h>
> >  #include <sys/types.h>
> >  #include <unistd.h>
> > 
> > int main(int argc, char *argv[])
> > {
> > 	pid_t pid, new_pid;
> > 	char buf[32];
> > 	int fd;
> > 
> > 	if (argc != 2)
> > 		return 1;
> > 
> > 	printf("Opening ns_last_pid...\n");
> > 	fd = open("/proc/sys/kernel/ns_last_pid", O_RDWR | O_CREAT, 0644);
> > 	if (fd < 0) {
> > 		perror("Cannot open ns_last_pid");
> > 		return 1;
> > 	}
> > 
> > 	printf("Locking ns_last_pid...\n");
> > 	if (flock(fd, LOCK_EX)) {
> > 		close(fd);
> > 		printf("Cannot lock ns_last_pid\n");
> > 		return 1;
> > 	}
> > 
> > 	pid = atoi(argv[1]);
> > 	snprintf(buf, sizeof(buf), "%d", pid - 1);
> > 	printf("Writing pid-1 to ns_last_pid...\n");
> > 	if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> > 		printf("Cannot write to buf\n");
> > 		return 1;
> > 	}
> > 
> > 	printf("Forking...\n");
> > 	new_pid = fork();
> > 	if (new_pid == 0) {
> > 		printf("I am the child!\n");
> > 		exit(0);
> > 	} else if (new_pid == pid)
> > 		printf("I am the parent. My child got the pid %d!\n", new_pid);
> > 	else
> > 		printf("pid (%d) does not match expected pid (%d)\n", new_pid, pid);
> > 
> > 	printf("Cleaning up...\n");
> > 	if (flock(fd, LOCK_UN))
> > 		printf("Cannot unlock\n");
> > 	close(fd);
> > 	return 0;
> > }
> > $ id -u; /home/libcap/ns_last_pid 300000
> > 1001
> > Opening ns_last_pid...
> > Locking ns_last_pid...
> > Writing pid-1 to ns_last_pid...
> > Forking...
> > I am the parent. My child got the pid 300000!
> > I am the child!
> > Cleaning up...
> > 
> > For the clone3() based approach:
> > $ cat clone3_set_tid.c
> >  #define _GNU_SOURCE
> >  #include <linux/sched.h>
> >  #include <stdint.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> >  #include <sys/syscall.h>
> >  #include <unistd.h>
> > 
> >  #define ptr_to_u64(ptr) ((__u64)((uintptr_t)(ptr)))
> > 
> > int main(int argc, char *argv[])
> > {
> > 	struct clone_args c_args = { };
> > 	pid_t pid, new_pid;
> > 
> > 	if (argc != 2)
> > 		return 1;
> > 
> > 	pid = atoi(argv[1]);
> > 	c_args.set_tid = ptr_to_u64(&pid);
> > 	c_args.set_tid_size = 1;
> > 
> > 	printf("Forking...\n");
> > 	new_pid = syscall(__NR_clone3, &c_args, sizeof(c_args));
> 
> (Note: I'm going to call the capability CAP_RESTORE but I think this
> applies regardless of whether the permissions stay with CAP_SYS_ADMIN..)
> 
> I haven't fully reviewed the discussion of the security consequences but
> my sense is this would require retaining CAP_RESTORE down the entire tree
> of processes being restored so each parent could call clone3() with the
> correct pid value for its child(ren).

If criu restores a process tree you need to have the required capability
in the starting user namespace and by design this means you also have it
automatically in all child user namespaces. Let alone that if you/criu
created the user namespace you have full privileges over it anway.
While criu restores a process tree it needs to retain its privilege
level. The individual tasks in the process tree that criu restores will
obviously be restored with the creds they were checkpointed with so it's
not like any of them inherit any capabilities they didn't already have
after criu finishes. So I don't see why any of this would be a problem.
Restoring into a (pre-existing) pid namespace hierarchy is another
problem but how this can be solved using the set_tid array was outlined
during Plumbers last year; one of the reasons this is an array.

> 
> Ideally you would drop CAP_RESTORE sooner -- preferrably only one
> process would need it. I think you could do that by changing what you pass
> down; instead of passing down a capability and a pid number, pass down a
> special "reservation" pidfd:

I'm sorry, but nack.
We're not going to do shenaningans like that with pidfds and clone3()
just to cater to this specific use-case that is solvable in less hacky
ways.

Thanks!
Christian

      reply	other threads:[~2020-06-12 14:40 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-22  5:53 [PATCH] capabilities: Introduce CAP_RESTORE Adrian Reber
2020-05-22  7:53 ` Christian Brauner
2020-05-22 18:02   ` Andrei Vagin
2020-05-22 13:41 ` Christian Brauner
2020-05-22 16:40 ` Casey Schaufler
2020-05-23  4:27   ` Andrei Vagin
2020-05-25  2:01     ` Casey Schaufler
2020-05-25  8:05   ` Adrian Reber
2020-05-25 18:55     ` Casey Schaufler
2020-05-27 13:48       ` Adrian Reber
2020-05-27 15:57         ` Casey Schaufler
2020-05-27 16:37           ` Nicolas Viennot
2020-05-27 16:46             ` Casey Schaufler
2020-05-26 13:59     ` Eric W. Biederman
     [not found]       ` <CALKUemw0UZ67yaDwAomHh0n8QZfjd52QvgEXTJ4R3JSrQjZX9g@mail.gmail.com>
2020-05-26 19:19         ` Casey Schaufler
2020-05-26 19:51         ` Jann Horn
2020-05-27 14:14       ` Adrian Reber
2020-05-27 15:29         ` Christian Brauner
2020-05-27 18:05           ` Nicolas Viennot
2020-05-28  9:48             ` Christian Brauner
2020-06-08  2:09               ` Andrei Vagin
2020-05-25 21:53 ` Jann Horn
2020-05-26  9:09   ` Radostin Stoyanov
2020-06-12  0:17 ` Matt Helsley
2020-06-12 14:39   ` Christian Brauner [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200612143926.ingimdxctgpffkm6@wittgenstein \
    --to=christian.brauner@ubuntu.com \
    --cc=0x7f454c46@gmail.com \
    --cc=Nicolas.Viennot@twosigma.com \
    --cc=acgoide@tycho.nsa.gov \
    --cc=areber@redhat.com \
    --cc=arnd@arndb.de \
    --cc=avagin@gmail.com \
    --cc=chf@redhat.com \
    --cc=dipeit@gmail.com \
    --cc=ebiederm@xmission.com \
    --cc=eparis@parisplace.org \
    --cc=gorcunov@openvz.org \
    --cc=jannh@google.com \
    --cc=kyurtsever@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=mclapinski@google.com \
    --cc=mhelsley@vmware.com \
    --cc=oleg@redhat.com \
    --cc=ovzxemul@gmail.com \
    --cc=rppt@linux.ibm.com \
    --cc=rstoyanov1@gmail.com \
    --cc=sargun@sargun.me \
    --cc=selinux@vger.kernel.org \
    --cc=serge@hallyn.com \
    --cc=stephen.smalley.work@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).