Fix permission checks on open("/proc/self/fd/N", O_RDWR)

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

fs/proc/fd.c static const struct dentry_operations tid_fd_dentry_operations = { · line 151

Quoted 10 lines · Andy Lutomirski

fs/proc/fd.c +10 -0

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

Quoted 15 lines · Al Viro

fs/proc/fd.c static const struct dentry_operations tid_fd_dentry_operations = { · line 151

Quoted 10 lines · Andy Lutomirski

fs/proc/fd.c +10 -0

+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?

Quoted 32 lines · Andy Lutomirski

Quoted 15 lines · Al Viro

fs/proc/fd.c static const struct dentry_operations tid_fd_dentry_operations = { · line 151

Quoted 10 lines · Andy Lutomirski

fs/proc/fd.c +10 -0

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

Quoted 1 line

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.

This patch does this:

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.

Hi

Quoted 1 line · Andy Lutomirski

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?

Quoted 5 lines

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

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.

Quoted 2 lines

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

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

Hi

Quoted 5 lines · Andy Lutomirski

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.

Quoted 5 lines

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.

Quoted 2 lines · David Herrmann

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.

Quoted 9 lines

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.

Hi

Quoted 4 lines · Andy Lutomirski

Quoted 2 lines · David Herrmann

I think it's safe to assume that any object you create is never world-accessible. So the worst you can get is 0600.

Quoted 3 lines

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?

Quoted 11 lines

Quoted 10 lines

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

Quoted 1 line

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.

Hi

Quoted 10 lines · Andy Lutomirski

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.

Quoted 1 line

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?

Quoted 4 lines

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

Such as here?

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

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.

Hi

Quoted 3 lines · Pavel Machek

Such as here?

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

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

Quoted from your link:

Quoted 3 lines

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()?

Quoted 3 lines · David Herrmann

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.

Hi

Quoted 5 lines · Andy Lutomirski

Quoted 3 lines · David Herrmann

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()?

Quoted 2 lines

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.

Hi

Quoted 3 lines · Pavel Machek

Such as here?

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

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

Quoted from your link:

Quoted 3 lines

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

Hi

Quoted 5 lines · Pavel Machek

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

Hi

Quoted 5 lines · Pavel Machek

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