Message-ID: <b7797029-2fd3-3773-a51a-bf073a96c848@schaufler-ca.com>
Date:   Tue, 26 May 2020 12:19:08 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Christine Flood <chf@...hat.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Adrian Reber <areber@...hat.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Pavel Emelyanov <ovzxemul@...il.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Andrei Vagin <avagin@...il.com>,
        Nicolas Viennot <Nicolas.Viennot@...sigma.com>,
        Michał Cłapiński <mclapinski@...gle.com>,
        Kamil Yurtsever <kyurtsever@...gle.com>,
        Dirk Petersen <dipeit@...il.com>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Radostin Stoyanov <rstoyanov1@...il.com>,
        Cyrill Gorcunov <gorcunov@...nvz.org>,
        Serge Hallyn <serge@...lyn.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Sargun Dhillon <sargun@...gun.me>,
        Arnd Bergmann <arnd@...db.de>,
        Aaron Goidel <acgoide@...ho.nsa.gov>,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, selinux@...r.kernel.org,
        Eric Paris <eparis@...isplace.org>,
        Jann Horn <jannh@...gle.com>, "Haley, Andrew" <aph@...hat.com>,
        "Bhole, Deepak" <dbhole@...hat.com>,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH] capabilities: Introduce CAP_RESTORE

On 5/26/2020 12:01 PM, Christine Flood wrote:

Please do not top-post on this list.

> Java applications suffer from slow startup times due to dynamic class loading and warming up the Just In Time compilers.  Not all Java users have root access on their machines.  Enabling CRIU in user mode solves this problem for us.  We are about to release a user library that will allow check pointing Java from within Java.  Having to run this as root would severely limit its utility.

The performance of dynamic loading is a well understood issue.
Please don't conflate that with the security issues involved.
Security is *not* the basic problem. If you are having problems
with application start-up performance you really should be be
addressing that directly rather than implementing sophisticated
workarounds that require system security changes.

>
>
> Christine
>
> On Tue, May 26, 2020 at 10:05 AM Eric W. Biederman <ebiederm@...ssion.com <mailto:ebiederm@...ssion.com>> wrote:
>
>     Adrian Reber <areber@...hat.com <mailto:areber@...hat.com>> writes:
>
>     > On Fri, May 22, 2020 at 09:40:37AM -0700, Casey Schaufler wrote:
>
>     >> What are the other blockers? Are you going to suggest additional new
>     >> capabilities to clear them?
>     >
>     > As mentioned somewhere else access to /proc/<pid>/map_files/ would be
>     > helpful. Right now I am testing with a JVM and it works without root
>     > just with the attached patch. Without access to /proc/<pid>/map_files/
>     > not everything CRIU can do will actually work, but we are a lot closer
>     > to what our users have been asking for.
>
>     The current permission checks on /proc/<pid>/map_files/ are simply
>     someone being over-cautious.
>
>     Someone needs to think through the threat landscape and figure out what
>     permission checks are actually needed.
>
>     Making the permission check ns_capable instead of capable is a
>     no-brainer.  Figuring out which user_ns to test against might be a
>     we bit harder.
>
>     We could probably even allow the owner of the process to open the files
>     but that requires someone doing the work of thinking through how
>     being able to opening files that you have mmaped might be a problem.
>
>     >> > There are probably a few more things guarded by CAP_SYS_ADMIN required
>     >> > to run checkpoint/restore as non-root,
>     >>
>     >> If you need CAP_SYS_ADMIN anyway you're not gaining anything by
>     >> separating out CAP_RESTORE.
>     >
>     > No, as described we can checkpoint and restore a JVM with this patch and
>     > it also solves the problem the set_ns_last_pid fork() loop daemon tries
>     > to solve. It is not enough to support the full functionality of CRIU as
>     > map_files is also important, but we do not need CAP_SYS_ADMIN and
>     > CAP_RESTORE. Only CAP_RESTORE would be necessary.
>     >
>     > With a new capability users can enable checkpoint/restore as non-root
>     > without giving CRIU access to any of the other possibilities offered by
>     > CAP_SYS_ADMIN. Setting a PID and map_files have been introduced for CRIU
>     > and used to live behind CONFIG_CHECKPOINT_RESTORE. Having a capability
>     > for checkpoint/restore would make it easier for CRIU users to run it as
>     > non-root and make it very clear what is possible when giving CRIU the
>     > new capability. No other things would be allowed than necessary for
>     > checkpoint/restore. Setting a PID is most important for the restore part
>     > and reading map_files would be helpful during checkpoint. So it actually
>     > should be called CAP_CHECKPOINT_RESTORE as Christian mentioned in
>     > another email.
>
>     Please if one is for checkpoint and one is for restore asking for a pair
>     of capabilities is probably more appropriate.
>
>     >> >  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.
>     >>
>     >> You've presented a couple of really inappropriate implementations
>     >> that would qualify as workarounds. But the other two are completely
>     >> appropriate within the system security policy. They don't "get around"
>     >> the problem, they use existing mechanisms as they are intended.
>     >
>     > I agree with the user namespace approach to be appropriate, but not the
>     > CAP_SYS_ADMIN approach as CRIU only needs a tiny subset (2 things) of
>     > what CAP_SYS_ADMIN allows.
>
>
>     If we are only talking 2 things can you please include in your patchset
>     a patch enabling those 2 things?
>
>     But even more than this we need a request that asks not for the least
>     you can possibly ask for but asks for what you need to do a good job.
>
>     I am having visions of a recurring discussion that says can we add one
>     more permission check to CAP_RESTORE or CAP_CHECKPOINT when they are
>     things we could know today.
>
>     Eric
>

Powered by blists - more mailing lists