Date:	Tue, 22 Apr 2014 06:49:51 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	David Herrmann <dh.herrmann@...il.com>
Cc:	Pavel Machek <pavel@....cz>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	"Theodore Ts'o" <tytso@....edu>
Subject: Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

On Tue, Apr 22, 2014 at 5:44 AM, David Herrmann <dh.herrmann@...il.com> wrote:
> Hi
>
> On Mon, Apr 21, 2014 at 6:22 PM, Andy Lutomirski <luto@...capital.net> wrote:
>> This patch does this:
>
> I can see _what_ the patch does, but your patch lacks any discussion
> _why_ it is needed. Can you provide at least one real example where
> this fixes a security issue?

Anyone who opens a file read-only and sends it over SCM_RIGHTS is
likely broken.  They may think that it's read-only, so it can't be
written, but this /proc/fd issue means that whoever receives it can
reopen it.

It's true that, if the inode doesn't allow the recipient write access,
then the recipient can't reopen, but there are lots of cases where the
inode can't reliably be expected not to allow write.  For example, the
inode could be unlinked, an O_TMPFILE file, a memfd handle, or in a
non-world-executable directory, and the file mode should be respected.

>
>> This may break userspace.  If so, I would guess that anything broken
>> by it is either an actual exploit or is so broken that it doesn't
>> deserve to continue working.  If it breaks something important, then
>> maybe it will need a sysctl.
>
> This patch breaks the following use-case:

This is totally broken.

>
> fd = open("/run", O_RDWR | O_TMPFILE);

Did you mean fd = open("/run", O_RDWR | O_TMPFILE, 0666)?  0600?
Because currently you don't actually know that you have write access
to the inode unless you have CAP_DAC_OVERRIDE.

This should fail to compile.  It doesn't because POSIX is dumb.

> sprintf(path, "/proc/self/fd/%d", fd);
> fd2 = open(buf, O_RDONLY);

And this is the whole point of this patch.

> sprintf(path, "/proc/self/fd/%d", fd2);
> linkat(AT_FDCWD, path, AT_FDCWD, "/run/some_lock_file", AT_FOLLOW_SYMLINK);

Seriously?  fd2, not fd?  I didn't intend to change flink behavior in
this patch, but you shouldn't be able to bypass file descriptor modes
by flinking the fd either.

Imagine that you gave fd2 to someone else.  Do you really want them
flinking and then reopening for write?

>
> I mean I explicitly create the object as _writable_ but then keep a
> read-only descriptor for debugging purposes (to make sure that the
> program no longer writes to the file). This is no security feature,
> but only a safety feature in case something goes wrong. But I still
> want to be able to create hard-links (I _do_ have write-permissions on
> the object/inode).

Then use fd for the flink step.

IMO it's an accident that this code ever worked.  The "linkable"
attribute is an inode flag for no particularly good reason, and IMO
your example indicates that it should have been a struct file flag
from day one.

If you are doing this, can you please fix it so it will keep working
even if the current flink insanity goes away?  Your code is already
screwed on current kernels, because you might accidentally create a
world-writable file in /run depending on what's in your third argument
register.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists