linux-kernel - RE: [PATCH v2 1/3] capabilities: Introduce CAP_CHECKPOINT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cda72e8402244a85862f95ea84ff9204@EXMBDFT11.ad.twosigma.com>
Date:   Tue, 9 Jun 2020 20:09:49 +0000
From:   Nicolas Viennot <Nicolas.Viennot@...sigma.com>
To:     Cyrill Gorcunov <gorcunov@...il.com>,
        Adrian Reber <areber@...hat.com>,
        Andy Lutomirski <luto@...nel.org>
CC:     Christian Brauner <christian.brauner@...ntu.com>,
        Eric Biederman <ebiederm@...ssion.com>,
        Pavel Emelyanov <ovzxemul@...il.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Andrei Vagin <avagin@...il.com>,
        Michał Cłapiński <mclapinski@...gle.com>,
        Kamil Yurtsever <kyurtsever@...gle.com>,
        "Dirk Petersen" <dipeit@...il.com>,
        Christine Flood <chf@...hat.com>,
        "Casey Schaufler" <casey@...aufler-ca.com>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Radostin Stoyanov <rstoyanov1@...il.com>,
        Serge Hallyn <serge@...lyn.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Sargun Dhillon <sargun@...gun.me>,
        Arnd Bergmann <arnd@...db.de>,
        "linux-security-module@...r.kernel.org" 
        <linux-security-module@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "selinux@...r.kernel.org" <selinux@...r.kernel.org>,
        Eric Paris <eparis@...isplace.org>,
        Jann Horn <jannh@...gle.com>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>
Subject: RE: [PATCH v2 1/3] capabilities: Introduce CAP_CHECKPOINT_RESTORE

>>  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);

> 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.
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.
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.

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 think Adrian added a CAP_CHECKPOINT_RESTORE on the map_files to avoid opening a can of worm. But I guess the cat is out of the bag now.

-Nico