[RFC 0/2] Fix permission checks on open("/proc/self/fd/N", O_RDWR)

[RFC 0/2] Fix permission checks on open("/proc/self/fd/N", O_RDWR)

[Andy Lutomirski]

This is a prototype of a real fix for a longstanding issue.  See patch 2
for the full descripton.

Andy Lutomirski (2):
  fs,proc: Pass nameidata to proc_get_link implementations
  fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

 fs/namei.c            |  3 +++
 fs/proc/base.c        | 13 ++++++++-----
 fs/proc/fd.c          | 22 ++++++++++++++++++----
 fs/proc/internal.h    |  3 ++-
 include/linux/namei.h |  1 +
 5 files changed, 32 insertions(+), 10 deletions(-)

-- 
1.9.0

[RFC 1/2] fs,proc: Pass nameidata to proc_get_link implementations

[Andy Lutomirski]

proc_fd_link should respect fs modes, and it needs to know the open
mode to do so.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 fs/proc/base.c     | 13 ++++++++-----
 fs/proc/fd.c       |  3 ++-
 fs/proc/internal.h |  3 ++-
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2d696b0..b8eca09 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -171,7 +171,8 @@ static int get_task_root(struct task_struct *task, struct path *root)
 	return result;
 }
 
-static int proc_cwd_link(struct dentry *dentry, struct path *path)
+static int proc_cwd_link(struct nameidata *nd,
+			 struct dentry *dentry, struct path *path)
 {
 	struct task_struct *task = get_proc_task(dentry->d_inode);
 	int result = -ENOENT;
@@ -188,7 +189,8 @@ static int proc_cwd_link(struct dentry *dentry, struct path *path)
 	return result;
 }
 
-static int proc_root_link(struct dentry *dentry, struct path *path)
+static int proc_root_link(struct nameidata *nd,
+			  struct dentry *dentry, struct path *path)
 {
 	struct task_struct *task = get_proc_task(dentry->d_inode);
 	int result = -ENOENT;
@@ -1407,7 +1409,8 @@ static const struct file_operations proc_pid_set_comm_operations = {
 	.release	= single_release,
 };
 
-static int proc_exe_link(struct dentry *dentry, struct path *exe_path)
+static int proc_exe_link(struct nameidata *nd,
+			 struct dentry *dentry, struct path *exe_path)
 {
 	struct task_struct *task;
 	struct mm_struct *mm;
@@ -1441,7 +1444,7 @@ static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
 	if (!proc_fd_access_allowed(inode))
 		goto out;
 
-	error = PROC_I(inode)->op.proc_get_link(dentry, &path);
+	error = PROC_I(inode)->op.proc_get_link(nd, dentry, &path);
 	if (error)
 		goto out;
 
@@ -1485,7 +1488,7 @@ static int proc_pid_readlink(struct dentry * dentry, char __user * buffer, int b
 	if (!proc_fd_access_allowed(inode))
 		goto out;
 
-	error = PROC_I(inode)->op.proc_get_link(dentry, &path);
+	error = PROC_I(inode)->op.proc_get_link(NULL, dentry, &path);
 	if (error)
 		goto out;
 
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 0788d09..deca407 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -139,7 +139,8 @@ static const struct dentry_operations tid_fd_dentry_operations = {
 	.d_delete	= pid_delete_dentry,
 };
 
-static int proc_fd_link(struct dentry *dentry, struct path *path)
+static int proc_fd_link(struct nameidata *nd,
+			struct dentry *dentry, struct path *path)
 {
 	struct files_struct *files = NULL;
 	struct task_struct *task;
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 3ab6d14..ae7ff1d 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -51,7 +51,8 @@ struct proc_dir_entry {
 };
 
 union proc_op {
-	int (*proc_get_link)(struct dentry *, struct path *);
+	int (*proc_get_link)(struct nameidata *,
+			     struct dentry *, struct path *);
 	int (*proc_read)(struct task_struct *task, char *page);
 	int (*proc_show)(struct seq_file *m,
 		struct pid_namespace *ns, struct pid *pid,
-- 
1.9.0

Re: [RFC 1/2] fs,proc: Pass nameidata to proc_get_link implementations

[Al Viro]

On Mon, Apr 21, 2014 at 09:22:47AM -0700, Andy Lutomirski wrote:
> proc_fd_link should respect fs modes, and it needs to know the open
> mode to do so.

So pass it by value.  NAK in that form.

[RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Andy Lutomirski]

This patch does this:

    # (echo asdf >/proc/self/fd/3) 3</dev/null
    bash: /proc/self/fd/3: Permission denied

I'm not confident that my use of nd->flags is correct.  I'm also not
confident that this hooks in to path lookup in the right place.

It is not complete.  It doesn't handle FMODE_READ.  It also doesn't
handle FMODE_EXEC or FMODE_PATH, but the desired semantics there are
less clear.  For example, this should probably continue to work:

    (/proc/self/fd/3 Hello) 3</bin/echo

This patch does not do anything to /proc/self/exe.  It probably should.

Something like this can possibly fix the accidental flink-via-proc
thing, too.

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.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 fs/namei.c            |  3 +++
 fs/proc/fd.c          | 19 ++++++++++++++++---
 include/linux/namei.h |  1 +
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index c6157c8..3f01836 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2894,6 +2894,9 @@ static int do_last(struct nameidata *nd, struct path *path,
 		goto finish_open;
 	}
 
+	if (open_flag & (O_WRONLY | O_RDWR))
+		nd->flags |= LOOKUP_WRITE;
+
 	if (!(open_flag & O_CREAT)) {
 		if (nd->last.name[nd->last.len])
 			nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index deca407..ab3373c 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -139,6 +139,17 @@ static const struct dentry_operations tid_fd_dentry_operations = {
 	.d_delete	= pid_delete_dentry,
 };
 
+static int proc_may_follow(struct nameidata *nd, struct file *f)
+{
+	if (!nd)
+		return 0;  /* This is readlink, */
+
+	if ((nd->flags & LOOKUP_WRITE) && !(f->f_mode & FMODE_WRITE))
+		return -EACCES;
+
+	return 0;
+}
+
 static int proc_fd_link(struct nameidata *nd,
 			struct dentry *dentry, struct path *path)
 {
@@ -159,9 +170,11 @@ static int proc_fd_link(struct nameidata *nd,
 		spin_lock(&files->file_lock);
 		fd_file = fcheck_files(files, fd);
 		if (fd_file) {
-			*path = fd_file->f_path;
-			path_get(&fd_file->f_path);
-			ret = 0;
+			ret = proc_may_follow(nd, fd_file);
+			if (ret == 0) {
+				*path = fd_file->f_path;
+				path_get(&fd_file->f_path);
+			}
 		}
 		spin_unlock(&files->file_lock);
 		put_files_struct(files);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 492de72..5077201 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -51,6 +51,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
 #define LOOKUP_CREATE		0x0200
 #define LOOKUP_EXCL		0x0400
 #define LOOKUP_RENAME_TARGET	0x0800
+#define LOOKUP_WRITE		0x8000
 
 #define LOOKUP_JUMPED		0x1000
 #define LOOKUP_ROOT		0x2000
-- 
1.9.0

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Al Viro]

On Mon, Apr 21, 2014 at 09:22:48AM -0700, Andy Lutomirski wrote:

> +static int proc_may_follow(struct nameidata *nd, struct file *f)
> +{
> +	if (!nd)
> +		return 0;  /* This is readlink, */
> +
> +	if ((nd->flags & LOOKUP_WRITE) && !(f->f_mode & FMODE_WRITE))
> +		return -EACCES;
> +
> +	return 0;
> +}

And this is just plain wrong.  WTF are you making the traversal of symlink
in the middle of pathname dependent on the open flags?

NAK.

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Andy Lutomirski]

On Mon, Apr 21, 2014 at 9:30 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Apr 21, 2014 at 09:22:48AM -0700, Andy Lutomirski wrote:
>
>> +static int proc_may_follow(struct nameidata *nd, struct file *f)
>> +{
>> +     if (!nd)
>> +             return 0;  /* This is readlink, */
>> +
>> +     if ((nd->flags & LOOKUP_WRITE) && !(f->f_mode & FMODE_WRITE))
>> +             return -EACCES;
>> +
>> +     return 0;
>> +}
>
> And this is just plain wrong.  WTF are you making the traversal of symlink
> in the middle of pathname dependent on the open flags?

Can you give me a hint?  There are three cases that I need to
distinguish, I think:

1. readlink.  Currently handled by nd == NULL.  It's ugly, and I'll clean it up.

2. Traversal in the middle of a path.  This can be either literally in
the middle (e.g. "/proc/self/fd/3/something_else") or in a symlink
that's the last component of the literal path (e.g. "fd3null" where
"fd3null" is a symlink to "/proc/self/fd/3/null" and "null" is either
a file or a symlink to /dev/null).  I have the latter type wrong in
this patch.

3. Actually opening /proc/self/fd/N.  This can be direct or by opening
a symlink to /proc/self/fd/N.  I think I have this case correct.

What's the best way to fix this?  Should I be checking nd->depth?

--Andy

>
> NAK.



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Andy Lutomirski]

On Mon, Apr 21, 2014 at 10:00 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Apr 21, 2014 at 9:30 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Mon, Apr 21, 2014 at 09:22:48AM -0700, Andy Lutomirski wrote:
>>
>>> +static int proc_may_follow(struct nameidata *nd, struct file *f)
>>> +{
>>> +     if (!nd)
>>> +             return 0;  /* This is readlink, */
>>> +
>>> +     if ((nd->flags & LOOKUP_WRITE) && !(f->f_mode & FMODE_WRITE))
>>> +             return -EACCES;
>>> +
>>> +     return 0;
>>> +}
>>
>> And this is just plain wrong.  WTF are you making the traversal of symlink
>> in the middle of pathname dependent on the open flags?
>
> Can you give me a hint?  There are three cases that I need to
> distinguish, I think:
>
> 1. readlink.  Currently handled by nd == NULL.  It's ugly, and I'll clean it up.
>
> 2. Traversal in the middle of a path.  This can be either literally in
> the middle (e.g. "/proc/self/fd/3/something_else") or in a symlink
> that's the last component of the literal path (e.g. "fd3null" where
> "fd3null" is a symlink to "/proc/self/fd/3/null" and "null" is either
> a file or a symlink to /dev/null).  I have the latter type wrong in
> this patch.
>
> 3. Actually opening /proc/self/fd/N.  This can be direct or by opening
> a symlink to /proc/self/fd/N.  I think I have this case correct.
>
> What's the best way to fix this?  Should I be checking nd->depth?

No, I think that's wrong, too.  I think that will cause me to screw up
symlinks to /proc/self/fd/3.  What's the right way to tell that
follow_link is happening on the very last pathname component?

Hmm.  I wonder what happens, or even what should happen, if the file
descriptor is a symlink opened with O_PATH | O_NOFOLLOW.

--Andy

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Andy Lutomirski]

On Mon, Apr 21, 2014 at 10:04 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Apr 21, 2014 at 10:00 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Mon, Apr 21, 2014 at 9:30 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>> On Mon, Apr 21, 2014 at 09:22:48AM -0700, Andy Lutomirski wrote:
>>>
>>>> +static int proc_may_follow(struct nameidata *nd, struct file *f)
>>>> +{
>>>> +     if (!nd)
>>>> +             return 0;  /* This is readlink, */
>>>> +
>>>> +     if ((nd->flags & LOOKUP_WRITE) && !(f->f_mode & FMODE_WRITE))
>>>> +             return -EACCES;
>>>> +
>>>> +     return 0;
>>>> +}
>>>
>>> And this is just plain wrong.  WTF are you making the traversal of symlink
>>> in the middle of pathname dependent on the open flags?
>>
>> Can you give me a hint?  There are three cases that I need to
>> distinguish, I think:
>>
>> 1. readlink.  Currently handled by nd == NULL.  It's ugly, and I'll clean it up.
>>
>> 2. Traversal in the middle of a path.  This can be either literally in
>> the middle (e.g. "/proc/self/fd/3/something_else") or in a symlink
>> that's the last component of the literal path (e.g. "fd3null" where
>> "fd3null" is a symlink to "/proc/self/fd/3/null" and "null" is either
>> a file or a symlink to /dev/null).  I have the latter type wrong in
>> this patch.
>>
>> 3. Actually opening /proc/self/fd/N.  This can be direct or by opening
>> a symlink to /proc/self/fd/N.  I think I have this case correct.
>>
>> What's the best way to fix this?  Should I be checking nd->depth?
>
> No, I think that's wrong, too.  I think that will cause me to screw up
> symlinks to /proc/self/fd/3.  What's the right way to tell that
> follow_link is happening on the very last pathname component?
>
> Hmm.  I wonder what happens, or even what should happen, if the file
> descriptor is a symlink opened with O_PATH | O_NOFOLLOW.

Here's a test case:

https://github.com/amluto/procfd_test

It doesn't cover O_PATH yet.

Both my code and current kernels fail.

--Andy

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[David Herrmann]

Hi

On Mon, Apr 21, 2014 at 6:22 PM, Andy Lutomirski <luto@amacapital.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?

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

fd = open("/run", O_RDWR | O_TMPFILE);
sprintf(path, "/proc/self/fd/%d", fd);
fd2 = open(buf, O_RDONLY);
sprintf(path, "/proc/self/fd/%d", fd2);
linkat(AT_FDCWD, path, AT_FDCWD, "/run/some_lock_file", AT_FOLLOW_SYMLINK);

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

Thanks
David

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Andy Lutomirski]

On Tue, Apr 22, 2014 at 5:44 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Apr 21, 2014 at 6:22 PM, Andy Lutomirski <luto@amacapital.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

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[David Herrmann]

Hi

On Tue, Apr 22, 2014 at 3:49 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> 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.

I think it's safe to assume that any object you create is never
world-accessible. So the worst you can get is 0600. So if we now take
your example, your patch doesn't fix the problem at all. Imagine two
processes, $sender and $receiver. If the receiver runs as a different
user as the sender, it cannot open /proc/self/fd/ writable due to
0600. So the only problematic case is if both run as the same user.
However, in that case, the receiver can _always_ access
/proc/$sender/fd/ and thus still gain writable access to the object,
even if its own fd is read-only and your patch was applied. (ignoring
the fact that they can kill() and ptrace each other..)

Protecting world-accessible objects by hiding them is imho wrong. And
protecting users against themselves is even worse.

>>
>> fd = open("/run", O_RDWR | O_TMPFILE);
>
> Did you mean fd = open("/run", O_RDWR | O_TMPFILE, 0666)?  0600?

Sorry, I meant S_IWUSR | S_IRUSR (0600).

Thanks
David

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Andy Lutomirski]

On Tue, Apr 22, 2014 at 7:17 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Tue, Apr 22, 2014 at 3:49 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> 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.
>
> I think it's safe to assume that any object you create is never
> world-accessible. So the worst you can get is 0600.

Can you explain what you mean?  I think that it's completely *unsafe*
to make this assumption unless you actually take some explicit action
to make sure it's correct.

> So if we now take
> your example, your patch doesn't fix the problem at all. Imagine two
> processes, $sender and $receiver. If the receiver runs as a different
> user as the sender, it cannot open /proc/self/fd/ writable due to
> 0600. So the only problematic case is if both run as the same user.
> However, in that case, the receiver can _always_ access
> /proc/$sender/fd/ and thus still gain writable access to the object,
> even if its own fd is read-only and your patch was applied. (ignoring
> the fact that they can kill() and ptrace each other..)

Incorrect.  That is exactly what my patch changes.

>
> Protecting world-accessible objects by hiding them is imho wrong. And
> protecting users against themselves is even worse.

Protecting users against themselves (i.e. other things with the same
uid) when they create namespaces and use seccomp filters is important.

--Andy

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[David Herrmann]

Hi

On Tue, Apr 22, 2014 at 4:33 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Tue, Apr 22, 2014 at 7:17 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> I think it's safe to assume that any object you create is never
>> world-accessible. So the worst you can get is 0600.
>
> Can you explain what you mean?  I think that it's completely *unsafe*
> to make this assumption unless you actually take some explicit action
> to make sure it's correct.

Which kernel-interface creates world-writable objects if a reasonable
umask like 022 is set?

>> So if we now take
>> your example, your patch doesn't fix the problem at all. Imagine two
>> processes, $sender and $receiver. If the receiver runs as a different
>> user as the sender, it cannot open /proc/self/fd/ writable due to
>> 0600. So the only problematic case is if both run as the same user.
>> However, in that case, the receiver can _always_ access
>> /proc/$sender/fd/ and thus still gain writable access to the object,
>> even if its own fd is read-only and your patch was applied. (ignoring
>> the fact that they can kill() and ptrace each other..)
>
> Incorrect.  That is exactly what my patch changes.

Are you sure? Note I wrote /proc/$sender/fd/ not /proc/$receiver/fd/.
The lookup on /proc/$sender/fd/ is done with the file of the _sender_,
which obviously is writable.

Thanks
David

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Andy Lutomirski]

On Tue, Apr 22, 2014 at 8:03 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Tue, Apr 22, 2014 at 4:33 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Tue, Apr 22, 2014 at 7:17 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>> I think it's safe to assume that any object you create is never
>>> world-accessible. So the worst you can get is 0600.
>>
>> Can you explain what you mean?  I think that it's completely *unsafe*
>> to make this assumption unless you actually take some explicit action
>> to make sure it's correct.
>
> Which kernel-interface creates world-writable objects if a reasonable
> umask like 022 is set?

That is an incredibly large "if".  Does Xorg change its umask when it
starts?  Whenever you write a setuid program, do you reset the umask?
Will every user of memfd_create who makes a read-only fd pointing at
the same thing remember to reset the umask?  How about libraries that
*can't* safely mess with umask because it's not thread-local?

>
>>> So if we now take
>>> your example, your patch doesn't fix the problem at all. Imagine two
>>> processes, $sender and $receiver. If the receiver runs as a different
>>> user as the sender, it cannot open /proc/self/fd/ writable due to
>>> 0600. So the only problematic case is if both run as the same user.
>>> However, in that case, the receiver can _always_ access
>>> /proc/$sender/fd/ and thus still gain writable access to the object,
>>> even if its own fd is read-only and your patch was applied. (ignoring
>>> the fact that they can kill() and ptrace each other..)
>>
>> Incorrect.  That is exactly what my patch changes.
>
> Are you sure? Note I wrote /proc/$sender/fd/ not /proc/$receiver/fd/.
> The lookup on /proc/$sender/fd/ is done with the file of the _sender_,
> which obviously is writable.

Sorry, I misread that.  This is a problem that people who design
non-uid-manipulating sandboxes have to handle.  Or they can use pid
namespaces.

--Andy

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Pavel Machek]

On Tue 2014-04-22 16:17:57, David Herrmann wrote:
> Hi
> 
> On Tue, Apr 22, 2014 at 3:49 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> > 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.
> 
> I think it's safe to assume that any object you create is never
> world-accessible. So the worst you can get is 0600. So if we now take
> your example, your patch doesn't fix the problem at all. Imagine two
> processes, $sender and $receiver. If the receiver runs as a different
> user as the sender, it cannot open /proc/self/fd/ writable due to
> 0600. So the only problematic case is if both run as the same user.
> However, in that case, the receiver can _always_ access
> /proc/$sender/fd/ and thus still gain writable access to the object,
> even if its own fd is read-only and your patch was applied. (ignoring
> the fact that they can kill() and ptrace each other..)
> 
> Protecting world-accessible objects by hiding them is imho wrong. And
> protecting users against themselves is even worse.

See the link for example where this matters in multi-user setting.

Agreed, protecting file using its directory's permissions is not
practice to be recommended, but we should not be breaking it. (And we
are not breaking it if /proc is not mounted...)

Enforcing read-only on file descriptor is IMO useful, and being able
to work around it using /proc/self/fd is a security problem...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Pavel Machek]

Hi!

> On Mon, Apr 21, 2014 at 6:22 PM, Andy Lutomirski <luto@amacapital.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?

Such as here?

http://www.securityfocus.com/archive/1/507386

> > 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:
> 
> fd = open("/run", O_RDWR | O_TMPFILE);
> sprintf(path, "/proc/self/fd/%d", fd);
> fd2 = open(buf, O_RDONLY);

You meant open(path, ) here?

> sprintf(path, "/proc/self/fd/%d", fd2);
> linkat(AT_FDCWD, path, AT_FDCWD, "/run/some_lock_file", AT_FOLLOW_SYMLINK);
> 
> 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).

Does some real code do it? I believe this deserves to be broken -- you
explicitely opened that read-only...

     	  							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[David Herrmann]

Hi

On Tue, Apr 22, 2014 at 4:31 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Such as here?
>
> http://www.securityfocus.com/archive/1/507386

Thanks, that's the first real example someone mentioned.

Quoted from your link:

> The reopen does check the inode permission, but it does not require
> you have any reachable path to the file. Someone _might_ use that as
> a traditional unix security mechanism, but if so it's probably quite rare.

In other words, the bug you describe is that /proc/pid/fd/ allows
access to objects without a reachable path to the only _real_
filesystem link. But isn't the same true for openat()?

Thanks
David

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Andy Lutomirski]

On Tue, Apr 22, 2014 at 8:19 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Tue, Apr 22, 2014 at 4:31 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> Such as here?
>>
>> http://www.securityfocus.com/archive/1/507386
>
> Thanks, that's the first real example someone mentioned.
>
> Quoted from your link:
>
>> The reopen does check the inode permission, but it does not require
>> you have any reachable path to the file. Someone _might_ use that as
>> a traditional unix security mechanism, but if so it's probably quite rare.
>
> In other words, the bug you describe is that /proc/pid/fd/ allows
> access to objects without a reachable path to the only _real_
> filesystem link. But isn't the same true for openat()?

I don't think so.  openat doesn't work on fds for things that aren't
directories.

--Andy

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[David Herrmann]

Hi

On Tue, Apr 22, 2014 at 5:24 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Tue, Apr 22, 2014 at 8:19 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> In other words, the bug you describe is that /proc/pid/fd/ allows
>> access to objects without a reachable path to the only _real_
>> filesystem link. But isn't the same true for openat()?
>
> I don't think so.  openat doesn't work on fds for things that aren't
> directories.

Sorry, I wasn't precise enough: I meant the same 'leak' occurs if you
keep a dir-fd on the directory in question _before_ it is set to 0600.
Just like the example race keeps a file-fd to the file in question. So
after the directory is set to 0600 you can use that dir-fd via
openat() to avoid the whole path-lookup just like you do it via /proc.

Thanks
David

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Andy Lutomirski]

On Tue, Apr 22, 2014 at 9:44 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Tue, Apr 22, 2014 at 5:24 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Tue, Apr 22, 2014 at 8:19 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>> In other words, the bug you describe is that /proc/pid/fd/ allows
>>> access to objects without a reachable path to the only _real_
>>> filesystem link. But isn't the same true for openat()?
>>
>> I don't think so.  openat doesn't work on fds for things that aren't
>> directories.
>
> Sorry, I wasn't precise enough: I meant the same 'leak' occurs if you
> keep a dir-fd on the directory in question _before_ it is set to 0600.
> Just like the example race keeps a file-fd to the file in question. So
> after the directory is set to 0600 you can use that dir-fd via
> openat() to avoid the whole path-lookup just like you do it via /proc.
>

I think this doesn't work -- fds pointing at directories don't capture
execute permission the way fds pointing at files can capture write
permission.  This is similar to the way that ls fails if you chmod 000
'.'.  Yes, it's weird.

--Andy

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Pavel Machek]

On Tue 2014-04-22 17:19:42, David Herrmann wrote:
> Hi
> 
> On Tue, Apr 22, 2014 at 4:31 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > Such as here?
> >
> > http://www.securityfocus.com/archive/1/507386
> 
> Thanks, that's the first real example someone mentioned.
> 
> Quoted from your link:
> 
> > The reopen does check the inode permission, but it does not require
> > you have any reachable path to the file. Someone _might_ use that as
> > a traditional unix security mechanism, but if so it's probably quite rare.
> 
> In other words, the bug you describe is that /proc/pid/fd/ allows
> access to objects without a reachable path to the only _real_
> filesystem link. But isn't the same true for openat()?

I don't think openat helps you. This is what we are talking about, it
is easy to reproduce. Can you reproduce it without /proc mounted?

I think that chmod 700 . should stop you. Openat seems no worse than
just placing cwd there...

pavel@toy:/tmp$ uname -a
Linux toy.ucw.cz 2.6.32-rc3 #21 Mon Oct 19 07:32:02 CEST 2009 armv5tel
GNU/Linux
pavel@toy:/tmp mkdir my_priv; cd my_priv
pavel@toy:/tmp/my_priv$ echo this file should never be writable >
unwritable_file
# lock down directory
pavel@toy:/tmp/my_priv$ chmod 700 .
# relax file permissions, directory is private, so this is safe
# check link count on unwritable_file. We would not want someone 
# to have a hard link to work around our permissions, would we?
pavel@toy:/tmp/my_priv$ chmod 666 unwritable_file 
pavel@toy:/tmp/my_priv$ cat unwritable_file 
this file should never be writable
pavel@toy:/tmp/my_priv$ cat unwritable_file 
got you
# Security problem here
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[David Herrmann]

Hi

On Tue, Apr 22, 2014 at 8:58 PM, Pavel Machek <pavel@ucw.cz> wrote:
> I don't think openat helps you. This is what we are talking about, it
> is easy to reproduce. Can you reproduce it without /proc mounted?
>
> I think that chmod 700 . should stop you. Openat seems no worse than
> just placing cwd there...

Example1:
$ mkdir -p subdir/next
$ chmod 000 subdir
$ touch subdir/next/test
=> EACCES
$ cd subdir
=> EACCES

Example2:
$ mkdir -p subdir/next
$ cd subdir/next
$ chmod 000 ..
$ touch test
=> SUCCESS

This is the exact same situation. The filesystem tree is exactly the
same in both situations, but in the first example CWD is outside of
"subdir", in the second example CWD is inside of "subdir". Thus, they
can create files in that directory, even though they have no access to
_any_ absolute path to that directory.

This is the exact same race that you describe via /proc/self/fd/. But
instead of keeping a ref to the dir via CWD, in your example you keep
the ref via a FD in that exact same directory and access it via /proc.

(Hint: instead of using CWD, you can also keep an FD via open(O_PATH)
and pass it to openat())

Thanks
David

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Andy Lutomirski]

On Tue, Apr 22, 2014 at 2:31 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Tue, Apr 22, 2014 at 8:58 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> I don't think openat helps you. This is what we are talking about, it
>> is easy to reproduce. Can you reproduce it without /proc mounted?
>>
>> I think that chmod 700 . should stop you. Openat seems no worse than
>> just placing cwd there...
>
> Example1:
> $ mkdir -p subdir/next
> $ chmod 000 subdir
> $ touch subdir/next/test
> => EACCES
> $ cd subdir
> => EACCES
>
> Example2:
> $ mkdir -p subdir/next
> $ cd subdir/next
> $ chmod 000 ..
> $ touch test
> => SUCCESS
>
> This is the exact same situation. The filesystem tree is exactly the
> same in both situations, but in the first example CWD is outside of
> "subdir", in the second example CWD is inside of "subdir". Thus, they
> can create files in that directory, even though they have no access to
> _any_ absolute path to that directory.
>
> This is the exact same race that you describe via /proc/self/fd/. But
> instead of keeping a ref to the dir via CWD, in your example you keep
> the ref via a FD in that exact same directory and access it via /proc.
>
> (Hint: instead of using CWD, you can also keep an FD via open(O_PATH)
> and pass it to openat())

So what?

It's well-known that the execute bit is only checked when you look
something up, and it's not checked all the way back to the root.  It's
not well known, nor is it POSIX, that you can fudge it via proc.

--Andy

Re: [RFC 2/2] fs,proc: Respect FMODE_WRITE when opening /proc/pid/fd/N

[Pavel Machek]

On Tue 2014-04-22 23:31:11, David Herrmann wrote:
> Hi
> 
> On Tue, Apr 22, 2014 at 8:58 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > I don't think openat helps you. This is what we are talking about, it
> > is easy to reproduce. Can you reproduce it without /proc mounted?
> >
> > I think that chmod 700 . should stop you. Openat seems no worse than
> > just placing cwd there...
> 
> Example1:
> $ mkdir -p subdir/next
> $ chmod 000 subdir
> $ touch subdir/next/test
> => EACCES
> $ cd subdir
> => EACCES
> 
> Example2:
> $ mkdir -p subdir/next
> $ cd subdir/next
> $ chmod 000 ..
> $ touch test
> => SUCCESS
> 
> This is the exact same situation. The filesystem tree is exactly the
> same in both situations, but in the first example CWD is outside of
> "subdir", in the second example CWD is inside of "subdir". Thus, they
> can create files in that directory, even though they have no access to
> _any_ absolute path to that directory.
> 
> This is the exact same race that you describe via /proc/self/fd/. But
> instead of keeping a ref to the dir via CWD, in your example you keep
> the ref via a FD in that exact same directory and access it via
> /proc.

Yes, that's how permissions work. You snipped my example. Can you show
how to write to "unwritable_file" below without /proc? Because I
believe I got my chmods right.

pavel@toy:/tmp$ uname -a
Linux toy.ucw.cz 2.6.32-rc3 #21 Mon Oct 19 07:32:02 CEST 2009 armv5tel
GNU/Linux
pavel@toy:/tmp mkdir my_priv; cd my_priv
pavel@toy:/tmp/my_priv$ echo this file should never be writable >
unwritable_file
# lock down directory
pavel@toy:/tmp/my_priv$ chmod 700 .
# relax file permissions, directory is private, so this is safe
# check link count on unwritable_file. We would not want someone
# to have a hard link to work around our permissions, would we?
pavel@toy:/tmp/my_priv$ chmod 666 unwritable_file
pavel@toy:/tmp/my_priv$ cat unwritable_file
this file should never be writable
pavel@toy:/tmp/my_priv$ cat unwritable_file
got you
# Security problem here


> (Hint: instead of using CWD, you can also keep an FD via open(O_PATH)
> and pass it to openat())

Feel free to use openat(), too.

Thanks,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html