Re: [PATCH v2 1/3] capabilities: Introduce CAP_CHECKPOINT_RESTORE

Nicolas Viennot [off-list ref] writes:

quoted

quoted

 proc_map_files_get_link(struct dentry *dentry,
 			struct inode *inode,
 		        struct delayed_call *done)
 {
-	if (!capable(CAP_SYS_ADMIN))
+	if (!(capable(CAP_SYS_ADMIN) || capable(CAP_CHECKPOINT_RESTORE)))
 		return ERR_PTR(-EPERM);

quoted

First of all -- sorry for late reply. You know, looking into this
code more I think this CAP_SYS_ADMIN is simply wrong: for example I
can't even fetch links for /proc/self/map_files. Still
/proc/$pid/maps (which as well points to the files opened) test for
ptrace-read permission. I think we need ptrace-may-attach test here
instead of these capabilities (if I can attach to a process I can
read any data needed, including the content of the mapped files, if
only I'm not missing something obvious).

Currently /proc/pid/map_files/* have exactly the same permission
checks as /proc/pid/fd/*, with the exception of the extra
CAP_SYS_ADMIN check. The check originated from the following
discussions where 3 security issues are discussed:
http://lkml.iu.edu/hypermail/linux/kernel/1505.2/02524.html
http://lkml.iu.edu/hypermail/linux/kernel/1505.2/04030.html

From what I understand, the extra CAP_SYS_ADMIN comes from the
following issues:

1. Being able to open dma-buf / kdbus region (referred in the
referenced email as problem #1). I don't fully understand what the
dangers are, but perhaps we could do CAP_SYS_ADMIN check only for such
dangerous files, as opposed to all files.

I don't know precisely the concern but my memory is that some drivers do
interesting things when mmaped.  Possibly even to changing the vm_file.

I think that is worth running to the ground and figuring out in the
context of checkpoint/restart because the ordinary checkpoint/restart
code won't be able deal with them either.

So I vote for figuring that case out and dealing with it.

2. /proc/pid/fd/* is already a security hole (Andy says "I hope to fix
that some day"). He essentially says that it's not because fds are
insecure that map_files should be too. He seems to claim that mapped
files that are then closed seems to be a bigger concern than other
opened files. However, in the present time (5 years after these email
conversations), the fd directory does not have the CAP_SYS_ADMIN check
which doesn't convinces me that the holes of /proc/pid/fd/* are such a
big of a deal. I'm not entirely sure what security issue Andy refers
to, but, I understand something along the lines of: Some process gets
an fd of a file read-only opened (via a unix socket for example, or
after a chroot), and gets to re-open the file in write access via
/proc/self/fd/N to do some damage.

I would hope the other permission checks on such a file will prevent
some of that nonsense.  But definitely worth taking a hard look at.

3. Being able to ftruncate a file after a chroot+privilege drop. I may
be wrong, but if privileges were dropped, then there's no reason that
the then unprivileged user would have write access to the mmaped file
inode. Seems a false problem.

Yes.

It turns out that some of these concerns have been addressed with the
introduction of memfd with seals, introduced around the same time
where the map_files discussions took place. These seals allow one to
share write access of an mmap region to an unsecure program, without
fearing of getting a SIGBUS because the unsecure program could call
ftruncate() on the fd. More on that at
https://lwn.net/Articles/593918/ . Also, that article says "There are
a number of fairly immediate use cases for the sealing functionality
in general. Graphics drivers could use it to safely receive buffers
from applications. The upcoming kdbus transport can benefit from
sealing.". This rings a bell with problem #1. Perhaps memfd is a
solution to Andy's concerns?

Overall, I think the CAP_SYS_ADMIN map_files/ extra check compared to
fd/ does not improve security in practice. Fds will be given to
insecure programs. Better security can be achieved with memfd seals,
and sane permissioning on files, regardless if they were once closed.

I would love to see the work put in to safely relax the permission check
from capable to ns_capable.  Which is just dealing with point 1.

There might be some other assumptions that a process can't get at mmaped
regions.

Eric