[PATCH] proc: fix races of /proc/PID/{fd/,fdinfo/,fdinfo/*}

[PATCH] proc: fix races of /proc/PID/{fd/,fdinfo/,fdinfo/*}

[Vasiliy Kulikov]

fd* files are restricted to the task's owner, but keeping opened procfs
file descriptors makes it possible to violate the permission model.
Keeping fdinfo/* may disclosure current position and flags, keeping
fdinfo/ and fd/ may disclosure number of opened files.

Used existing (un)lock_trace functions to deal with the race.

CC: Stable Tree <stable@kernel.org>
Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 Hopefully, I'll propose more simple way to guard private procfs files
 soon, without these "scattered" ptrace checks.

 fs/proc/base.c |   86 +++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 55 insertions(+), 31 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 08e3ecc..b0477c3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1906,37 +1906,46 @@ static int proc_fd_info(struct inode *inode, struct path *path, char *info)
 	struct files_struct *files = NULL;
 	struct file *file;
 	int fd = proc_fd(inode);
+	int rc;
 
-	if (task) {
-		files = get_files_struct(task);
-		put_task_struct(task);
-	}
-	if (files) {
-		/*
-		 * We are not taking a ref to the file structure, so we must
-		 * hold ->file_lock.
-		 */
-		spin_lock(&files->file_lock);
-		file = fcheck_files(files, fd);
-		if (file) {
-			if (path) {
-				*path = file->f_path;
-				path_get(&file->f_path);
-			}
-			if (info)
-				snprintf(info, PROC_FDINFO_MAX,
-					 "pos:\t%lli\n"
-					 "flags:\t0%o\n",
-					 (long long) file->f_pos,
-					 file->f_flags);
-			spin_unlock(&files->file_lock);
-			put_files_struct(files);
-			return 0;
+	if (!task)
+		return -ESRCH;
+
+	rc = lock_trace(task);
+	if (rc)
+		goto out_task;
+
+	files = get_files_struct(task);
+	if (files == NULL)
+		goto out_unlock;
+	/*
+	 * We are not taking a ref to the file structure, so we must
+	 * hold ->file_lock.
+	 */
+	spin_lock(&files->file_lock);
+	file = fcheck_files(files, fd);
+	if (file) {
+		if (path) {
+			*path = file->f_path;
+			path_get(&file->f_path);
 		}
-		spin_unlock(&files->file_lock);
-		put_files_struct(files);
+		if (info)
+			snprintf(info, PROC_FDINFO_MAX,
+					"pos:\t%lli\n"
+					"flags:\t0%o\n",
+					(long long) file->f_pos,
+					file->f_flags);
+		rc = 0;
+	} else {
+		rc = -ENOENT;
 	}
-	return -ENOENT;
+	spin_unlock(&files->file_lock);
+	put_files_struct(files);
+out_unlock:
+	unlock_trace(task);
+out_task:
+	put_task_struct(task);
+	return rc;
 }
 
 static int proc_fd_link(struct inode *inode, struct path *path)
@@ -2057,13 +2066,20 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 	struct task_struct *task = get_proc_task(dir);
 	unsigned fd = name_to_int(dentry);
 	struct dentry *result = ERR_PTR(-ENOENT);
+	int rc;
 
 	if (!task)
 		goto out_no_task;
 	if (fd == ~0U)
 		goto out;
 
+	rc = lock_trace(task);
+	result = ERR_PTR(rc);
+	if (rc)
+		goto out;
+
 	result = instantiate(dir, dentry, task, &fd);
+	unlock_trace(task);
 out:
 	put_task_struct(task);
 out_no_task:
@@ -2083,23 +2099,28 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 	retval = -ENOENT;
 	if (!p)
 		goto out_no_task;
+
+	retval = lock_trace(p);
+	if (retval)
+		goto out;
+
 	retval = 0;
 
 	fd = filp->f_pos;
 	switch (fd) {
 		case 0:
 			if (filldir(dirent, ".", 1, 0, inode->i_ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		case 1:
 			ino = parent_ino(dentry);
 			if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		default:
 			files = get_files_struct(p);
 			if (!files)
-				goto out;
+				goto out_unlock;
 			rcu_read_lock();
 			for (fd = filp->f_pos-2;
 			     fd < files_fdtable(files)->max_fds;
@@ -2123,6 +2144,9 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 			rcu_read_unlock();
 			put_files_struct(files);
 	}
+
+out_unlock:
+	unlock_trace(p);
 out:
 	put_task_struct(p);
 out_no_task:
-- 
1.7.0.4

[kernel-hardening] [PATCH] proc: fix races of /proc/PID/{fd/,fdinfo/,fdinfo/*}

[Vasiliy Kulikov]

fd* files are restricted to the task's owner, but keeping opened procfs
file descriptors makes it possible to violate the permission model.
Keeping fdinfo/* may disclosure current position and flags, keeping
fdinfo/ and fd/ may disclosure number of opened files.

Used existing (un)lock_trace functions to deal with the race.

CC: Stable Tree <stable@kernel.org>
Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 Hopefully, I'll propose more simple way to guard private procfs files
 soon, without these "scattered" ptrace checks.

 fs/proc/base.c |   86 +++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 55 insertions(+), 31 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 08e3ecc..b0477c3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1906,37 +1906,46 @@ static int proc_fd_info(struct inode *inode, struct path *path, char *info)
 	struct files_struct *files = NULL;
 	struct file *file;
 	int fd = proc_fd(inode);
+	int rc;
 
-	if (task) {
-		files = get_files_struct(task);
-		put_task_struct(task);
-	}
-	if (files) {
-		/*
-		 * We are not taking a ref to the file structure, so we must
-		 * hold ->file_lock.
-		 */
-		spin_lock(&files->file_lock);
-		file = fcheck_files(files, fd);
-		if (file) {
-			if (path) {
-				*path = file->f_path;
-				path_get(&file->f_path);
-			}
-			if (info)
-				snprintf(info, PROC_FDINFO_MAX,
-					 "pos:\t%lli\n"
-					 "flags:\t0%o\n",
-					 (long long) file->f_pos,
-					 file->f_flags);
-			spin_unlock(&files->file_lock);
-			put_files_struct(files);
-			return 0;
+	if (!task)
+		return -ESRCH;
+
+	rc = lock_trace(task);
+	if (rc)
+		goto out_task;
+
+	files = get_files_struct(task);
+	if (files == NULL)
+		goto out_unlock;
+	/*
+	 * We are not taking a ref to the file structure, so we must
+	 * hold ->file_lock.
+	 */
+	spin_lock(&files->file_lock);
+	file = fcheck_files(files, fd);
+	if (file) {
+		if (path) {
+			*path = file->f_path;
+			path_get(&file->f_path);
 		}
-		spin_unlock(&files->file_lock);
-		put_files_struct(files);
+		if (info)
+			snprintf(info, PROC_FDINFO_MAX,
+					"pos:\t%lli\n"
+					"flags:\t0%o\n",
+					(long long) file->f_pos,
+					file->f_flags);
+		rc = 0;
+	} else {
+		rc = -ENOENT;
 	}
-	return -ENOENT;
+	spin_unlock(&files->file_lock);
+	put_files_struct(files);
+out_unlock:
+	unlock_trace(task);
+out_task:
+	put_task_struct(task);
+	return rc;
 }
 
 static int proc_fd_link(struct inode *inode, struct path *path)
@@ -2057,13 +2066,20 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 	struct task_struct *task = get_proc_task(dir);
 	unsigned fd = name_to_int(dentry);
 	struct dentry *result = ERR_PTR(-ENOENT);
+	int rc;
 
 	if (!task)
 		goto out_no_task;
 	if (fd == ~0U)
 		goto out;
 
+	rc = lock_trace(task);
+	result = ERR_PTR(rc);
+	if (rc)
+		goto out;
+
 	result = instantiate(dir, dentry, task, &fd);
+	unlock_trace(task);
 out:
 	put_task_struct(task);
 out_no_task:
@@ -2083,23 +2099,28 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 	retval = -ENOENT;
 	if (!p)
 		goto out_no_task;
+
+	retval = lock_trace(p);
+	if (retval)
+		goto out;
+
 	retval = 0;
 
 	fd = filp->f_pos;
 	switch (fd) {
 		case 0:
 			if (filldir(dirent, ".", 1, 0, inode->i_ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		case 1:
 			ino = parent_ino(dentry);
 			if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		default:
 			files = get_files_struct(p);
 			if (!files)
-				goto out;
+				goto out_unlock;
 			rcu_read_lock();
 			for (fd = filp->f_pos-2;
 			     fd < files_fdtable(files)->max_fds;
@@ -2123,6 +2144,9 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 			rcu_read_unlock();
 			put_files_struct(files);
 	}
+
+out_unlock:
+	unlock_trace(p);
 out:
 	put_task_struct(p);
 out_no_task:
-- 
1.7.0.4

Re: [PATCH] proc: fix races of /proc/PID/{fd/,fdinfo/,fdinfo/*}

[Andrew Morton]

On Thu, 4 Aug 2011 20:20:09 +0400
Vasiliy Kulikov <segoon@openwall.com> wrote:

> fd* files are restricted to the task's owner, but keeping opened procfs
> file descriptors makes it possible to violate the permission model.
> Keeping fdinfo/* may disclosure current position and flags, keeping
> fdinfo/ and fd/ may disclosure number of opened files.
> 
> Used existing (un)lock_trace functions to deal with the race.

what race.

When fixing a bug, please completely, utterly and exhaustively describe
the bug!

> CC: Stable Tree <stable@kernel.org>

And when cc'ing -stable, please make it very clear why you consider the
bug sufficiently serious to warant mackporting the fix into earlier
kernels.

[kernel-hardening] Re: [PATCH] proc: fix races of /proc/PID/{fd/,fdinfo/,fdinfo/*}

[Andrew Morton]

On Thu, 4 Aug 2011 20:20:09 +0400
Vasiliy Kulikov <segoon@openwall.com> wrote:

> fd* files are restricted to the task's owner, but keeping opened procfs
> file descriptors makes it possible to violate the permission model.
> Keeping fdinfo/* may disclosure current position and flags, keeping
> fdinfo/ and fd/ may disclosure number of opened files.
> 
> Used existing (un)lock_trace functions to deal with the race.

what race.

When fixing a bug, please completely, utterly and exhaustively describe
the bug!

> CC: Stable Tree <stable@kernel.org>

And when cc'ing -stable, please make it very clear why you consider the
bug sufficiently serious to warant mackporting the fix into earlier
kernels.

[PATCH] proc: fix races against execve() of /proc/PID/{fd/,fdinfo/,fdinfo/*}

[Vasiliy Kulikov]

fd* files are restricted to the task's owner, and other users may not
get direct access to them.  But one may open any of these files and run
any setuid program, keeping opened file descriptors.  As there are
permission checks on open(), but not on readdir() and read(), operations
on the kept file descriptors will not be checked.  It makes it possible
to violate procfs permission model.

Reading fdinfo/* may disclosure current fds' position and flags, reading
directory contents of fdinfo/ and fd/ may disclosure the number of opened
files by the target task.  This information is not sensible per se, but
it can reveal some private information (like length of a password stored in
a file) under certain conditions.

Used existing (un)lock_trace functions to deal with the issue by calling
ptrace_may_access() permission checks.

CC: Stable Tree <stable@kernel.org>
Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 Hopefully, I'll propose more simple way to guard private procfs files
 soon, without these "scattered" ptrace checks.

 fs/proc/base.c |   86 +++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 55 insertions(+), 31 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 08e3ecc..b0477c3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1906,37 +1906,46 @@ static int proc_fd_info(struct inode *inode, struct path *path, char *info)
 	struct files_struct *files = NULL;
 	struct file *file;
 	int fd = proc_fd(inode);
+	int rc;
 
-	if (task) {
-		files = get_files_struct(task);
-		put_task_struct(task);
-	}
-	if (files) {
-		/*
-		 * We are not taking a ref to the file structure, so we must
-		 * hold ->file_lock.
-		 */
-		spin_lock(&files->file_lock);
-		file = fcheck_files(files, fd);
-		if (file) {
-			if (path) {
-				*path = file->f_path;
-				path_get(&file->f_path);
-			}
-			if (info)
-				snprintf(info, PROC_FDINFO_MAX,
-					 "pos:\t%lli\n"
-					 "flags:\t0%o\n",
-					 (long long) file->f_pos,
-					 file->f_flags);
-			spin_unlock(&files->file_lock);
-			put_files_struct(files);
-			return 0;
+	if (!task)
+		return -ESRCH;
+
+	rc = lock_trace(task);
+	if (rc)
+		goto out_task;
+
+	files = get_files_struct(task);
+	if (files == NULL)
+		goto out_unlock;
+	/*
+	 * We are not taking a ref to the file structure, so we must
+	 * hold ->file_lock.
+	 */
+	spin_lock(&files->file_lock);
+	file = fcheck_files(files, fd);
+	if (file) {
+		if (path) {
+			*path = file->f_path;
+			path_get(&file->f_path);
 		}
-		spin_unlock(&files->file_lock);
-		put_files_struct(files);
+		if (info)
+			snprintf(info, PROC_FDINFO_MAX,
+					"pos:\t%lli\n"
+					"flags:\t0%o\n",
+					(long long) file->f_pos,
+					file->f_flags);
+		rc = 0;
+	} else {
+		rc = -ENOENT;
 	}
-	return -ENOENT;
+	spin_unlock(&files->file_lock);
+	put_files_struct(files);
+out_unlock:
+	unlock_trace(task);
+out_task:
+	put_task_struct(task);
+	return rc;
 }
 
 static int proc_fd_link(struct inode *inode, struct path *path)
@@ -2057,13 +2066,20 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 	struct task_struct *task = get_proc_task(dir);
 	unsigned fd = name_to_int(dentry);
 	struct dentry *result = ERR_PTR(-ENOENT);
+	int rc;
 
 	if (!task)
 		goto out_no_task;
 	if (fd == ~0U)
 		goto out;
 
+	rc = lock_trace(task);
+	result = ERR_PTR(rc);
+	if (rc)
+		goto out;
+
 	result = instantiate(dir, dentry, task, &fd);
+	unlock_trace(task);
 out:
 	put_task_struct(task);
 out_no_task:
@@ -2083,23 +2099,28 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 	retval = -ENOENT;
 	if (!p)
 		goto out_no_task;
+
+	retval = lock_trace(p);
+	if (retval)
+		goto out;
+
 	retval = 0;
 
 	fd = filp->f_pos;
 	switch (fd) {
 		case 0:
 			if (filldir(dirent, ".", 1, 0, inode->i_ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		case 1:
 			ino = parent_ino(dentry);
 			if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		default:
 			files = get_files_struct(p);
 			if (!files)
-				goto out;
+				goto out_unlock;
 			rcu_read_lock();
 			for (fd = filp->f_pos-2;
 			     fd < files_fdtable(files)->max_fds;
@@ -2123,6 +2144,9 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 			rcu_read_unlock();
 			put_files_struct(files);
 	}
+
+out_unlock:
+	unlock_trace(p);
 out:
 	put_task_struct(p);
 out_no_task:
-- 
1.7.0.4

[kernel-hardening] [PATCH] proc: fix races against execve() of /proc/PID/{fd/,fdinfo/,fdinfo/*}

[Vasiliy Kulikov]

fd* files are restricted to the task's owner, and other users may not
get direct access to them.  But one may open any of these files and run
any setuid program, keeping opened file descriptors.  As there are
permission checks on open(), but not on readdir() and read(), operations
on the kept file descriptors will not be checked.  It makes it possible
to violate procfs permission model.

Reading fdinfo/* may disclosure current fds' position and flags, reading
directory contents of fdinfo/ and fd/ may disclosure the number of opened
files by the target task.  This information is not sensible per se, but
it can reveal some private information (like length of a password stored in
a file) under certain conditions.

Used existing (un)lock_trace functions to deal with the issue by calling
ptrace_may_access() permission checks.

CC: Stable Tree <stable@kernel.org>
Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 Hopefully, I'll propose more simple way to guard private procfs files
 soon, without these "scattered" ptrace checks.

 fs/proc/base.c |   86 +++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 55 insertions(+), 31 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 08e3ecc..b0477c3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1906,37 +1906,46 @@ static int proc_fd_info(struct inode *inode, struct path *path, char *info)
 	struct files_struct *files = NULL;
 	struct file *file;
 	int fd = proc_fd(inode);
+	int rc;
 
-	if (task) {
-		files = get_files_struct(task);
-		put_task_struct(task);
-	}
-	if (files) {
-		/*
-		 * We are not taking a ref to the file structure, so we must
-		 * hold ->file_lock.
-		 */
-		spin_lock(&files->file_lock);
-		file = fcheck_files(files, fd);
-		if (file) {
-			if (path) {
-				*path = file->f_path;
-				path_get(&file->f_path);
-			}
-			if (info)
-				snprintf(info, PROC_FDINFO_MAX,
-					 "pos:\t%lli\n"
-					 "flags:\t0%o\n",
-					 (long long) file->f_pos,
-					 file->f_flags);
-			spin_unlock(&files->file_lock);
-			put_files_struct(files);
-			return 0;
+	if (!task)
+		return -ESRCH;
+
+	rc = lock_trace(task);
+	if (rc)
+		goto out_task;
+
+	files = get_files_struct(task);
+	if (files == NULL)
+		goto out_unlock;
+	/*
+	 * We are not taking a ref to the file structure, so we must
+	 * hold ->file_lock.
+	 */
+	spin_lock(&files->file_lock);
+	file = fcheck_files(files, fd);
+	if (file) {
+		if (path) {
+			*path = file->f_path;
+			path_get(&file->f_path);
 		}
-		spin_unlock(&files->file_lock);
-		put_files_struct(files);
+		if (info)
+			snprintf(info, PROC_FDINFO_MAX,
+					"pos:\t%lli\n"
+					"flags:\t0%o\n",
+					(long long) file->f_pos,
+					file->f_flags);
+		rc = 0;
+	} else {
+		rc = -ENOENT;
 	}
-	return -ENOENT;
+	spin_unlock(&files->file_lock);
+	put_files_struct(files);
+out_unlock:
+	unlock_trace(task);
+out_task:
+	put_task_struct(task);
+	return rc;
 }
 
 static int proc_fd_link(struct inode *inode, struct path *path)
@@ -2057,13 +2066,20 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 	struct task_struct *task = get_proc_task(dir);
 	unsigned fd = name_to_int(dentry);
 	struct dentry *result = ERR_PTR(-ENOENT);
+	int rc;
 
 	if (!task)
 		goto out_no_task;
 	if (fd == ~0U)
 		goto out;
 
+	rc = lock_trace(task);
+	result = ERR_PTR(rc);
+	if (rc)
+		goto out;
+
 	result = instantiate(dir, dentry, task, &fd);
+	unlock_trace(task);
 out:
 	put_task_struct(task);
 out_no_task:
@@ -2083,23 +2099,28 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 	retval = -ENOENT;
 	if (!p)
 		goto out_no_task;
+
+	retval = lock_trace(p);
+	if (retval)
+		goto out;
+
 	retval = 0;
 
 	fd = filp->f_pos;
 	switch (fd) {
 		case 0:
 			if (filldir(dirent, ".", 1, 0, inode->i_ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		case 1:
 			ino = parent_ino(dentry);
 			if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		default:
 			files = get_files_struct(p);
 			if (!files)
-				goto out;
+				goto out_unlock;
 			rcu_read_lock();
 			for (fd = filp->f_pos-2;
 			     fd < files_fdtable(files)->max_fds;
@@ -2123,6 +2144,9 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 			rcu_read_unlock();
 			put_files_struct(files);
 	}
+
+out_unlock:
+	unlock_trace(p);
 out:
 	put_task_struct(p);
 out_no_task:
-- 
1.7.0.4

Re: [PATCH] proc: fix races against execve() of /proc/PID/{fd/,fdinfo/,fdinfo/*}

[Andrew Morton]

On Fri, 26 Aug 2011 17:29:09 +0400
Vasiliy Kulikov <segoon@openwall.com> wrote:

> fd* files are restricted to the task's owner, and other users may not
> get direct access to them.  But one may open any of these files and run
> any setuid program, keeping opened file descriptors.  As there are
> permission checks on open(), but not on readdir() and read(), operations
> on the kept file descriptors will not be checked.  It makes it possible
> to violate procfs permission model.
> 
> Reading fdinfo/* may disclosure current fds' position and flags, reading
> directory contents of fdinfo/ and fd/ may disclosure the number of opened
> files by the target task.  This information is not sensible per se, but
> it can reveal some private information (like length of a password stored in
> a file) under certain conditions.
> 
> Used existing (un)lock_trace functions to deal with the issue by calling
> ptrace_may_access() permission checks.

This doesn't apply to current mainline.  Please redo, retest, resend?

[kernel-hardening] Re: [PATCH] proc: fix races against execve() of /proc/PID/{fd/,fdinfo/,fdinfo/*}

[Andrew Morton]

On Fri, 26 Aug 2011 17:29:09 +0400
Vasiliy Kulikov <segoon@openwall.com> wrote:

> fd* files are restricted to the task's owner, and other users may not
> get direct access to them.  But one may open any of these files and run
> any setuid program, keeping opened file descriptors.  As there are
> permission checks on open(), but not on readdir() and read(), operations
> on the kept file descriptors will not be checked.  It makes it possible
> to violate procfs permission model.
> 
> Reading fdinfo/* may disclosure current fds' position and flags, reading
> directory contents of fdinfo/ and fd/ may disclosure the number of opened
> files by the target task.  This information is not sensible per se, but
> it can reveal some private information (like length of a password stored in
> a file) under certain conditions.
> 
> Used existing (un)lock_trace functions to deal with the issue by calling
> ptrace_may_access() permission checks.

This doesn't apply to current mainline.  Please redo, retest, resend?

[PATCH v2] proc: fix races against execve() of /proc/PID/fd**

[Vasiliy Kulikov]

fd* files are restricted to the task's owner, and other users may not
get direct access to them.  But one may open any of these files and run
any setuid program, keeping opened file descriptors.  As there are
permission checks on open(), but not on stat(), readdir(), and read(),
operations on the kept file descriptors will not be checked.  It makes
it possible to violate procfs permission model.

Reading fdinfo/* may disclosure current fds' position and flags, reading
directory contents of fdinfo/ and fd/ may disclosure the number of opened
files by the target task.  This information is not sensible per se, but
it can reveal some private information (like length of a password stored in
a file) under certain conditions.

Used existing (un)lock_trace functions to check for ptrace_may_access(),
but instead of using EPERM return code from it use EACCES to be
consistent with existing proc_pid_follow_link()/proc_pid_readlink()
return codes.  If they'd differ, attacker can guess what fds exist by
analyzing stat() return code.  Patched handlers: stat() for fd/*, stat()
and read() for fdindo/*, readdir() and lookup() for fd/ and fdinfo/.

v2 - Rebased to v3.1-rc3.
   - Handle stat() case.

CC: Stable Tree <stable@kernel.org>
Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 fs/proc/base.c |  145 +++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 101 insertions(+), 44 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5eb0206..c162080 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1665,12 +1665,43 @@ out:
 	return error;
 }
 
+static int proc_pid_fd_link_getattr(struct vfsmount *mnt, struct dentry *dentry,
+		struct kstat *stat)
+{
+	struct inode *inode = dentry->d_inode;
+	struct task_struct *task = get_proc_task(inode);
+	int rc;
+
+	rc = -EACCES;
+	if (lock_trace(task))
+		goto out_task;
+
+	generic_fillattr(inode, stat);
+	unlock_trace(task);
+	put_task_struct(task);
+	rc = 0;
+out_task:
+	return rc;
+}
+
 static const struct inode_operations proc_pid_link_inode_operations = {
 	.readlink	= proc_pid_readlink,
 	.follow_link	= proc_pid_follow_link,
 	.setattr	= proc_setattr,
 };
 
+static const struct inode_operations proc_fdinfo_link_inode_operations = {
+	.setattr	= proc_setattr,
+	.getattr	= proc_pid_fd_link_getattr,
+};
+
+static const struct inode_operations proc_fd_link_inode_operations = {
+	.readlink	= proc_pid_readlink,
+	.follow_link	= proc_pid_follow_link,
+	.setattr	= proc_setattr,
+	.getattr	= proc_pid_fd_link_getattr,
+};
+
 
 /* building an inode */
 
@@ -1902,49 +1933,61 @@ out:
 
 static int proc_fd_info(struct inode *inode, struct path *path, char *info)
 {
-	struct task_struct *task = get_proc_task(inode);
-	struct files_struct *files = NULL;
+	struct task_struct *task;
+	struct files_struct *files;
 	struct file *file;
 	int fd = proc_fd(inode);
+	int rc;
 
-	if (task) {
-		files = get_files_struct(task);
-		put_task_struct(task);
-	}
-	if (files) {
-		/*
-		 * We are not taking a ref to the file structure, so we must
-		 * hold ->file_lock.
-		 */
-		spin_lock(&files->file_lock);
-		file = fcheck_files(files, fd);
-		if (file) {
-			unsigned int f_flags;
-			struct fdtable *fdt;
-
-			fdt = files_fdtable(files);
-			f_flags = file->f_flags & ~O_CLOEXEC;
-			if (FD_ISSET(fd, fdt->close_on_exec))
-				f_flags |= O_CLOEXEC;
-
-			if (path) {
-				*path = file->f_path;
-				path_get(&file->f_path);
-			}
-			if (info)
-				snprintf(info, PROC_FDINFO_MAX,
-					 "pos:\t%lli\n"
-					 "flags:\t0%o\n",
-					 (long long) file->f_pos,
-					 f_flags);
-			spin_unlock(&files->file_lock);
-			put_files_struct(files);
-			return 0;
+	task = get_proc_task(inode);
+	if (!task)
+		return -ENOENT;
+
+	rc = -EACCES;
+	if (lock_trace(task))
+		goto out_task;
+
+	rc = -ENOENT;
+	files = get_files_struct(task);
+	if (files == NULL)
+		goto out_unlock;
+
+	/*
+	 * We are not taking a ref to the file structure, so we must
+	 * hold ->file_lock.
+	 */
+	spin_lock(&files->file_lock);
+	file = fcheck_files(files, fd);
+	if (file) {
+		unsigned int f_flags;
+		struct fdtable *fdt;
+
+		fdt = files_fdtable(files);
+		f_flags = file->f_flags & ~O_CLOEXEC;
+		if (FD_ISSET(fd, fdt->close_on_exec))
+			f_flags |= O_CLOEXEC;
+
+		if (path) {
+			*path = file->f_path;
+			path_get(&file->f_path);
 		}
-		spin_unlock(&files->file_lock);
-		put_files_struct(files);
-	}
-	return -ENOENT;
+		if (info)
+			snprintf(info, PROC_FDINFO_MAX,
+				 "pos:\t%lli\n"
+				 "flags:\t0%o\n",
+				 (long long) file->f_pos,
+				 f_flags);
+		rc = 0;
+	} else
+		rc = -ENOENT;
+	spin_unlock(&files->file_lock);
+	put_files_struct(files);
+
+out_unlock:
+	unlock_trace(task);
+out_task:
+	put_task_struct(task);
+	return rc;
 }
 
 static int proc_fd_link(struct inode *inode, struct path *path)
@@ -2039,7 +2082,7 @@ static struct dentry *proc_fd_instantiate(struct inode *dir,
 	spin_unlock(&files->file_lock);
 	put_files_struct(files);
 
-	inode->i_op = &proc_pid_link_inode_operations;
+	inode->i_op = &proc_fd_link_inode_operations;
 	inode->i_size = 64;
 	ei->op.proc_get_link = proc_fd_link;
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
@@ -2071,7 +2114,12 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 	if (fd == ~0U)
 		goto out;
 
+	result = ERR_PTR(-EACCES);
+	if (lock_trace(task))
+		goto out;
+
 	result = instantiate(dir, dentry, task, &fd);
+	unlock_trace(task);
 out:
 	put_task_struct(task);
 out_no_task:
@@ -2091,23 +2139,28 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 	retval = -ENOENT;
 	if (!p)
 		goto out_no_task;
+
+	retval = -EACCES;
+	if (lock_trace(p))
+		goto out;
+
 	retval = 0;
 
 	fd = filp->f_pos;
 	switch (fd) {
 		case 0:
 			if (filldir(dirent, ".", 1, 0, inode->i_ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		case 1:
 			ino = parent_ino(dentry);
 			if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		default:
 			files = get_files_struct(p);
 			if (!files)
-				goto out;
+				goto out_unlock;
 			rcu_read_lock();
 			for (fd = filp->f_pos-2;
 			     fd < files_fdtable(files)->max_fds;
@@ -2131,6 +2184,9 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 			rcu_read_unlock();
 			put_files_struct(files);
 	}
+
+out_unlock:
+	unlock_trace(p);
 out:
 	put_task_struct(p);
 out_no_task:
@@ -2187,6 +2243,7 @@ static int proc_fd_permission(struct inode *inode, int mask)
 /*
  * proc directories can do almost nothing..
  */
+
 static const struct inode_operations proc_fd_inode_operations = {
 	.lookup		= proc_lookupfd,
 	.permission	= proc_fd_permission,
@@ -2208,6 +2265,7 @@ static struct dentry *proc_fdinfo_instantiate(struct inode *dir,
 	ei->fd = fd;
 	inode->i_mode = S_IFREG | S_IRUSR;
 	inode->i_fop = &proc_fdinfo_file_operations;
+	inode->i_op = &proc_fdinfo_link_inode_operations;
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
 	d_add(dentry, inode);
 	/* Close the race of the process dying before we return the dentry */
-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

[kernel-hardening] [PATCH v2] proc: fix races against execve() of /proc/PID/fd**

[Vasiliy Kulikov]

fd* files are restricted to the task's owner, and other users may not
get direct access to them.  But one may open any of these files and run
any setuid program, keeping opened file descriptors.  As there are
permission checks on open(), but not on stat(), readdir(), and read(),
operations on the kept file descriptors will not be checked.  It makes
it possible to violate procfs permission model.

Reading fdinfo/* may disclosure current fds' position and flags, reading
directory contents of fdinfo/ and fd/ may disclosure the number of opened
files by the target task.  This information is not sensible per se, but
it can reveal some private information (like length of a password stored in
a file) under certain conditions.

Used existing (un)lock_trace functions to check for ptrace_may_access(),
but instead of using EPERM return code from it use EACCES to be
consistent with existing proc_pid_follow_link()/proc_pid_readlink()
return codes.  If they'd differ, attacker can guess what fds exist by
analyzing stat() return code.  Patched handlers: stat() for fd/*, stat()
and read() for fdindo/*, readdir() and lookup() for fd/ and fdinfo/.

v2 - Rebased to v3.1-rc3.
   - Handle stat() case.

CC: Stable Tree <stable@kernel.org>
Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 fs/proc/base.c |  145 +++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 101 insertions(+), 44 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5eb0206..c162080 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1665,12 +1665,43 @@ out:
 	return error;
 }
 
+static int proc_pid_fd_link_getattr(struct vfsmount *mnt, struct dentry *dentry,
+		struct kstat *stat)
+{
+	struct inode *inode = dentry->d_inode;
+	struct task_struct *task = get_proc_task(inode);
+	int rc;
+
+	rc = -EACCES;
+	if (lock_trace(task))
+		goto out_task;
+
+	generic_fillattr(inode, stat);
+	unlock_trace(task);
+	put_task_struct(task);
+	rc = 0;
+out_task:
+	return rc;
+}
+
 static const struct inode_operations proc_pid_link_inode_operations = {
 	.readlink	= proc_pid_readlink,
 	.follow_link	= proc_pid_follow_link,
 	.setattr	= proc_setattr,
 };
 
+static const struct inode_operations proc_fdinfo_link_inode_operations = {
+	.setattr	= proc_setattr,
+	.getattr	= proc_pid_fd_link_getattr,
+};
+
+static const struct inode_operations proc_fd_link_inode_operations = {
+	.readlink	= proc_pid_readlink,
+	.follow_link	= proc_pid_follow_link,
+	.setattr	= proc_setattr,
+	.getattr	= proc_pid_fd_link_getattr,
+};
+
 
 /* building an inode */
 
@@ -1902,49 +1933,61 @@ out:
 
 static int proc_fd_info(struct inode *inode, struct path *path, char *info)
 {
-	struct task_struct *task = get_proc_task(inode);
-	struct files_struct *files = NULL;
+	struct task_struct *task;
+	struct files_struct *files;
 	struct file *file;
 	int fd = proc_fd(inode);
+	int rc;
 
-	if (task) {
-		files = get_files_struct(task);
-		put_task_struct(task);
-	}
-	if (files) {
-		/*
-		 * We are not taking a ref to the file structure, so we must
-		 * hold ->file_lock.
-		 */
-		spin_lock(&files->file_lock);
-		file = fcheck_files(files, fd);
-		if (file) {
-			unsigned int f_flags;
-			struct fdtable *fdt;
-
-			fdt = files_fdtable(files);
-			f_flags = file->f_flags & ~O_CLOEXEC;
-			if (FD_ISSET(fd, fdt->close_on_exec))
-				f_flags |= O_CLOEXEC;
-
-			if (path) {
-				*path = file->f_path;
-				path_get(&file->f_path);
-			}
-			if (info)
-				snprintf(info, PROC_FDINFO_MAX,
-					 "pos:\t%lli\n"
-					 "flags:\t0%o\n",
-					 (long long) file->f_pos,
-					 f_flags);
-			spin_unlock(&files->file_lock);
-			put_files_struct(files);
-			return 0;
+	task = get_proc_task(inode);
+	if (!task)
+		return -ENOENT;
+
+	rc = -EACCES;
+	if (lock_trace(task))
+		goto out_task;
+
+	rc = -ENOENT;
+	files = get_files_struct(task);
+	if (files == NULL)
+		goto out_unlock;
+
+	/*
+	 * We are not taking a ref to the file structure, so we must
+	 * hold ->file_lock.
+	 */
+	spin_lock(&files->file_lock);
+	file = fcheck_files(files, fd);
+	if (file) {
+		unsigned int f_flags;
+		struct fdtable *fdt;
+
+		fdt = files_fdtable(files);
+		f_flags = file->f_flags & ~O_CLOEXEC;
+		if (FD_ISSET(fd, fdt->close_on_exec))
+			f_flags |= O_CLOEXEC;
+
+		if (path) {
+			*path = file->f_path;
+			path_get(&file->f_path);
 		}
-		spin_unlock(&files->file_lock);
-		put_files_struct(files);
-	}
-	return -ENOENT;
+		if (info)
+			snprintf(info, PROC_FDINFO_MAX,
+				 "pos:\t%lli\n"
+				 "flags:\t0%o\n",
+				 (long long) file->f_pos,
+				 f_flags);
+		rc = 0;
+	} else
+		rc = -ENOENT;
+	spin_unlock(&files->file_lock);
+	put_files_struct(files);
+
+out_unlock:
+	unlock_trace(task);
+out_task:
+	put_task_struct(task);
+	return rc;
 }
 
 static int proc_fd_link(struct inode *inode, struct path *path)
@@ -2039,7 +2082,7 @@ static struct dentry *proc_fd_instantiate(struct inode *dir,
 	spin_unlock(&files->file_lock);
 	put_files_struct(files);
 
-	inode->i_op = &proc_pid_link_inode_operations;
+	inode->i_op = &proc_fd_link_inode_operations;
 	inode->i_size = 64;
 	ei->op.proc_get_link = proc_fd_link;
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
@@ -2071,7 +2114,12 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 	if (fd == ~0U)
 		goto out;
 
+	result = ERR_PTR(-EACCES);
+	if (lock_trace(task))
+		goto out;
+
 	result = instantiate(dir, dentry, task, &fd);
+	unlock_trace(task);
 out:
 	put_task_struct(task);
 out_no_task:
@@ -2091,23 +2139,28 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 	retval = -ENOENT;
 	if (!p)
 		goto out_no_task;
+
+	retval = -EACCES;
+	if (lock_trace(p))
+		goto out;
+
 	retval = 0;
 
 	fd = filp->f_pos;
 	switch (fd) {
 		case 0:
 			if (filldir(dirent, ".", 1, 0, inode->i_ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		case 1:
 			ino = parent_ino(dentry);
 			if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		default:
 			files = get_files_struct(p);
 			if (!files)
-				goto out;
+				goto out_unlock;
 			rcu_read_lock();
 			for (fd = filp->f_pos-2;
 			     fd < files_fdtable(files)->max_fds;
@@ -2131,6 +2184,9 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 			rcu_read_unlock();
 			put_files_struct(files);
 	}
+
+out_unlock:
+	unlock_trace(p);
 out:
 	put_task_struct(p);
 out_no_task:
@@ -2187,6 +2243,7 @@ static int proc_fd_permission(struct inode *inode, int mask)
 /*
  * proc directories can do almost nothing..
  */
+
 static const struct inode_operations proc_fd_inode_operations = {
 	.lookup		= proc_lookupfd,
 	.permission	= proc_fd_permission,
@@ -2208,6 +2265,7 @@ static struct dentry *proc_fdinfo_instantiate(struct inode *dir,
 	ei->fd = fd;
 	inode->i_mode = S_IFREG | S_IRUSR;
 	inode->i_fop = &proc_fdinfo_file_operations;
+	inode->i_op = &proc_fdinfo_link_inode_operations;
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
 	d_add(dentry, inode);
 	/* Close the race of the process dying before we return the dentry */
-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH v2] proc: fix races against execve() of /proc/PID/fd**

[Vasiliy Kulikov]

On Sat, Aug 27, 2011 at 23:01 +0400, Vasiliy Kulikov wrote:
> fd* files are restricted to the task's owner, and other users may not
> get direct access to them.  But one may open any of these files and run
> any setuid program, keeping opened file descriptors.  As there are
> permission checks on open(), but not on stat(), readdir(), and read(),
> operations on the kept file descriptors will not be checked.  It makes
> it possible to violate procfs permission model.
> 
> Reading fdinfo/* may disclosure current fds' position and flags, reading
> directory contents of fdinfo/ and fd/ may disclosure the number of opened
> files by the target task.  This information is not sensible per se, but
> it can reveal some private information (like length of a password stored in
> a file) under certain conditions.
> 
> Used existing (un)lock_trace functions to check for ptrace_may_access(),
> but instead of using EPERM return code from it use EACCES to be
> consistent with existing proc_pid_follow_link()/proc_pid_readlink()
> return codes.  If they'd differ, attacker can guess what fds exist by
> analyzing stat() return code.  Patched handlers: stat() for fd/*, stat()
> and read() for fdindo/*, readdir() and lookup() for fd/ and fdinfo/.
> 
> v2 - Rebased to v3.1-rc3.
>    - Handle stat() case.
> 
> CC: Stable Tree <stable@kernel.org>
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
> ---
...
> @@ -2187,6 +2243,7 @@ static int proc_fd_permission(struct inode *inode, int mask)
>  /*
>   * proc directories can do almost nothing..
>   */
> +
>  static const struct inode_operations proc_fd_inode_operations = {
>  	.lookup		= proc_lookupfd,
>  	.permission	= proc_fd_permission,

Oops, odd blank line.  Andrew, should I resend the patch to fix it?

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

[kernel-hardening] Re: [PATCH v2] proc: fix races against execve() of /proc/PID/fd**

[Vasiliy Kulikov]

On Sat, Aug 27, 2011 at 23:01 +0400, Vasiliy Kulikov wrote:
> fd* files are restricted to the task's owner, and other users may not
> get direct access to them.  But one may open any of these files and run
> any setuid program, keeping opened file descriptors.  As there are
> permission checks on open(), but not on stat(), readdir(), and read(),
> operations on the kept file descriptors will not be checked.  It makes
> it possible to violate procfs permission model.
> 
> Reading fdinfo/* may disclosure current fds' position and flags, reading
> directory contents of fdinfo/ and fd/ may disclosure the number of opened
> files by the target task.  This information is not sensible per se, but
> it can reveal some private information (like length of a password stored in
> a file) under certain conditions.
> 
> Used existing (un)lock_trace functions to check for ptrace_may_access(),
> but instead of using EPERM return code from it use EACCES to be
> consistent with existing proc_pid_follow_link()/proc_pid_readlink()
> return codes.  If they'd differ, attacker can guess what fds exist by
> analyzing stat() return code.  Patched handlers: stat() for fd/*, stat()
> and read() for fdindo/*, readdir() and lookup() for fd/ and fdinfo/.
> 
> v2 - Rebased to v3.1-rc3.
>    - Handle stat() case.
> 
> CC: Stable Tree <stable@kernel.org>
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
> ---
...
> @@ -2187,6 +2243,7 @@ static int proc_fd_permission(struct inode *inode, int mask)
>  /*
>   * proc directories can do almost nothing..
>   */
> +
>  static const struct inode_operations proc_fd_inode_operations = {
>  	.lookup		= proc_lookupfd,
>  	.permission	= proc_fd_permission,

Oops, odd blank line.  Andrew, should I resend the patch to fix it?

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH v2] proc: fix races against execve() of /proc/PID/fd**

[Cyrill Gorcunov]

On Sat, Aug 27, 2011 at 11:01:47PM +0400, Vasiliy Kulikov wrote:
...
>  
> +static int proc_pid_fd_link_getattr(struct vfsmount *mnt, struct dentry *dentry,
> +		struct kstat *stat)
> +{
> +	struct inode *inode = dentry->d_inode;
> +	struct task_struct *task = get_proc_task(inode);
> +	int rc;
> +

Are we sure if the task will be always valid here?

	if (!task)
		return -ENOENT;

	Cyrill

[kernel-hardening] Re: [PATCH v2] proc: fix races against execve() of /proc/PID/fd**

[Cyrill Gorcunov]

On Sat, Aug 27, 2011 at 11:01:47PM +0400, Vasiliy Kulikov wrote:
...
>  
> +static int proc_pid_fd_link_getattr(struct vfsmount *mnt, struct dentry *dentry,
> +		struct kstat *stat)
> +{
> +	struct inode *inode = dentry->d_inode;
> +	struct task_struct *task = get_proc_task(inode);
> +	int rc;
> +

Are we sure if the task will be always valid here?

	if (!task)
		return -ENOENT;

	Cyrill

Re: [PATCH v2] proc: fix races against execve() of /proc/PID/fd**

[Vasiliy Kulikov]

Hi Cyrill,

On Sun, Aug 28, 2011 at 13:25 +0400, Cyrill Gorcunov wrote:
> On Sat, Aug 27, 2011 at 11:01:47PM +0400, Vasiliy Kulikov wrote:
> ...
> >  
> > +static int proc_pid_fd_link_getattr(struct vfsmount *mnt, struct dentry *dentry,
> > +		struct kstat *stat)
> > +{
> > +	struct inode *inode = dentry->d_inode;
> > +	struct task_struct *task = get_proc_task(inode);
> > +	int rc;
> > +
> 
> Are we sure if the task will be always valid here?
> 
> 	if (!task)
> 		return -ENOENT;

No, you're right.  It needs (task == NULL) check.  Thank you!

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

[kernel-hardening] Re: [PATCH v2] proc: fix races against execve() of /proc/PID/fd**

[Vasiliy Kulikov]

Hi Cyrill,

On Sun, Aug 28, 2011 at 13:25 +0400, Cyrill Gorcunov wrote:
> On Sat, Aug 27, 2011 at 11:01:47PM +0400, Vasiliy Kulikov wrote:
> ...
> >  
> > +static int proc_pid_fd_link_getattr(struct vfsmount *mnt, struct dentry *dentry,
> > +		struct kstat *stat)
> > +{
> > +	struct inode *inode = dentry->d_inode;
> > +	struct task_struct *task = get_proc_task(inode);
> > +	int rc;
> > +
> 
> Are we sure if the task will be always valid here?
> 
> 	if (!task)
> 		return -ENOENT;

No, you're right.  It needs (task == NULL) check.  Thank you!

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

[PATCH v3] proc: fix races against execve() of /proc/PID/fd**

[Vasiliy Kulikov]

fd* files are restricted to the task's owner, and other users may not
get direct access to them.  But one may open any of these files and run
any setuid program, keeping opened file descriptors.  As there are
permission checks on open(), but not on readdir() and read(), operations
on the kept file descriptors will not be checked.  It makes it possible
to violate procfs permission model.

Reading fdinfo/* may disclosure current fds' position and flags, reading
directory contents of fdinfo/ and fd/ may disclosure the number of opened
files by the target task.  This information is not sensible per se, but
it can reveal some private information (like length of a password stored in
a file) under certain conditions.

Used existing (un)lock_trace functions to check for ptrace_may_access(),
but instead of using EPERM return code from it use EACCES to be
consistent with existing proc_pid_follow_link()/proc_pid_readlink()
return code.  If they differ, attacker can guess what fds exist by
analyzing stat() return code.  Patched handlers: stat() for fd/*, stat()
and read() for fdindo/*, readdir() and lookup() for fd/ and fdinfo/.

v3 - Added a check of get_proc_task() result for NULL.

v2 - Rebased to v3.1-rc3.
   - Handle stat() case.

CC: Stable Tree <stable@kernel.org>
Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 fs/proc/base.c |  146 +++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 103 insertions(+), 43 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5eb0206..b65bd88 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1665,12 +1665,46 @@ out:
 	return error;
 }
 
+static int proc_pid_fd_link_getattr(struct vfsmount *mnt, struct dentry *dentry,
+		struct kstat *stat)
+{
+	struct inode *inode = dentry->d_inode;
+	struct task_struct *task = get_proc_task(inode);
+	int rc;
+
+	if (task == NULL)
+		return -ESRCH;
+
+	rc = -EACCES;
+	if (lock_trace(task))
+		goto out_task;
+
+	generic_fillattr(inode, stat);
+	unlock_trace(task);
+	put_task_struct(task);
+	rc = 0;
+out_task:
+	return rc;
+}
+
 static const struct inode_operations proc_pid_link_inode_operations = {
 	.readlink	= proc_pid_readlink,
 	.follow_link	= proc_pid_follow_link,
 	.setattr	= proc_setattr,
 };
 
+static const struct inode_operations proc_fdinfo_link_inode_operations = {
+	.setattr	= proc_setattr,
+	.getattr	= proc_pid_fd_link_getattr,
+};
+
+static const struct inode_operations proc_fd_link_inode_operations = {
+	.readlink	= proc_pid_readlink,
+	.follow_link	= proc_pid_follow_link,
+	.setattr	= proc_setattr,
+	.getattr	= proc_pid_fd_link_getattr,
+};
+
 
 /* building an inode */
 
@@ -1902,49 +1936,61 @@ out:
 
 static int proc_fd_info(struct inode *inode, struct path *path, char *info)
 {
-	struct task_struct *task = get_proc_task(inode);
-	struct files_struct *files = NULL;
+	struct task_struct *task;
+	struct files_struct *files;
 	struct file *file;
 	int fd = proc_fd(inode);
+	int rc;
 
-	if (task) {
-		files = get_files_struct(task);
-		put_task_struct(task);
-	}
-	if (files) {
-		/*
-		 * We are not taking a ref to the file structure, so we must
-		 * hold ->file_lock.
-		 */
-		spin_lock(&files->file_lock);
-		file = fcheck_files(files, fd);
-		if (file) {
-			unsigned int f_flags;
-			struct fdtable *fdt;
-
-			fdt = files_fdtable(files);
-			f_flags = file->f_flags & ~O_CLOEXEC;
-			if (FD_ISSET(fd, fdt->close_on_exec))
-				f_flags |= O_CLOEXEC;
-
-			if (path) {
-				*path = file->f_path;
-				path_get(&file->f_path);
-			}
-			if (info)
-				snprintf(info, PROC_FDINFO_MAX,
-					 "pos:\t%lli\n"
-					 "flags:\t0%o\n",
-					 (long long) file->f_pos,
-					 f_flags);
-			spin_unlock(&files->file_lock);
-			put_files_struct(files);
-			return 0;
+	task = get_proc_task(inode);
+	if (!task)
+		return -ENOENT;
+
+	rc = -EACCES;
+	if (lock_trace(task))
+		goto out_task;
+
+	rc = -ENOENT;
+	files = get_files_struct(task);
+	if (files == NULL)
+		goto out_unlock;
+
+	/*
+	 * We are not taking a ref to the file structure, so we must
+	 * hold ->file_lock.
+	 */
+	spin_lock(&files->file_lock);
+	file = fcheck_files(files, fd);
+	if (file) {
+		unsigned int f_flags;
+		struct fdtable *fdt;
+
+		fdt = files_fdtable(files);
+		f_flags = file->f_flags & ~O_CLOEXEC;
+		if (FD_ISSET(fd, fdt->close_on_exec))
+			f_flags |= O_CLOEXEC;
+
+		if (path) {
+			*path = file->f_path;
+			path_get(&file->f_path);
 		}
-		spin_unlock(&files->file_lock);
-		put_files_struct(files);
-	}
-	return -ENOENT;
+		if (info)
+			snprintf(info, PROC_FDINFO_MAX,
+				 "pos:\t%lli\n"
+				 "flags:\t0%o\n",
+				 (long long) file->f_pos,
+				 f_flags);
+		rc = 0;
+	} else
+		rc = -ENOENT;
+	spin_unlock(&files->file_lock);
+	put_files_struct(files);
+
+out_unlock:
+	unlock_trace(task);
+out_task:
+	put_task_struct(task);
+	return rc;
 }
 
 static int proc_fd_link(struct inode *inode, struct path *path)
@@ -2039,7 +2085,7 @@ static struct dentry *proc_fd_instantiate(struct inode *dir,
 	spin_unlock(&files->file_lock);
 	put_files_struct(files);
 
-	inode->i_op = &proc_pid_link_inode_operations;
+	inode->i_op = &proc_fd_link_inode_operations;
 	inode->i_size = 64;
 	ei->op.proc_get_link = proc_fd_link;
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
@@ -2071,7 +2117,12 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 	if (fd == ~0U)
 		goto out;
 
+	result = ERR_PTR(-EACCES);
+	if (lock_trace(task))
+		goto out;
+
 	result = instantiate(dir, dentry, task, &fd);
+	unlock_trace(task);
 out:
 	put_task_struct(task);
 out_no_task:
@@ -2091,23 +2142,28 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 	retval = -ENOENT;
 	if (!p)
 		goto out_no_task;
+
+	retval = -EACCES;
+	if (lock_trace(p))
+		goto out;
+
 	retval = 0;
 
 	fd = filp->f_pos;
 	switch (fd) {
 		case 0:
 			if (filldir(dirent, ".", 1, 0, inode->i_ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		case 1:
 			ino = parent_ino(dentry);
 			if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		default:
 			files = get_files_struct(p);
 			if (!files)
-				goto out;
+				goto out_unlock;
 			rcu_read_lock();
 			for (fd = filp->f_pos-2;
 			     fd < files_fdtable(files)->max_fds;
@@ -2131,6 +2187,9 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 			rcu_read_unlock();
 			put_files_struct(files);
 	}
+
+out_unlock:
+	unlock_trace(p);
 out:
 	put_task_struct(p);
 out_no_task:
@@ -2208,6 +2267,7 @@ static struct dentry *proc_fdinfo_instantiate(struct inode *dir,
 	ei->fd = fd;
 	inode->i_mode = S_IFREG | S_IRUSR;
 	inode->i_fop = &proc_fdinfo_file_operations;
+	inode->i_op = &proc_fdinfo_link_inode_operations;
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
 	d_add(dentry, inode);
 	/* Close the race of the process dying before we return the dentry */
-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

[kernel-hardening] [PATCH v3] proc: fix races against execve() of /proc/PID/fd**

[Vasiliy Kulikov]

fd* files are restricted to the task's owner, and other users may not
get direct access to them.  But one may open any of these files and run
any setuid program, keeping opened file descriptors.  As there are
permission checks on open(), but not on readdir() and read(), operations
on the kept file descriptors will not be checked.  It makes it possible
to violate procfs permission model.

Reading fdinfo/* may disclosure current fds' position and flags, reading
directory contents of fdinfo/ and fd/ may disclosure the number of opened
files by the target task.  This information is not sensible per se, but
it can reveal some private information (like length of a password stored in
a file) under certain conditions.

Used existing (un)lock_trace functions to check for ptrace_may_access(),
but instead of using EPERM return code from it use EACCES to be
consistent with existing proc_pid_follow_link()/proc_pid_readlink()
return code.  If they differ, attacker can guess what fds exist by
analyzing stat() return code.  Patched handlers: stat() for fd/*, stat()
and read() for fdindo/*, readdir() and lookup() for fd/ and fdinfo/.

v3 - Added a check of get_proc_task() result for NULL.

v2 - Rebased to v3.1-rc3.
   - Handle stat() case.

CC: Stable Tree <stable@kernel.org>
Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 fs/proc/base.c |  146 +++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 103 insertions(+), 43 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5eb0206..b65bd88 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1665,12 +1665,46 @@ out:
 	return error;
 }
 
+static int proc_pid_fd_link_getattr(struct vfsmount *mnt, struct dentry *dentry,
+		struct kstat *stat)
+{
+	struct inode *inode = dentry->d_inode;
+	struct task_struct *task = get_proc_task(inode);
+	int rc;
+
+	if (task == NULL)
+		return -ESRCH;
+
+	rc = -EACCES;
+	if (lock_trace(task))
+		goto out_task;
+
+	generic_fillattr(inode, stat);
+	unlock_trace(task);
+	put_task_struct(task);
+	rc = 0;
+out_task:
+	return rc;
+}
+
 static const struct inode_operations proc_pid_link_inode_operations = {
 	.readlink	= proc_pid_readlink,
 	.follow_link	= proc_pid_follow_link,
 	.setattr	= proc_setattr,
 };
 
+static const struct inode_operations proc_fdinfo_link_inode_operations = {
+	.setattr	= proc_setattr,
+	.getattr	= proc_pid_fd_link_getattr,
+};
+
+static const struct inode_operations proc_fd_link_inode_operations = {
+	.readlink	= proc_pid_readlink,
+	.follow_link	= proc_pid_follow_link,
+	.setattr	= proc_setattr,
+	.getattr	= proc_pid_fd_link_getattr,
+};
+
 
 /* building an inode */
 
@@ -1902,49 +1936,61 @@ out:
 
 static int proc_fd_info(struct inode *inode, struct path *path, char *info)
 {
-	struct task_struct *task = get_proc_task(inode);
-	struct files_struct *files = NULL;
+	struct task_struct *task;
+	struct files_struct *files;
 	struct file *file;
 	int fd = proc_fd(inode);
+	int rc;
 
-	if (task) {
-		files = get_files_struct(task);
-		put_task_struct(task);
-	}
-	if (files) {
-		/*
-		 * We are not taking a ref to the file structure, so we must
-		 * hold ->file_lock.
-		 */
-		spin_lock(&files->file_lock);
-		file = fcheck_files(files, fd);
-		if (file) {
-			unsigned int f_flags;
-			struct fdtable *fdt;
-
-			fdt = files_fdtable(files);
-			f_flags = file->f_flags & ~O_CLOEXEC;
-			if (FD_ISSET(fd, fdt->close_on_exec))
-				f_flags |= O_CLOEXEC;
-
-			if (path) {
-				*path = file->f_path;
-				path_get(&file->f_path);
-			}
-			if (info)
-				snprintf(info, PROC_FDINFO_MAX,
-					 "pos:\t%lli\n"
-					 "flags:\t0%o\n",
-					 (long long) file->f_pos,
-					 f_flags);
-			spin_unlock(&files->file_lock);
-			put_files_struct(files);
-			return 0;
+	task = get_proc_task(inode);
+	if (!task)
+		return -ENOENT;
+
+	rc = -EACCES;
+	if (lock_trace(task))
+		goto out_task;
+
+	rc = -ENOENT;
+	files = get_files_struct(task);
+	if (files == NULL)
+		goto out_unlock;
+
+	/*
+	 * We are not taking a ref to the file structure, so we must
+	 * hold ->file_lock.
+	 */
+	spin_lock(&files->file_lock);
+	file = fcheck_files(files, fd);
+	if (file) {
+		unsigned int f_flags;
+		struct fdtable *fdt;
+
+		fdt = files_fdtable(files);
+		f_flags = file->f_flags & ~O_CLOEXEC;
+		if (FD_ISSET(fd, fdt->close_on_exec))
+			f_flags |= O_CLOEXEC;
+
+		if (path) {
+			*path = file->f_path;
+			path_get(&file->f_path);
 		}
-		spin_unlock(&files->file_lock);
-		put_files_struct(files);
-	}
-	return -ENOENT;
+		if (info)
+			snprintf(info, PROC_FDINFO_MAX,
+				 "pos:\t%lli\n"
+				 "flags:\t0%o\n",
+				 (long long) file->f_pos,
+				 f_flags);
+		rc = 0;
+	} else
+		rc = -ENOENT;
+	spin_unlock(&files->file_lock);
+	put_files_struct(files);
+
+out_unlock:
+	unlock_trace(task);
+out_task:
+	put_task_struct(task);
+	return rc;
 }
 
 static int proc_fd_link(struct inode *inode, struct path *path)
@@ -2039,7 +2085,7 @@ static struct dentry *proc_fd_instantiate(struct inode *dir,
 	spin_unlock(&files->file_lock);
 	put_files_struct(files);
 
-	inode->i_op = &proc_pid_link_inode_operations;
+	inode->i_op = &proc_fd_link_inode_operations;
 	inode->i_size = 64;
 	ei->op.proc_get_link = proc_fd_link;
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
@@ -2071,7 +2117,12 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 	if (fd == ~0U)
 		goto out;
 
+	result = ERR_PTR(-EACCES);
+	if (lock_trace(task))
+		goto out;
+
 	result = instantiate(dir, dentry, task, &fd);
+	unlock_trace(task);
 out:
 	put_task_struct(task);
 out_no_task:
@@ -2091,23 +2142,28 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 	retval = -ENOENT;
 	if (!p)
 		goto out_no_task;
+
+	retval = -EACCES;
+	if (lock_trace(p))
+		goto out;
+
 	retval = 0;
 
 	fd = filp->f_pos;
 	switch (fd) {
 		case 0:
 			if (filldir(dirent, ".", 1, 0, inode->i_ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		case 1:
 			ino = parent_ino(dentry);
 			if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0)
-				goto out;
+				goto out_unlock;
 			filp->f_pos++;
 		default:
 			files = get_files_struct(p);
 			if (!files)
-				goto out;
+				goto out_unlock;
 			rcu_read_lock();
 			for (fd = filp->f_pos-2;
 			     fd < files_fdtable(files)->max_fds;
@@ -2131,6 +2187,9 @@ static int proc_readfd_common(struct file * filp, void * dirent,
 			rcu_read_unlock();
 			put_files_struct(files);
 	}
+
+out_unlock:
+	unlock_trace(p);
 out:
 	put_task_struct(p);
 out_no_task:
@@ -2208,6 +2267,7 @@ static struct dentry *proc_fdinfo_instantiate(struct inode *dir,
 	ei->fd = fd;
 	inode->i_mode = S_IFREG | S_IRUSR;
 	inode->i_fop = &proc_fdinfo_file_operations;
+	inode->i_op = &proc_fdinfo_link_inode_operations;
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
 	d_add(dentry, inode);
 	/* Close the race of the process dying before we return the dentry */
-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH v3] proc: fix races against execve() of /proc/PID/fd**

[Andrew Morton]

On Mon, 29 Aug 2011 22:00:11 +0400
Vasiliy Kulikov <segoon@openwall.com> wrote:

> fd* files are restricted to the task's owner, and other users may not
> get direct access to them.  But one may open any of these files and run
> any setuid program, keeping opened file descriptors.  As there are
> permission checks on open(), but not on readdir() and read(), operations
> on the kept file descriptors will not be checked.  It makes it possible
> to violate procfs permission model.
> 
> Reading fdinfo/* may disclosure current fds' position and flags, reading
> directory contents of fdinfo/ and fd/ may disclosure the number of opened
> files by the target task.  This information is not sensible per se, but
> it can reveal some private information (like length of a password stored in
> a file) under certain conditions.
> 
> Used existing (un)lock_trace functions to check for ptrace_may_access(),
> but instead of using EPERM return code from it use EACCES to be
> consistent with existing proc_pid_follow_link()/proc_pid_readlink()
> return code.  If they differ, attacker can guess what fds exist by
> analyzing stat() return code.  Patched handlers: stat() for fd/*, stat()
> and read() for fdindo/*, readdir() and lookup() for fd/ and fdinfo/.
> 
> +	rc = -EACCES;
> +	if (lock_trace(task))
> +		goto out_task;
> +
>
> ...
>
> +	rc = -EACCES;
> +	if (lock_trace(task))
> +		goto out_task;
> +
>
> ...
>
> +	result = ERR_PTR(-EACCES);
> +	if (lock_trace(task))
> +		goto out;
> +
>
> ...
>
> +
> +	retval = -EACCES;
> +	if (lock_trace(p))
> +		goto out;
> +
>
> ...
>

lock_trace() can return -EPERM, and it can return whatever
mutex_lock_killable() returned (-EINTR, perhaps other things?).

The patch simply overwrites this return value with -EACCES.  Is this
deliberate and correct?  If so, please explain?

[kernel-hardening] Re: [PATCH v3] proc: fix races against execve() of /proc/PID/fd**

[Andrew Morton]

On Mon, 29 Aug 2011 22:00:11 +0400
Vasiliy Kulikov <segoon@openwall.com> wrote:

> fd* files are restricted to the task's owner, and other users may not
> get direct access to them.  But one may open any of these files and run
> any setuid program, keeping opened file descriptors.  As there are
> permission checks on open(), but not on readdir() and read(), operations
> on the kept file descriptors will not be checked.  It makes it possible
> to violate procfs permission model.
> 
> Reading fdinfo/* may disclosure current fds' position and flags, reading
> directory contents of fdinfo/ and fd/ may disclosure the number of opened
> files by the target task.  This information is not sensible per se, but
> it can reveal some private information (like length of a password stored in
> a file) under certain conditions.
> 
> Used existing (un)lock_trace functions to check for ptrace_may_access(),
> but instead of using EPERM return code from it use EACCES to be
> consistent with existing proc_pid_follow_link()/proc_pid_readlink()
> return code.  If they differ, attacker can guess what fds exist by
> analyzing stat() return code.  Patched handlers: stat() for fd/*, stat()
> and read() for fdindo/*, readdir() and lookup() for fd/ and fdinfo/.
> 
> +	rc = -EACCES;
> +	if (lock_trace(task))
> +		goto out_task;
> +
>
> ...
>
> +	rc = -EACCES;
> +	if (lock_trace(task))
> +		goto out_task;
> +
>
> ...
>
> +	result = ERR_PTR(-EACCES);
> +	if (lock_trace(task))
> +		goto out;
> +
>
> ...
>
> +
> +	retval = -EACCES;
> +	if (lock_trace(p))
> +		goto out;
> +
>
> ...
>

lock_trace() can return -EPERM, and it can return whatever
mutex_lock_killable() returned (-EINTR, perhaps other things?).

The patch simply overwrites this return value with -EACCES.  Is this
deliberate and correct?  If so, please explain?

Re: [PATCH v3] proc: fix races against execve() of /proc/PID/fd**

[Andrew Morton]

On Mon, 29 Aug 2011 22:00:11 +0400
Vasiliy Kulikov <segoon@openwall.com> wrote:

> Used existing (un)lock_trace functions to check for ptrace_may_access(),
> but instead of using EPERM return code from it use EACCES to be
> consistent with existing proc_pid_follow_link()/proc_pid_readlink()
> return code.  If they differ, attacker can guess what fds exist by
> analyzing stat() return code.

doh, I missed that bit.  Fair enough.

[kernel-hardening] Re: [PATCH v3] proc: fix races against execve() of /proc/PID/fd**

[Andrew Morton]

On Mon, 29 Aug 2011 22:00:11 +0400
Vasiliy Kulikov <segoon@openwall.com> wrote:

> Used existing (un)lock_trace functions to check for ptrace_may_access(),
> but instead of using EPERM return code from it use EACCES to be
> consistent with existing proc_pid_follow_link()/proc_pid_readlink()
> return code.  If they differ, attacker can guess what fds exist by
> analyzing stat() return code.

doh, I missed that bit.  Fair enough.