vfs: Tighten up linkat(..., AT_EMPTY_PATH)

There have long been two ways to ask the kernel to create a new hardlink to the inode represented by an fd: linkat(..., AT_EMPTY_PATH) and AT_SYMLINK_FOLLOW on /proc/self/fd/N. The latter has no particular security restrictions, but the former required privilege until:

commit bb2314b47996491bbc5add73633905c3120b6268
Author: Andy Lutomirski <[email protected]>
Date: Thu Aug 1 21:44:31 2013 -0700

fs: Allow unprivileged linkat(..., AT_EMPTY_PATH) aka flink

Spender pointed out that there could be code that passes an fd to an untrusted, chrooted process, and allowing that process to flink the fd could be dangerous. (I'm not aware of any actual known attack.)

So let's be careful for now: only allow linkat(..., AT_EMPTY_PATH) if the target is I_LINKABLE. I_LINKABLE is only set for inodes created by O_TMPFILE without O_EXCL, which is an explicit request for flinkability.

---

Changes from v1: Removed an unnecessary spin_lock.

 fs/namei.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

index 89a612e..9802d51 100644
@@ -3652,6 +3652,26 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de
 }
 
 /*
+ * flink is dangerous.  For now, only allow it when specifically requested
+ * or when the caller is privileged.
+ *
+ * NB: This is not currently enforced for AT_SYMLINK_FOLLOW on procfs.
+ *     Fixing that would be intrusive and could break something.
+ */
+static bool may_flink(const struct path *path)
+{
+	struct inode *inode = path->dentry->d_inode;
+
+	/*
+	 * This is racy: I_LINKABLE could be cleared between this check
+	 * and the actual link operation.  This is odd but not a security
+	 * problem: the caller could get the same effect by flinking once
+	 * and then using normal link(2) to create a second link.
+	 */
+	return (inode->i_state & I_LINKABLE) || capable(CAP_DAC_READ_SEARCH);
+}
+
+/*
  * Hardlinks are often used in delicate situations.  We avoid
  * security-related surprises by not following symlinks on the
  * newname.  --KAB

@@ -3670,10 +3690,7 @@ SYSCALL_DEFINE5(linkat, int, olddfd, const char __user *, oldname,
 
 	if ((flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
 		return -EINVAL;
-	/*
-	 * Using empty names is equivalent to using AT_SYMLINK_FOLLOW
-	 * on /proc/self/fd/<fd>.
-	 */
+
 	if (flags & AT_EMPTY_PATH)
 		how = LOOKUP_EMPTY;
 

@@ -3684,6 +3701,11 @@ retry:
 	if (error)
 		return error;
 
+	if ((flags & AT_EMPTY_PATH) && !may_flink(&old_path)) {
+		error = -EPERM;
+		goto out;
+	}
+
 	new_dentry = user_path_create(newdfd, newname, &new_path,
 					(how & LOOKUP_REVAL));
 	error = PTR_ERR(new_dentry);

1.8.3.1

OK no HTML from me here, so re-adding the 3 lists if that can help reaching an end faster on this stuff.

That's my understanding as well since it's what replaces /proc/self/fd/ which is he difference in this case. However AT_EMPTY_PATH is currently limited to linkat(2), fstatat(2) and name_to_handle_at(2) from what I'm seeing, so it looks like only linkat would permit the calling process to gain more permissions by using it when it has no /proc access.

In my understanding, it does not necessarily need to klil that thing to get an issue. Let's imagine a completely made up example. A malware scanner on a proxy uses sandboxes to analyse suspicious contents. For this it holds a read-only mmap to the signature database, forks and chroots the scanner process to avoid any risk of getting fooled by the malicious code. If the malicious code manages to execute some code in the context of the sandboxed scanner that still has access to the signature db, it could use linkat(AT_EMPTH_PATH) to this fd to recreate a file in the chroot, and reopen it R/W to modify it and remove any trace of its signature.

Sure it's a bit far-fetched, but I think that between this and the most trivial case, there might be a few real world cases. Also I still feel concerned by the ability for an unprivileged process to create device nodes in a chroot using this. I don't have any immediate impact in mind but I tend to design my chroots so that I'm sure there is no way to get a /dev there, and this would leave me a bit of doubt.

I agree. A mount option would be great for some constrained environments.

:-)

Willy

So I really detest this just because it's such a special case. Now this is only useful for that one special case, and the thing very fundamentally checks that one special case in a place that is impossible to check for the /proc case, so the proc case remains totally separate.

Which just bothers me.

I think we could easily at least allow the "file->f_creds == current->creds" case (and yes, I literally mean comparing the pointers
- not only is it cheaper, but it literally means "nothing odd has happened in between opening and the lookup").

And I'm wondering if we shouldn't actually do that at "path_init" time. Right now the code says:

                /* Caller must check execute permissions on the
starting path component */
                struct fd f = fdget_raw(dfd);

and then uses the struct file mindlessly.

I'm wondering if we should just do some validation in that place, and say:

- for directories, we require exec permissions here
- for everything else, we require that f->f_cred == current->cred check.

I dunno. But that I_LINKABLE thing just bothers me. It screams "I'm hacky" to me.

Linus

I agreed, simply because the condition here is different from the one in /proc.

I have read some code last evening to try to understand how /proc/pid/fd entries were granted access to various processes, because I would love to see the same condition being used in both places. Unfortunately, it's beyond my skills, and I stopped after my random attempts gave me some panics.

Willy

Agreed. It sucks. See below (the bottom).

I thought about that, but it won't catch chroot. (Admittedly, chroot without changing creds is asking for trouble.)

Does this work for the procfs case? As far as I understand it (which isn't saying much), it goes through the symlink-following path.

What if we added another field to struct nameidata that's indicates what restrictions need to be enforced when following magical symlinks and then enforcing them when nd_jump_link gets used. (There are only two of these, both in procfs.)

Then open could check that the original fd was opened for a superset of the requested permissions (or that the caller has CAP_DAC_OVERRIDE), linkat could check whatever it feels like checking, etc.

This would allow all of these issues to be fixed for real (controlled by sysctl, presumably).

--Andy

Andy Lutomirski
AMA Capital Management, LLC

Right. The /proc case is still separate, and we really should do something about that too. But again, I don't think I_LINKABLE is the thing to use there either. We probably should tighten up the magic /proc follow-link a lot.

Yes, I think that might be just the kind of thing to do. Except some tightening could well be quite regardless of any extra flags.

Linus

What's the point of nd_jump_link anyway? The only way I can think of for a magic symlink in /proc to point to another symlink is to open a symlink with O_PATH | O_NOFOLLOW. Actually trying to use the resulting link in /proc results in -ELOOP. (Even just trying to open a normal symlink with O_NOFOLLOW and without O_PATH results in -ELOOP.)

It might be a lot simpler to rig up nd_jump_link to immediately terminate lookup and let the callers (or the outer level of lookup) deal with it. That way the checks could be (more) easily unified with AT_EMPTY_PATH.

--Andy

Andy Lutomirski
AMA Capital Management, LLC

It's not only that, it also supports sockets and pipes that you can access via /proc/pid/fd and not via a real symlink which would try to open eg "pipe:[23456]" instead of the real file. So you can't get rid of it without breaking existing apps (starting with your shell for which /dev/stdin is a link to /proc/self/fd/0 for example).

Willy

Let me rephrase that: why do we allow these types of lookup to recurse like normal symlinks? I'm proposing that these links immediately terminate lookup and return back to user_path_at_empty, which can, in turn, do an extra check to see if the inode that was found is one of these magic inodes (or checks to see if nd_jump_link was called). user_path_at_empty could then enforce LOOKUP_EMPTY-list restrictions (or the caller could).

--Andy

It can nest *inside* a regular symlink. So there should not be any recursion of pure /proc style symlink jumps, but they live within the nesting that is normal symlink behavior.

Linus

Sure. But aren't they always last?

With the current code structure, trying to enforce some kind of security restriction in the middle of lookup seems really unpleasant. But if the final step is to verify one of these links and then either reject or end lookup right there, it'll be (IMO) much easier to understand.

--Andy

Andy Lutomirski
AMA Capital Management, LLC

What do you mean? I'd say that the /proc lookup is always *innermost*. Which means that it certainly cannot bail out, since there are many levels of nesting outside of it.

If it's conditional (ie "linkat behaves differently from openat"), it certainly means that we'd have to pass in that info in annoying ways.

But having unconditional rules like "you can only follow the proc link if you have CAP_SEARCH _or_ you match the file->f_cred" doesn't sound too painful.

Linus

I was thinking iteratively, so I said "last" instead of "innermost".

Hmm. That depends on whether things like I_LINKABLE should be considered. We might also want to ban open("/proc/self/fd/3", O_RDWR) if !CAP_DAC_READ_SEARCH and fd wasn't opened with O_RDWR. Both of those will require passing information in or out.

I'll see how nasty this ends up being. (This may take awhile -- I'm not at all familiar with this code, and this is at best a minor side project for me.)

--Andy

Nope. All we need to pass is one more LOOKUP_... Add

	if (unlikely(nd->last_type == LAST_BIND)) {
		if ((nd->flags & LOOKUP_BLAH) && !may_flink(...)) {
			terminate_walk(nd);
			return -EINVAL;
		}
	}

in the beginning of lookup_last() and pass LOOKUP_BLAH in flags when linkat() calls user_path_at(). That will affect *only* the terminal symlinks and cost nothing in all normal cases. The same check can bloody well go into path_init() - take

                if (*name) {
                        if (!can_lookup(dentry->d_inode)) {
                                fdput(f);
                                return -ENOTDIR;
                        }
                }
in there and slap
		else {
			if ((flags & LOOKUP_BLAH) && !may_flink(...)) {
                                fdput(f);
				return -EINVAL;
			}
		}

after it.

OK, let me summarize these threads so far:
* restrictions for flink() are needed and they'd better be
consistent for AT_SYMLINK_FOLLOW + /proc/<pid>/fd/<n> and simply passing the descriptor as dfd.
* CAP_DAC_OVERRIDE is sufficient; so should be O_TMPFILE used
to open that sucker.
* lookup_last() is the natural place for catching the case
of following a trailing procfs symlink - it can be done very cheaply there.

FWIW, I'm tempted to try the following trick:
* introduce FMODE_FLINK in file->f_mode; O_TMPFILE would set it,
unless O_EXCL is present.
* introduce LOOKUP_LINK, to be passed by sys_linkat() when
resolving the target.
* have path_init() called with empty pathname and LOOKUP_LINK in
flags do checks for FMODE_FLINK or CAP_DAC_OVERRIDE
* have ->proc_get_link() report whether the target is linkable
(either as bool * or by returning 1 instead of 0). After the call of ->proc_get_link() check that and set nd->last_type to LAST_BIND_LINKABLE. Note that *all* places looking at ->last_type treat LAST_BIND as "none of the above" - we never compare with it, so splitting it in two wouldn't break anything.
* have lookup_last() check if LOOKUP_LINK is present and ->last_type
is LAST_BIND; fail unless we have CAP_DAC_OVERRIDE.

AFAICS, it gets more or less sane behaviour; additionally, it makes possible to introduce explicit "I want that descriptor to be suitable for flink()" open(2) flag - that would require teaching do_last() about LOOKUP_LINK, making it check for CAP_DAC_OVERRIDE if it sees LAST_BIND / LOOKUP_LINK, same as lookup_last() above (we obviously want to avoid the possibility to take a non-flinkable descriptor and use it to reopen the sucker in flinkable way).

Alternatively we can revert "fs: Allow unprivileged linkat(..., AT_EMPTY_PATH) aka flink" for the time being. flink() is certainly an awful mess and I seriously regret touching it ;-/

Comments? Hell, maybe somebody even has printable ones - stranger things have happened...

I think this is more screwed up than just flink and open. For example:

$ echo 'WTF' >test
$ truncate -s 1 /proc/self/fd/3 3<test
$ cat test
W$

IMO that should have failed.

In an ideal world (I think) ffrob(N), frobat(N, "", AT_EMPTY_PATH), and frobat(AT_FDCWD, "/proc/self/fd/N) should generally do the same thing. This includes flink (even though the flink variant doesn't exist). open is a bit special.

Of course, we're rather inconsistent with AT_EMPTY_PATH. utimensat accepts a null filename instead of a blank filename, and it doesn't appear to check the file mode at all.

Sigh.

--Andy

Andy Lutomirski
AMA Capital Management, LLC

Why? truncate() always follows links, so what's the problem with that one? That you get checks of truncate() and not ftruncate()?

What about the cases where frob() and ffrob() check for different things?

The same as the issue with all these other things: the fd might have survived a privilege drop or been passed through exec or SCM_RIGHTS, and the holder of the fd might not be able to see the inode.

For example, suppose a daemon creates a file with O_TMPFILE | O_RDWR. Then it does open("/proc/self/fd/N", O_RDONLY) to get a read-only fd for the same temporary file. It passes that fd to something else. It's rather surprising that the recipient would be able to truncate it using /proc/self/fd when it couldn't ftruncate it due to its being O_RDONLY.

(Of course, this can be worked around by setting the mode to 0644, but I doubt that everyone will get that right.)

I'll go out on a limb and say that every single case where ffrob has a check that frob("/proc/self/fd/N") doesn't is wrong. Maybe we're stuck with them for backwards compatibility, but that doesn't mean they're good ideas.

--Andy

[ .. snipped .. ]

Yes. I think we should do this, but I think we should also look at what _other_ LOOKUP_xyz we should do for the /proc case.

For the read-only fd case, we should have a LOOKUP_WRITE flag, and return -EPERM if an operation is a write, and we terminate in that LAST_BIND case.

That would catch the truncate() case, but also the "open a read-only fd for write or O_TRUNC" case.

Anything else? What other path operations matter that follow links than truncate(), link() and open()?

Linus

Timestamp updates, chmod/chown, xattr mess...

Ok, so that's just too much details.

So I'll just go back to square one, and wonder if we could/should just make the rule be that in order to be in that LAST_BIND case, you really have to have f_cred match your own credentials. Or have CAP_SEARCH.

And just not have any new LOOKUP_xyz flags at all. No special cases, no different semantics for different ops, just check f_cred. Because if you had the permissions to do the original open (ie f_cred matches your current credentials), then that shows that you originally had all the pathname permissions, and you are still the same person.

And yeah, you may have opened in for reading (so the file is technically read-only), but obviously we're re-doing all the inode permission checks anyway, so the only thing /proc/<pid>/fd/<n> really gave you was the path traversal. So we shouldn't bother with "the file descriptor is only readable", because that is simply *irrelevant*.

So it means that the case Andi brought up (truncating or open-for-write a fd that we only had open for reading) would continue to be allowed, because while it "sounds odd", there is no actual problem.

And CAP_SEARCH is very much about that path lookup again. So it's consistent with the notion that "ok, you may do odd things to file descriptors through /proc, but we check that you cannot avoid the pathname lookup rules".

And then we do exactly the same to flink(). So then we're all consistent again. Not the consistency Andy worried about, but that's the consistency that was was the security worries with flink are all about. Because the issues with the "use the file descriptor, not the path to the file descriptor" really are *not* about the endpoint itself (since we will re-do the permission check for that particular inode anyway), but about the path leading up to that end-point.

Linus

Nope. That doesn't work. It breaks the chrome sandboxing.

Right now, following a /proc fd symlink requires ptrace access to the process. Which is actually pretty strict, and makes sense. But it does mean that there are other capabilities than CAP_DAC_READ_SEARCH at play.

I'm playing with a patch that then in addition to the ptrace check *also* requires that the file was opened with the same credentials as the follower _or_ the task being followed. I'll see if that works out.

Linus

Chrome sandboxing still _works_ with the attached patch, but there's a few audit messages about the sandbox being denied dac_read_search. And I'm really not very happy with this anyway.

So I'm going to send it out (using email to archive things and see if anybody has any comments), and then forget about it.

Linus

 fs/proc/fd.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

index 0ff80f9b930f..7a97dfbb4217 100644
@@ -137,6 +137,33 @@ static const struct dentry_operations tid_fd_dentry_operations = {
 	.d_delete	= pid_delete_dentry,
 };
 
+static inline int match_cred(const struct cred *a, const struct cred *b)
+{
+	return a->fsuid == b->fsuid && a->fsgid == b->fsgid;
+}
+
+/*
+ * To get here, we have ptrace() access to the task in question.
+ *
+ * But we still require that the file itself was either opened by that
+ * task or by ourselves, or that we have path search capability.
+ */
+static int get_fd_path(struct task_struct *task, struct file *file, struct path *path)
+{
+	if (!match_cred(file->f_cred, current_cred())) {
+		int match;
+
+		rcu_read_lock();
+		match = match_cred(file->f_cred, __task_cred(task));
+		rcu_read_unlock();
+		if (!match && !capable(CAP_DAC_READ_SEARCH))
+			return -EACCES;
+	}
+	*path = file->f_path;
+	path_get(&file->f_path);
+	return 0;
+}
+
 static int proc_fd_link(struct dentry *dentry, struct path *path)
 {
 	struct files_struct *files = NULL;

@@ -155,11 +182,8 @@ static int proc_fd_link(struct dentry *dentry, struct path *path)
 
 		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;
-		}
+		if (fd_file)
+			ret = get_fd_path(task, fd_file, path);
 		spin_unlock(&files->file_lock);
 		put_files_struct(files);
 	}

I'm still not thrilled with this approach. I'll see if I can find a clean way to use fmode later today.

--Andy

Andy Lutomirski
AMA Capital Management, LLC

This is an experiment to see if we can get nice semantics for all syscalls that either follow symlinks or allow AT_EMPTY_PATH without jumping through enormous hoops. This converts truncate (although you can't tell using truncate from coreutils, because it actually uses open + ftruncate).

The basic idea is that there's a new helper function user_file_or_path_at. It takes an fd and a path and, depending on flags, the emptiness of the path, and whether path is a magic /proc symlink (or a symlink to a magic /proc/symlink), it returns either a struct path or a struct file *.

To make this genuinely useful, something similar will need to happen for open/openat.

The lifetime rules for the contents of struct nameidata seem weird. If all users first memset the thing to zero and called nd_put or something when they were done, this would be cleaner. But they don't.

---
 fs/namei.c            | 73 ++++++++++++++++++++++++++++++++++++++++
 fs/open.c             | 93 +++++++++++++++++++++++++++++----------------------
 fs/proc/base.c        | 44 +++++++++++++-----------
 fs/proc/fd.c          | 10 +++---
 fs/proc/internal.h    |  3 +-
 include/linux/namei.h | 14 ++++++++
 6 files changed, 171 insertions(+), 66 deletions(-)

index 89a612e..0d905ac 100644
@@ -691,6 +691,28 @@ void nd_jump_link(struct nameidata *nd, struct path *path)
 	nd->flags |= LOOKUP_JUMPED;
 }
 
+/*
+ * Helper for /proc/pid/fd/N-style magic links.  This is like nd_jump_link
+ * except that it will apply additional security checks.  Caller must have
+ * taken a reference to file beforehand.
+ */
+void nd_jump_link_file(struct nameidata *nd, struct file *file)
+{
+	path_put(&nd->path);
+
+	nd->path = file->f_path;
+	path_get(&nd->path);
+	if (nd->flags & LOOKUP_FILE) {
+		if (nd->last_symlink_file)
+			fput(nd->last_symlink_file);
+		nd->last_symlink_file = file;
+	} else {
+		fput(file);
+	}
+	nd->inode = nd->path.dentry->d_inode;
+	nd->flags |= LOOKUP_JUMPED;
+}
+
 static inline void put_link(struct nameidata *nd, struct path *link, void *cookie)
 {
 	struct inode *inode = link->dentry->d_inode;

@@ -842,6 +864,10 @@ follow_link(struct path *link, struct nameidata *nd, void **p)
 		goto out_put_nd_path;
 
 	nd->last_type = LAST_BIND;
+	if (unlikely((nd->flags & LOOKUP_FILE) && nd->last_symlink_file)) {
+		fput(nd->last_symlink_file);
+		nd->last_symlink_file = NULL;
+	}
 	*p = dentry->d_inode->i_op->follow_link(dentry, nd);
 	error = PTR_ERR(*p);
 	if (IS_ERR(*p))

@@ -2156,6 +2182,53 @@ int user_path_at(int dfd, const char __user *name, unsigned flags,
 	return user_path_at_empty(dfd, name, flags, path, NULL);
 }
 
+int user_file_or_path_at(int dfd, const char __user *filename,
+			 int lookup_flags, bool null_filename_okay,
+			 struct file_or_path *out)
+{
+	struct nameidata nd;
+	struct filename *tmp;
+	int err;
+	int empty = 0;
+
+	if (!filename && null_filename_okay) {
+		out->file = fget(dfd);
+		return out->file ? 0 : -EBADF;
+	}
+
+	tmp = getname_flags(filename, lookup_flags, &empty);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+
+	if (empty) {
+		out->file = fget(dfd);
+		return out->file ? 0 : -EBADF;
+	}
+
+	nd.last_symlink_file = NULL;
+	BUG_ON(lookup_flags & LOOKUP_PARENT);
+
+	err = filename_lookup(dfd, tmp, lookup_flags | LOOKUP_FILE, &nd);
+	putname(tmp);
+	if (!err) {
+		if (nd.last_symlink_file && nd.last_type == LAST_BIND) {
+			out->file = nd.last_symlink_file;
+			memset(&out->path, 0, sizeof(out->path));
+			path_put(&nd.path);
+		} else {
+			out->path = nd.path;
+			out->file = NULL;
+			if (nd.last_symlink_file)
+				fput(nd.last_symlink_file);
+		}
+	} else {
+		if (nd.last_symlink_file)
+			fput(nd.last_symlink_file);
+	}
+
+	return err;
+}
+
 /*
  * NB: most callers don't do anything directly with the reference to the
  *     to struct filename, but the nd->last pointer points into the name string

index 0ff80f9..4c88fc2 100644
@@ -137,7 +137,7 @@ 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 dentry *dentry, struct file_or_path *link)
 {
 	struct files_struct *files = NULL;
 	struct task_struct *task;

@@ -151,13 +151,11 @@ static int proc_fd_link(struct dentry *dentry, struct path *path)
 
 	if (files) {
 		int fd = proc_fd(dentry->d_inode);
-		struct file *fd_file;
 
 		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);
+		link->file = fcheck_files(files, fd);
+		if (link->file) {
+			get_file(link->file);
 			ret = 0;
 		}
 		spin_unlock(&files->file_lock);

index 651d09a..fc936d5 100644
@@ -14,6 +14,7 @@
 #include <linux/spinlock.h>
 #include <linux/atomic.h>
 #include <linux/binfmts.h>
+#include <linux/namei.h>
 
 struct ctl_table_header;
 struct mempolicy;

@@ -51,7 +52,7 @@ struct proc_dir_entry {
 };
 
 union proc_op {
-	int (*proc_get_link)(struct dentry *, struct path *);
+	int (*proc_get_link)(struct dentry *, struct file_or_path *link);
 	int (*proc_read)(struct task_struct *task, char *page);
 	int (*proc_show)(struct seq_file *m,
 		struct pid_namespace *ns, struct pid *pid,

index 5a5ff57..77f585e 100644
@@ -15,6 +15,7 @@ struct nameidata {
 	struct qstr	last;
 	struct path	root;
 	struct inode	*inode; /* path.dentry.d_inode */
+	struct file	*last_symlink_file;
 	unsigned int	flags;
 	unsigned	seq;
 	int		last_type;

@@ -56,9 +57,21 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
 #define LOOKUP_ROOT		0x2000
 #define LOOKUP_EMPTY		0x4000
 
+#define LOOKUP_FILE		0x8000
+
 extern int user_path_at(int, const char __user *, unsigned, struct path *);
 extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
 
+struct file_or_path
+{
+	struct file *file;
+	struct path path;
+};
+
+extern int user_file_or_path_at(int dfd, const char __user *filename,
+				int lookup_flags, bool null_filename_okay,
+				struct file_or_path *out);
+
 #define user_path(name, path) user_path_at(AT_FDCWD, name, LOOKUP_FOLLOW, path)
 #define user_lpath(name, path) user_path_at(AT_FDCWD, name, 0, path)
 #define user_path_dir(name, path) \

@@ -83,6 +96,7 @@ extern struct dentry *lock_rename(struct dentry *, struct dentry *);
 extern void unlock_rename(struct dentry *, struct dentry *);
 
 extern void nd_jump_link(struct nameidata *nd, struct path *path);
+extern void nd_jump_link_file(struct nameidata *nd, struct file *file);
 
 static inline void nd_set_link(struct nameidata *nd, char *path)
 {

1.8.3.1

So this seems *way* too complex. I'd much rather see "nd->flags" get a LOOKUP_READONLY flag, for example, that gets set by proc_pid_follow_link() when it hits a read-only file descriptor (and gets cleared by other lookups).

Wouldn't that be *much* more straightforward?

Linus

It would if it works. It certainly would for truncate, setxattr, etc.

There are funny cases, though. For example, execing an O_WRONLY fd should probably fail, as should opening an O_WRONLY fd as O_RDWR. O_APPEND is also funny. flink will (I suspect) always want to be a bit special.

There are also O_PATH fds, and I'm not sure what the semantics of O_PATH fds are or should be when they refer to something other than a directory.

The benefit of my approach is that it's really obvious that truncate("/proc/self/fd/N") and ftruncate(N) do exactly the same thing. The downside is that the namei code is a bit gross and there was more rearranging of ftruncate than I would have liked. (I also lost the benefit of fget_light, but I could fix that by passing a struct fd around instead of a struct file *.)

--Andy

O_PATH file just points to specific location in the tree, no more and no less.

With that thing namei code becomes _really_ gross and we'll have to live with it afterwards. No. Moreover, grabbing references to struct file is the wrong thing to do - we need far less information than that.

Let's look at that zoo first.

a) linkat() target. flink(2) doesn't exist, so we can choose whatever we bloody please for that case. And frankly, I would gladly go for O_TMPFILE sans O_EXCL or sufficient caps. See upthread for proposed solution.

b) chdir(). Checks are identical in chdir() and fchdir(), don't bloody care how the file had been opened and don't need to care - anything we could've accessed after such chdir() we could've accessed by slapping a relative path to the end of the path we'd used to reach that thing.

c) chroot(). fchroot() doesn't exist and realistically anyone who can do chroot(2) is well past caring about anything. We really don't care how the damn thing had been opened.

d) fchownat(). Neither it nor fchown() give a flying fuch about the way file had been opened.

e) fchmodat(). Same story.

f) faccessat(). IMO we don't care how the damn thing had been opened.

g) name_to_handle_at(). Not sure if we need any capability checks, but we definitely don't give a damn about the way file had been opened.

h) one case in quotactl(). With CAP_SYS_ADMIN required. If attacker got that, it's too fucking late anyway.

i) fstatat() and friends, statfs(), utimes() et.al, {set,get,list,remove}xattr()
- no difference in checks between pathname- and descriptor-based calls anyway. Please, note that for fsetxattr() we do *NOT* require the file to be opened for write; adding such requirement would be a user-visible API change, and one fairly likely to break stuff, at that.

j) umount() and pivot_root(). We really don't care how the file had been opened, and attacker capable of playing with that can do a lot more.

k) inotify_add_watch()/fanotify_mark(). No descriptor-based versions, no permission checks other than "may read whatever we ended up with". I really doubt that we care about the way fd had been opened in case of /proc/<pid>/fd/<fd>.

l) truncate() mess.

m) open() mess.

AFAICS, the *only* cases when we might possibly care are linkat() target, truncate() and open(). Note, BTW, that right now we *do* allow an attempt to reopen a file via procfs symlink r/w, even when file had been r/o. It's subject to permissions on the object being opened, but that's it.

I'm not sure we can change that - again, it's a user-visible API, and the change is very likely to break some scripts. In fact, it's about as dangerous as a full-blown switch to dup-style semantics for procfs opens, and it's a lot less attractive.

For truncate() we would only need to have FMODE_WRITE reported, more or less the same way as FMODE_FLINK. And without open() changes it doesn't buy us anything at all...

I've no problem with unrolling the user_path_at() in do_sys_truncate() into an explicit loop by trailing symlinks and checking for indication left by proc_pid_follow_link(), more or less the same way as with LOOKUP_LINK in lookup_last(). It's _far_ less invasive than playing with "oh, here we fill a struct path or maybe a struct file" horrors, pinning struct file for no reason, etc.

AFAICS, the real question is whether we dare to change open() behaviour on /proc/*/fd/*. I've played with that a bit and I believe that I can do the switch to dup-style with very localized changes in fs/namei.c and fs/proc/{base,fd}.c. Will be even binary compatible kernel-side - ->atomic_open() returns NULL/ERR_PTR where it used to return 0/-error, not that we had many instances to convert. *IF* that variant is not out of consideration for userland API stability reasons, I would certainly prefer to go that way; turns out that these days we can pull it off without black magic in descriptor handling, etc. Linus?

Pseudo-related: I just reverted the flink() commit in my tree, so that at least wrt that whole thing we'll be back to the original behavior, broken or not. Clearly we're not done with this discussion, and the patches flying around aren't appropriate for 3.11, so..

Linus

I don't know whether ftruncate(some O_PATH fd) should work. But this probably barely matters.

open_by_handle(name_to_handle(read-only fd)) is dangerous for the same reason that open("/proc/self/fd/N", O_RDWR) is, but the capability checks have that covered.

Sigh. I wonder if that was deliberate. Anyway, it is what it is.

I personally find the check-mode-but-get-a-new-struct-file version to be less weird that the dup approach. Either approach will break scripts that try to write to /dev/stdin (which is the whole point).

--Andy

It shouldn't. No IO on these guys at all.

What, breaking existing userland? IMO that's a thing to avoid, unless we have really, really strong reasons not to. And yes, it goes for both variants... FWIW, I'm not convinced that the reasons you are giving for it are strong enough - passing somebody a read-only file descriptor to a file they could open for write and relying on their inability to truncate the fscker just because it's not reachable via any path they've got search permissions to looks like a Bloody Bad Idea(tm), and not only because it won't do what you hope it'll do on existing kernels. It's very easy to fuck up and end up with a searchable path to the damn thing; e.g. /proc/<pid>/cwd/foo will bypass the grandparent of foo not being searchable for you, etc.

This affects O_TMPFILE, for example -- create a file with O_TMPFILE | O_RDWR and mode 0666 (by accident), write something, then open("/proc/self/fd/N", O_RDONLY) and send the resulting fd to someone. They can't directly write it, but they can reopen it O_RDWR.

I agree that flink is the main issue here, FWIW. I think that the current semantics are just too screwed up to make it really safe to pass around fds with reduced access modes and expect those modes to stick.

--Andy

No.

I'm curious what's wrong with the general concept. (I agree that the implementation is heinous.)

If I ever wanted to add a new *at syscall, I'd like to be able to do:

int sys_whateverat(int dfd, const char __user *name, int flags) {
resolve_the_thing(dfd, name, flags);

  if (it's a file) {
    check fmode;
  } else {
    inode_permission();

}

  actually_do_something(inode);

  unreference_whatever_i_got();

return ret;
}

thereby killing the fwhatever and whateverat birds with one stone.

...phew. I wasn't looking forward to testing and debugging my crap :)

--Andy

Sorry for off-topic, I am just curios.

But sock_no_open() disallows this, and for good reason I guess.

I am wondering, perhaps anon_inode should do the same? I do not see any problem, but it looks pointless and misleading to allow to open a file you can do nothing with.

Or is there any reason why, say, open("anon_inode:[perf_event]") should succeed?

Thanks,

Oleg.

@@ -24,7 +24,15 @@
 
 static struct vfsmount *anon_inode_mnt __read_mostly;
 static struct inode *anon_inode_inode;
-static const struct file_operations anon_inode_fops;
+
+static int anon_open(struct inode *inode, struct file *file)
+{
+	return -ENXIO;
+}
+
+static const struct file_operations anon_inode_fops = {
+	.open = anon_open,
+};
 
 /*
  * anon_inodefs_dname() is called from d_path().

Hi Oleg,

Hmmm not exactly, it works for a pipe but not for a socket. It even breaks /dev/stdin (/proc/self/fd/0) for processes running attached to a socket (eg: shell script) :

sh-3.1$ ls -la /proc/$$/fd/
total 0
dr-x------ 2 willy users 0 Aug 24 23:03 .
dr-xr-xr-x 6 willy users 0 Aug 24 23:03 ..
lrwx------ 1 willy users 64 Aug 24 23:03 0 -> socket:[1247293] lrwx------ 1 willy users 64 Aug 24 23:03 1 -> socket:[1247293] lrwx------ 1 willy users 64 Aug 24 23:03 2 -> socket:[1247293] lrwx------ 1 willy users 64 Aug 24 23:03 255 -> socket:[1247293] sh-3.1$ read < /dev/stdin
sh: /dev/stdin: No such device or address
sh-3.1$ read < /dev/fd/0
sh: /dev/fd/0: No such device or address

But :

window 1:

  willy@pcw:~$ ssh 0 sh -i
  sh-3.1$ echo $$
  9832
  sh-3.1$ ls -la /proc/$$/fd/
  total 0
  dr-x------ 2 willy users  0 Aug 24 23:16 .
  dr-xr-xr-x 6 willy users  0 Aug 24 23:16 ..
  lr-x------ 1 willy users 64 Aug 24 23:16 0 -> pipe:[1247914]
  l-wx------ 1 willy users 64 Aug 24 23:16 1 -> pipe:[1247915]
  l-wx------ 1 willy users 64 Aug 24 23:16 2 -> pipe:[1247916]
  l-wx------ 1 willy users 64 Aug 24 23:17 255 -> pipe:[1247916]
  sh-3.1$ 

window 2:

  willy@pcw:~$ echo hello > /proc/9832/fd/1
  willy@pcw:~$ echo whoami > /proc/9832/fd/0

window 1:
sh-3.1$ hello
willy
sh-3.1$

I don't know, I'm often a bit confused by entries in /proc/pid/fd because I don't always know which ones might safely be used or not.

I doubt it. It seems to me that most such entries are implemented for completeness while most valid uses only concern /proc/self/fd. Maybe if we had an option so that only /proc/self/fd would actually allow to access the fds while all /proc/pid/fd would only show what they map to, it would be a good step forward.

How? The fundamental problem is not visibility of that stuff, it's new opened file for the same object (Linux behaviour) vs. new descriptor refering to the same opened file (*BSD and friends). We can't get anon_... sanely reopened in the former semantics and they are very visibly different for regular files, so switching to *BSD one is not feasible - too high odds of userland breakage. The difference in semantics, of course, is that on Linux opening /dev/stdin gives you a descriptor with independent current IO position; on *BSD you get a descriptor sharing the current IO position with stdin. IOW, it's independent open() of the same file vs. dup().

We are really stuck with the current semantics here - switching to *BSD one would not only mean serious surgery on descriptor handling (it's one of the wartier areas in *BSD VFS, in large part because of magic-open-really-a-dup kludges they have to do), it would change a long-standing userland API that had been there for nearly 20 years _and_ one that tends to be used in corner cases of hell knows how many scripts.

Thanks for explaining Al, that really helps me understand. However there's still a difference between /proc/pid called from the process itself (=/proc/self) and called from other processes that seems to suit the situation :

  willy@eeepc:~$ ls -la /tmp/bash 
  -r-x--x--x 1 root users 916852 2013-08-25 08:19 /tmp/bash*
  willy@eeepc:~$ exec /tmp/bash -i
  willy@eeepc:~$ echo $$
  22678
  willy@eeepc:~$ ls -la /proc/22678/fd
  ls: cannot open directory /proc/22678/fd: Permission denied
  willy@eeepc:~$ ls -la /proc/22678/exe 
  ls: cannot read symbolic link /proc/22678/exe: Permission denied
  willy@eeepc:~$ cat /proc/22678/fd/0 
  cat: /proc/22678/fd/0: Permission denied

but :

  willy@eeepc:~$ read < /proc/22678/fd/0 
  azerazerazer
  willy@eeepc:~$ echo $REPLY
  azerazerazer

strace clearly shows that the process was allowed to inspect itself and the other ones were not :

  willy@eeepc:~$ strace -p 22678
  open("/proc/22678/fd/0", O_RDONLY|O_LARGEFILE) = 3

  willy@eeepc:~$ strace cat /proc/22678/fd/0 
  open("/proc/22678/fd/0", O_RDONLY|O_LARGEFILE) = -1 EACCES (Permission denied)

It looks like this difference was introduced by this patch (which also fixes this issue we've been having for a very long time on 2.4 and early 2.6) :

8948e11 Allow access to /proc/$PID/fd after setuid()

Thus I'm wondering if something like this could help, the idea would be that a with the appropriate mount option, a task could only look at its own descriptors unless it's running with privileges :

    static int proc_fd_permission(struct inode *inode, int mask,
                                  struct nameidata *nd)
    {
       if (task_pid(current) == proc_pid(inode))
             return 0;
       if (capable(CAP_DAC_OVERRIDE))
             return 0;
       if (proc_mounted_with_strict_option)
             return -EACCES;
       return generic_permission(inode, mask, NULL);
    }

Thus it would not change the default behaviour except for people who would mount /proc with a special option.

Thanks,
Willy

/proc/self has magic special properties, as you noticed.

I'd much rather try to do it in general, and use "file->f_cred" more aggressively for /proc/<pid>/fd/ security.

We don't use f_cred at all in /proc, but that's because /proc predates that whole thing. So instead we use the credentials of the task when we want to look at credentials of the file, because that was the closest approximation we used to have.

Look at the code that creates the fd stat information, for example. It's in tid_fd_revalidate(), and it really doesn't make much sense to use the task credentials for it. I wonder if we should do something like the appended (whitespace-damaged and totally untested) patch.

Linus

---
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 75f2890abbd8..2a5a53cc7a0a 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -74,7 +74,6 @@ static int tid_fd_revalidate(struct dentry
*dentry, unsigned int flags)

   {
          struct files_struct *files;
          struct task_struct *task;
  -       const struct cred *cred;
          struct inode *inode;
          int fd;

@@ -95,19 +94,17 @@ static int tid_fd_revalidate(struct dentry
*dentry, unsigned int flags)

                          if (file) {
                                  unsigned f_mode = file->f_mode;

- rcu_read_unlock();
- put_files_struct(files);
-

                                  if (task_dumpable(task)) {
  -                                       rcu_read_lock();
  -                                       cred = __task_cred(task);
  +                                       const struct cred *cred =
file->f_cred;
                                          inode->i_uid = cred->euid;
                                          inode->i_gid = cred->egid;
  -                                       rcu_read_unlock();
                                  } else {
                                          inode->i_uid = GLOBAL_ROOT_UID;
                                          inode->i_gid = GLOBAL_ROOT_GID;
                                  }

+ rcu_read_unlock();
+ put_files_struct(files);
+

                                  if (S_ISLNK(inode->i_mode)) {
                                          unsigned i_mode = S_IFLNK;

Cough. I am going off-topic again, but I can't resist...

Or pid_revalidate(), but my concern is task_dumpable() logic.

pid_revalidate() does inode->i_*id = GLOBAL_ROOT_*ID if task_dumpable() fails, but it can fail simply because ->mm = NULL.

This means that almost everything in /proc/zombie-pid/ becomes root. Doesn't really hurt, but for what? Looks a bit strange imho.

Oleg.

The zombie case shouldn't be relevant, because a zombie will have closed all the file descriptors anyway, so they no longer exist.

That said, task_dumpable isn't wonderful, and I suspect we could drop that logic entirely in the tid-fd case if we just use f_cred.

The reason we have task_dumpable is exactly because we use the task credentials, and they may not really be relevant to the file credentials. IOW, it's there to protect against execve'ing a suid program that opens some protected file and then in setuid()'s back the the original user after having done the critical stuff. But file->f_cred is exactly about the credentials at the time of the open, so it should make things like that irrelevant.

Linus

I specially mentioned that this is off-topic ;)

Probably yes, but I do not understand this S_IFLNK && uid/chmod magic in tid_fd_revalidate(). And afaics this should not affect readlink() anyway. So yes, ->f_cred makes more sense to me, but I can't comment.

But, afaics, speaking of task_dumpable() this doesn't matter. Please forget about /proc/fd or zombies. I can't even understand proc_pid_make_inode() or pid_revalidate().

	$ id
	uid=1000(tst) gid=100(users) groups=100(users)
	$ cp `which ls` ls
	$ chmod a-r ./ls
	$
	$ ./ls -l /proc/self/
	total 0
	-r-------- 1 root root  0 Aug 26 06:35 auxv
	-r--r--r-- 1 root root  0 Aug 26 06:35 cgroup
	--w------- 1 root root  0 Aug 26 06:35 clear_refs
	-r--r--r-- 1 root root  0 Aug 26 06:35 cmdline
	-rw-r--r-- 1 root root  0 Aug 26 06:35 comm
	-rw-r--r-- 1 root root  0 Aug 26 06:35 coredump_filter
	lrwxrwxrwx 1 root root  0 Aug 26 06:35 cwd -> /home/tst
	-r-------- 1 root root  0 Aug 26 06:35 environ
	lrwxrwxrwx 1 root root  0 Aug 26 06:35 exe -> /home/tst/ls
	dr-x------ 2 root root  0 Aug 26 06:35 fd
	dr-x------ 2 root root  0 Aug 26 06:35 fdinfo
	-rw-r--r-- 1 root root  0 Aug 26 06:35 gid_map
	-r--r--r-- 1 root root  0 Aug 26 06:35 limits
	-r--r--r-- 1 root root  0 Aug 26 06:35 maps
	-rw------- 1 root root  0 Aug 26 06:35 mem
	-r--r--r-- 1 root root  0 Aug 26 06:35 mountinfo
	-r--r--r-- 1 root root  0 Aug 26 06:35 mounts
	-r-------- 1 root root  0 Aug 26 06:35 mountstats
	dr-xr-xr-x 4 tst  users 0 Aug 26 06:35 net
	dr-x--x--x 2 root root  0 Aug 26 06:35 ns
	-rw-r--r-- 1 root root  0 Aug 26 06:35 oom_adj
	-r--r--r-- 1 root root  0 Aug 26 06:35 oom_score
	-rw-r--r-- 1 root root  0 Aug 26 06:35 oom_score_adj
	-r--r--r-- 1 root root  0 Aug 26 06:35 pagemap
	-r--r--r-- 1 root root  0 Aug 26 06:35 personality
	-rw-r--r-- 1 root root  0 Aug 26 06:35 projid_map
	lrwxrwxrwx 1 root root  0 Aug 26 06:35 root -> /
	-r--r--r-- 1 root root  0 Aug 26 06:35 smaps
	-r--r--r-- 1 root root  0 Aug 26 06:35 stack
	-r--r--r-- 1 root root  0 Aug 26 06:35 stat
	-r--r--r-- 1 root root  0 Aug 26 06:35 statm
	-r--r--r-- 1 root root  0 Aug 26 06:35 status
	-r--r--r-- 1 root root  0 Aug 26 06:35 syscall
	dr-xr-xr-x 3 tst  users 0 Aug 26 06:35 task
	-rw-r--r-- 1 root root  0 Aug 26 06:35 uid_map
	-r--r--r-- 1 root root  0 Aug 26 06:35 wchan

For what? Say,

	-r--r--r-- 1 root root  0 Aug 26 06:35 status

but it is S_IRUGO anyway, why do we need to change the owner?

	dr-x------ 2 root root  0 Aug 26 06:35 fd

OK, this means that I can't access this dir from another process. Not sure we really want this in this case but

	$ ./ls /proc/self/fd
	0  1  2  3

still works, I guess thanks to proc_fd_permission().

However, say,

	-r-------- 1 root root  0 Aug 26 06:35 mountstats

actually becomes unreadable even via /proc/self/.

Imho, this all is confusing. Perhaps it makes sense to "chmod", say, /proc/pid/maps if !task_dumpable(), but "chown" looks strange.

Oleg.

And btw. Whatever we do, shouldn't we change proc_fd_permission()?

/proc/self is actually /proc/tgid, this means that the task_pid() check can't help if a sub-thread uses /proc/self.

Oleg.

@@ -288,7 +288,7 @@ int proc_fd_permission(struct inode *ino
 	int rv = generic_permission(inode, mask);
 	if (rv == 0)
 		return 0;
-	if (task_pid(current) == proc_pid(inode))
+	if (task_tgid(current) == proc_pid(inode))
 		rv = 0;
 	return rv;
 }

proc_fd_permission() says "process can still access /proc/self/fd after it has executed a setuid()", but the "task_pid() = proc_pid() check only helps if the task is group leader, /proc/self points to /proc/leader-pid.

Change this check to use task_tgid() so that the whole process can access /proc/self/fd.

Note: CLONE_THREAD doesn't require CLONE_FILES so task->files can differ, but I don't think this can lead to any security problem.

Test-case:

	void *tfunc(void *arg)
	{
		assert(opendir("/proc/self/fd"));
		return NULL;
	}

	int main(void)
	{
		pthread_t t;
		pthread_create(&t, NULL, tfunc, NULL);
		pthread_join(t, NULL);
		return 0;
	}

fails if, say, this executable is not readable and suid_dumpable = 0.

---
 fs/proc/fd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 0ff80f9..985ea88 100644
@@ -286,7 +286,7 @@ int proc_fd_permission(struct inode *inode, int mask)
 	int rv = generic_permission(inode, mask);
 	if (rv == 0)
 		return 0;
-	if (task_pid(current) == proc_pid(inode))
+	if (task_tgid(current) == proc_pid(inode))
 		rv = 0;
 	return rv;
 }

1.5.5.1

Patch looks ok to me, but since this has never worked and nobody has actually complained, I can't really convince myself that this is critical.

So I'm not going to apply it for 3.11, and it had better wait for the next merge window.

Linus

Actually, let's back-track..

Did you try the other approach? Make /proc/self point to the thread instead of the task?

The thread-group leader seems to have these extra files:

- autogroup, coredump_filter, mountstats, net, task

but quite frankly, at least "net" and "task" look like they should exist there - with "task" pointing back to the actual task (it would make more sense for "/proc/<pid>/task" itself to be named "threads", but whatever).

Yes, it would be semantically different, but it would mean that "/proc/self/fd/" would actually make sense in a way that it currently does *not* - which would seem fairly important, since the primary use for it tends to be /dev/stdin.

And the other semantic differences might be much harder to notice. Worth testing?

Linus

On Mon, Aug 26, 2013 at 12:35:26PM -0700, Linus Torvalds wrote:
(...)

I remember another user, don't know if that has changed. UPX used to build self-extract binaries that opened /proc/self/fd/3. It might be worth checking this as well if doing important changes, because it's typically a usage that could be discovered to be broken months after the change!

Willy

But /proc/<tgid>/fd and /proc/<tid>/fd should be the same. Unless it plays with unshare() or clone(CLONE_THREAD /* no CLONE_FILES */).

Unlike, but:

Oh yes, I agree. This change is trivial but nasty, god knows what people actually do with /proc/self.

Oleg.

Yes, I thought about this. But I agree with Eric, we probably need another magic link, /proc/thread or whatever.

And. I think that s/task_pid/task_tgid/ in proc_fd_permission() makes sense anyway. It is not only for /proc/self, why we should restrict the access to /proc/<sub-thread>/fd ?

Note really afaics. Yes, tgid_base_stuff and tid_base_stuff differ, but proc_root_lookup() uses tgid_base_stuff in any case, so /proc/<tid>/ also has task,mountstats,etc even if it is not leader.

And I am afraid this can break things. But I leave this to you and Eric.

Personally I think that /proc/self pointing to "current" is better, and in fact I was surprised when I recently found that this is not true. But perhaps it is too late to change this old behaviour.

I think this doesn't matter "in practice", normally all threads have the same ->files. Who needs CLONE_THREAD without CLONE_FILES ?

Perhaps... Well, if Andrew takes this patch (assuming you and Eric ack it), we can see if we have any bug reports.

Oleg.

/proc/self points to /proc/<tgid>, not to /proc/<tid>, and thus "self" actually means "group leader".

Change fs/proc/self.c to print the caller's tid, this is probably what the users actually expect.

Note: this is obviously semantically different and the difference is subtle, but most (all?) users should not notice this change. Lets see if it breaks something.

---
 fs/proc/self.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

index 6b6a993..7da7154 100644
@@ -11,26 +11,26 @@ static int proc_self_readlink(struct dentry *dentry, char __user *buffer,
 			      int buflen)
 {
 	struct pid_namespace *ns = dentry->d_sb->s_fs_info;
-	pid_t tgid = task_tgid_nr_ns(current, ns);
+	pid_t tid = task_pid_nr_ns(current, ns);
 	char tmp[PROC_NUMBUF];
-	if (!tgid)
+	if (!tid)
 		return -ENOENT;
-	sprintf(tmp, "%d", tgid);
+	sprintf(tmp, "%d", tid);
 	return vfs_readlink(dentry,buffer,buflen,tmp);
 }
 
 static void *proc_self_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	struct pid_namespace *ns = dentry->d_sb->s_fs_info;
-	pid_t tgid = task_tgid_nr_ns(current, ns);
+	pid_t tid = task_pid_nr_ns(current, ns);
 	char *name = ERR_PTR(-ENOENT);
-	if (tgid) {
+	if (tid) {
 		/* 11 for max length of signed int in decimal + NULL term */
 		name = kmalloc(12, GFP_KERNEL);
 		if (!name)
 			name = ERR_PTR(-ENOMEM);
 		else
-			sprintf(name, "%d", tgid);
+			sprintf(name, "%d", tid);
 	}
 	nd_set_link(nd, name);
 	return NULL;

@@ -57,7 +57,7 @@ int proc_setup_self(struct super_block *s)
 	struct inode *root_inode = s->s_root->d_inode;
 	struct pid_namespace *ns = s->s_fs_info;
 	struct dentry *self;
-	
+
 	mutex_lock(&root_inode->i_mutex);
 	self = d_alloc_name(s->s_root, "self");
 	if (self) {

1.5.5.1

Yeah, thinking more about this, this can't work. I think you'd need to use "/proc/<tgid>/task/<tid>" wouldn't you?

Or perhaps even more complicated, and say "if it's a task group leader, keep the old /proc/<tgid>, otherwise do the full thread path".

Anyway, does not not sound worth it. Your previous simple patch is probably the right thing to do.

Linus

Why? To me /proc/self == /proc/$((sys_gettid)) looks more natural. Say, /proc/self/task... But this is subjective.

Not to mention, that would be much more visible change.

OK, lets forget it.

Although to be honest, I was seduced by "Worth testing". I mean I am just curious, who can suffer from this change? Nevermind, please ignore.

Oleg.

Actually, you're right - I incorrectly thought we had removed the thread id lookup from the top level in /proc. But we never actually did that. We only removed them from readdir.

So while you won't see thread id's in the directory listing, we *do* successfully look up thread id's when specified explicitly. It's confusing, but it happens to work. So you can do

ls -l /proc/<tid>/

and get the expected result, but if you do

ls -l /proc | grep <tid>

it won't actually show up unless the thread ID is also the thread group leader.

Yeah, if we were to redesign /proc I'd do it differently, but I think we should just accept that it works "well enough" and there's just too much risk from making changes that aren't strictly required.

Linus

Yes, agreed.

To all: just in case, I will be completely offline till Sep 10, vacation.

So, Eric, if you still disagree with s/task_pid/task_tgid/ change please nack that (minor) patch, we can return to this later.

Oleg.

No, it lives in tid_base_stuff, it should be only visible in /proc/*/task/tid/ dir.

(and perhaps it also makes sense to remove mem/exe-like things from
tid_base_stuff...)

Anyway I agree, this change is risky.

Oleg.

There is at least a semantic goofiness here.

There is /proc/<tgid>/fd and /proc/<tgid>/task/<pid>/fd, and the same permission check is used by both.

We might just want to have a /proc/thread symlink as well so people don't have this issue. Of course that would require people to use it, and I think the common case if people care is call gettid() and build the path themselves.

Eric

Yes, and we have /proc/<tid>/ which includes fd as well.

Yes! I agree.

In particular, from the changelog:

	Note: CLONE_THREAD doesn't require CLONE_FILES so task->files can
	differ,

so /proc/self/fd doesn't necessarily mean current->files, this can confuse the application.

And I also assume that you agree with this change ;)

Oleg.

At least I hope... Because I think this is another issue.

And because with or without a /proc/thread symlink, I believe that people don't expect that /proc/self can restrict a sub-thread, and CLONE_THREAS without CLONE_FILES is exotic.

Oleg.

I don't disagree. Comparing tgid to pids is goofy and my brain is elsewhere so I have no thought through the implications.

Actually thinking I think the check should really be. In which case we are comparing what we really care about.

int proc_fd_permission(struct inode *inode, int mask)

{
	int rv = generic_permission(inode, mask);
	if (rv == 0)
		return 0;

        rcu_read_lock();
        struct task *task = pid_task(proc_pid(inode));
        if (task && (current->files == task->files))
		rv = 0;
        rcu_read_unlock();

	return rv;
}

Eric

But for what?

To me, this looks like the unnecessary semantic complication. It looks as if we actually need to restrict the access to /proc/self/fd or /proc/<tgid>/fd or /proc/<subthread-tid>/fd.

I do not think there is any security reason to deny this. They share ->mm, a sub-thread can do "everything" with its leader or vice versa.

same_thread_group() looks more simple and natural to me. And note that __ptrace_may_access() was recently changed (in -mm) to use same_thread_group() instead of "task == current".

So personally I'd prefer to not change this patch and I think it makes sense even with "make /proc/self point to thread" I sent.

But. please tell me if you really dislike it. You are maintainer, I won't argue.

Oleg.

Actually, I'm pretty sure we did have the "dup" semantics at one point (long ago), and they were really nice (because you could use them to see where in the stream the fd was etc). It just fit so horribly badly into the VFS semantics that it got changed into the current "new file descriptor" one. Afaik, nothing broke.

So I'm not really sure about the "we're stuck with it" for semantic reasons, and it turns out that very few programs/scripts actually use /proc/<pid>/fd/<nr> at all (random use of /dev/stdin is likely the most common case). But I agree about the "serious surgery on descriptor handling" part.

Linus

Well... We are actually in better position for that these days; right now we have very few instances of ->atomic_open(), so we could change the calling conventions for it. It returns 0 or -error and we could turn that into NULL, ERR_PTR(-error) or a reference to already opened struct file. It's not _that_ far to propagate from that point - atomic_open() <- lookup_open() <- do_last() <- path_openat(). So the amount of surgery is nowhere near the horrors we used to need (and *BSD actually does).

We could try that, but I'm really afraid that semantics changes will break stuff; worse yet, that it'll happen to stuff in dusty corners of random admin scripts nobody able to debug anymore ;-/

We have fdinfo now, which is IMO much less scary. Programs can find the stream position, but they can't change it. OTOH...

.../dev/stdin doesn't actually do what you expect if input comes from something seekable.

$ cat /proc/self/fd/3
test
$ cat /proc/self/fd/3
test
$ cat /proc/self/fd/3
test
$ cat <&3
est
$ cat /proc/self/fd/3
test
$ cat <&3

(I'm not going to advocate for changing this.)

--Andy

Andy Lutomirski
AMA Capital Management, LLC

Hi!

Hmm, are you going to break my exploit?

http://www.exploit-db.com/exploits/10038/

I'd like that, because I don't think /proc should allow people to bypass directory permissions.
Pavel

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

Hi Willy,

But this is what I meant, sorry for confusion.

Let me try to explain. Just in case, this has nothing to do with security and I do not see any problem, still I think there is something wrong (but harmless).

Suppose that you are trying to open(/proc/pid/$pipe-or-socket-fd). nd_jump_link() sets nd->inode correctly, then dentry_open() does the rest. Everything is correct at this stage, the new file gets the correct f_inode/f_op.

However, unlike fifo_open(), socket_file_ops->open() can not actually create the file/sock connection, so sock_no_open() just fails and nothing bad happens.

But if you open an anon_inodefs file via proc, you get the "bogus" file. There is a single anon_inode_inode, its ->i_fop points to the empty anon_inode_fops, this has nothing to do with ->f_op of the actual file you tried to open.

Nothing bad happens, still I think this is simply wrong and misleading, and thus I think it would be better to disallow this via anon_open().

Oleg.

Damn. Sorry for noise, I removed lkml by accident, let me resend.

Oleg.

open("/proc/pid/$anon-fd") should fail, we can't create the new file with correct f_op/etc correctly. Currently this creates the bogus file with the empty anon_inode_fops, this is harmless but still wrong and misleading.

Add anon_inode_fops->anon_open() which simply returns ENXIO like sock_no_open() does in this case.

---
 fs/anon_inodes.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

index 47a65df..af25036 100644
@@ -24,7 +24,15 @@
 
 static struct vfsmount *anon_inode_mnt __read_mostly;
 static struct inode *anon_inode_inode;
-static const struct file_operations anon_inode_fops;
+
+static int anon_open(struct inode *inode, struct file *file)
+{
+	return -ENXIO;
+}
+
+static const struct file_operations anon_inode_fops = {
+	.open = anon_open,
+};
 
 /*
  * anon_inodefs_dname() is called from d_path().

1.5.5.1

Indeed I checked yesterday that it seems to use proc_pid_follow_link() for fd/, cwd, root and exe, which means the same tests are used everywhere.

I tried to add a test based on a mount option before this call to nd_jump_link() when I realized my attempt was a total disaster because I'm a noob. But what I think would be nice (at least as an opt-in) would be :

- processes which don't share the same root should not be allowed to

    access files through /proc/pid/{root,cwd,exe,fd/*}

- keep the current restrictions as well of course

- the exact same restrictions should apply to AT_EMPTY_PATH

I might be totally wrong, but as a user that's what I find natural and what I tend to expect.

Do not forget that 2 other syscalls seem to support AT_EMPTH_PATH as well if that makes a difference.

If needed for backwards compatibility, possibly, though I doubt that there are apps that *rely* on the current lack of isolation between chroots. But at the same time I hate to break existing setups :-)

Willy