[PATCH v2 0/2] proc: /proc/<pid>/environ offset fixes

[PATCH v2 0/2] proc: /proc/<pid>/environ offset fixes

[Djalal Harouni]

Hi,

This is the V2 to correctly check offsets on /proc/<pid>/environ before
reading. This was previously discussed here:
http://lkml.org/lkml/2012/7/22/79

Due to incorrect offset checks, currently one can read from aribtrary
addresses on /proc/<pid>/environ, not only the environment address range
as shown here (the same thread):
http://lkml.org/lkml/2012/7/22/163

The bug is in environ_read().


That first patch was not complete as noted by Oleg Nestrov, since even
with positive offsets we can perhaps overflow the address from where to
read the environment variables, or perhaps we can make multiple lseek()
calls with a positive offset set to 0x7fffffff, this will pass the
fs/read_write.c:lseek_execute() checks, and one can make the
'mm->env_addr + offset' point to another VMA. This will make
/proc/<pid>/environ to act like /proc/<pid>/mem.

The first version removed only negative offsets which when converted to
unsigned long will overflow the 'mm->env_addr + offset' the address from
where to start to read the environment variables, and will also pass all
the fs/read_write.c:lseek_execute() and environ_read() checks.

As suggested by Oleg Nesterov this version makes sure to fix the offset
checks, then it removes negative offsets support on /proc/<pid>/environ
since it does not need them.


Thanks to patch 'proc: clean up /proc/<pid>/environ handling'
commit b409e578d9a4ec95913e ,this is not a security issue since at
->open() there is: the ptrace check + save the current 'mm' for next
operations.


Djalal Harouni (2):
  proc: environ_read() make sure offset points to environment address range
  proc: do not allow negative offsets on /proc/<pid>/environ

 fs/proc/base.c |   22 +++++++++++++---------
  1 files changed, 13 insertions(+), 9 deletions(-)


V2:
 * Added the [PATCH 1/2] to make sure that the offset points to the
   environment address range as suggested by Oleg Nesterov.
 * Updated the [PATCH 2/2] changelog entry since we have added [PATCH 1/2]

[PATCH v2 1/2] proc: environ_read() make sure offset points to environment address range

[Djalal Harouni]

Currently the following offset and environment address range check in
environ_read() of /proc/<pid>/environ is buggy:

int this_len = mm->env_end - (mm->env_start + src);
if (this_len <= 0)
	break;

Large or negative offsets on /proc/<pid>/environ converted to 'unsigned
long' may pass this check since '(mm->env_start + src)' can overflow and
'this_len' will be positive.

This can turn /proc/<pid>/environ to act like /proc/<pid>/mem since
(mm->env_start + src) will point and read from another VMA.

There are two fixes here plus some code cleaning:
1) Fix the overflow by checking if the offset that was converted to
unsigned long will always point to the [mm->env_start, mm->env_end] address
range.

2) Remove the truncation that was made to the result of the check, storing
the result in 'int this_len' will alter its value and we can not depend on
it.

For kernels that have commit b409e578d9a4ec95913e06d8f which adds the
appropriate ptrace check and saves the 'mm' at ->open() time, this is not
a security issue.

This patch is taken from the grsecurity patch since it was just made
available.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Brad Spengler <spender@grsecurity.net>
Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/base.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2772208..39ee093 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -827,15 +827,16 @@ static ssize_t environ_read(struct file *file, char __user *buf,
 	if (!atomic_inc_not_zero(&mm->mm_users))
 		goto free;
 	while (count > 0) {
-		int this_len, retval, max_len;
+		size_t this_len, max_len;
+		int retval;
 
-		this_len = mm->env_end - (mm->env_start + src);
-
-		if (this_len <= 0)
+		if (src >= (mm->env_end - mm->env_start))
 			break;
 
-		max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
-		this_len = (this_len > max_len) ? max_len : this_len;
+		this_len = mm->env_end - (mm->env_start + src);
+
+		max_len = min_t(size_t, PAGE_SIZE, count);
+		this_len = min(max_len, this_len);
 
 		retval = access_remote_vm(mm, (mm->env_start + src),
 			page, this_len, 0);
-- 
1.7.1

[PATCH v2 2/2] proc: do not allow negative offsets on /proc/<pid>/environ

[Djalal Harouni]

__mem_open() which is called by both /proc/<pid>/environ and
/proc/<pid>/mem ->open() handlers will allow the use of negative offsets.
/proc/<pid>/mem has negative offsets but not /proc/<pid>/environ.

Clean this by moving the 'force FMODE_UNSIGNED_OFFSET flag' to mem_open()
to allow negative offsets only on /proc/<pid>/mem.

Cc: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/base.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 39ee093..1b6c84c 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -695,8 +695,6 @@ static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
 		mmput(mm);
 	}
 
-	/* OK to pass negative loff_t, we can catch out-of-range */
-	file->f_mode |= FMODE_UNSIGNED_OFFSET;
 	file->private_data = mm;
 
 	return 0;
@@ -704,7 +702,12 @@ static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
 
 static int mem_open(struct inode *inode, struct file *file)
 {
-	return __mem_open(inode, file, PTRACE_MODE_ATTACH);
+	int ret = __mem_open(inode, file, PTRACE_MODE_ATTACH);
+
+	/* OK to pass negative loff_t, we can catch out-of-range */
+	file->f_mode |= FMODE_UNSIGNED_OFFSET;
+
+	return ret;
 }
 
 static ssize_t mem_rw(struct file *file, char __user *buf,
-- 
1.7.1

Re: [PATCH v2 2/2] proc: do not allow negative offsets on /proc/<pid>/environ

[Oleg Nesterov]

On 07/24, Djalal Harouni wrote:
>
>  static int mem_open(struct inode *inode, struct file *file)
>  {
> -	return __mem_open(inode, file, PTRACE_MODE_ATTACH);
> +	int ret = __mem_open(inode, file, PTRACE_MODE_ATTACH);
> +
> +	/* OK to pass negative loff_t, we can catch out-of-range */
> +	file->f_mode |= FMODE_UNSIGNED_OFFSET;
> +
> +	return ret;
>  }

It could be even simpler, I meant

	file->f_mode |= FMODE_UNSIGNED_OFFSET;
	return __mem_open(inode, file, PTRACE_MODE_ATTACH);

Never mind, this is very minor and the patch is already in -mm.

Oleg.

Re: [PATCH v2 0/2] proc: /proc/<pid>/environ offset fixes

[Kees Cook]

Hi,

On Tue, Jul 24, 2012 at 8:29 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> This is the V2 to correctly check offsets on /proc/<pid>/environ before
> reading. This was previously discussed here:
> http://lkml.org/lkml/2012/7/22/79
>
> Due to incorrect offset checks, currently one can read from aribtrary
> addresses on /proc/<pid>/environ, not only the environment address range
> as shown here (the same thread):
> http://lkml.org/lkml/2012/7/22/163
>
> The bug is in environ_read().
> [...]
> Djalal Harouni (2):
>   proc: environ_read() make sure offset points to environment address range
>   proc: do not allow negative offsets on /proc/<pid>/environ
>
>  fs/proc/base.c |   22 +++++++++++++---------
>   1 files changed, 13 insertions(+), 9 deletions(-)

This looks good, thanks!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security