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

[PATCH] 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.

Allowing negative offsets on /proc/<pid>/environ can turn it to act like
/proc/<pid>/mem. A negative offset will pass the
fs/read_write.c:lseek_execute() and the environ_read() checks and will
point to another VMA.

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

You must be able to ptrace the target to open /proc/<pid>/environ ,so
this is not a security issue, but we should not be able to abuse it.

Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
New kernels include mm->env_start in /proc/<pid>/stat
To dump .text area: lseek() to 0x00400000 - mm->env_start

 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 2772208..9623a18 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);
+	if (!ret)
+		/* 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] proc: do not allow negative offsets on /proc/<pid>/environ

[Oleg Nesterov]

On 07/22, Djalal Harouni wrote:
>
> __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.

Probablt the patch makes sense, but I can't understand the changelog...

> Allowing negative offsets on /proc/<pid>/environ can turn it to act like
> /proc/<pid>/mem. A negative offset will pass the
> fs/read_write.c:lseek_execute() and the environ_read() checks and will
> point to another VMA.

which VMA?

environ_read() can only read the memory from [env_start, env_end], and
it should check *ppos anyway to ensure it doesn't read something else.

>  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);
> +	if (!ret)
> +		/* OK to pass negative loff_t, we can catch out-of-range */
> +		file->f_mode |= FMODE_UNSIGNED_OFFSET;
> +
> +	return ret;

I guess you can set FMODE_UNSIGNED_OFFSET unconditionally, it doesn't
matter if __mem_open() fails. But I won't insist.

Oleg.

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

[Djalal Harouni]

[-- Attachment #1: Type: text/plain, Size: 3338 bytes --]

Hi Oleg,

On Sun, Jul 22, 2012 at 10:00:49PM +0200, Oleg Nesterov wrote:
> On 07/22, Djalal Harouni wrote:
> >
> > __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.
> 
> Probablt the patch makes sense, but I can't understand the changelog...
> 
> > Allowing negative offsets on /proc/<pid>/environ can turn it to act like
> > /proc/<pid>/mem. A negative offset will pass the
> > fs/read_write.c:lseek_execute() and the environ_read() checks and will
> > point to another VMA.
> 
> which VMA?
It depends on the offset. Please see below.

> environ_read() can only read the memory from [env_start, env_end], and
> it should check *ppos anyway to ensure it doesn't read something else.
Yes I agree, but currently that's not the case, there are no checks on *ppos.
So if you pass a negative offset you will be able to read from an arbitrary
address.

I'll send another patch tomorrow to add the checks for *ppos.



Since negative offsets are allowed we can pass it to lseek():

1) ->llseek()
     -> generic_file_llseek()
        -> generic_file_llseek_size()
           -> lseek_execute()

  inside fs/read_write.c:lseek_execute() we pass the two checks and
  file->f_pos will be updated.


2) ->read()
     -> environ_read()

  inside environ_read() there is only a one check:

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


  Here 'src' is 'src = *ppos' the negative offset converted to unsigned long
  and (mm->env_start + src) can overflow and point to another VMA.

  int this_len = mm->env_end - (mm->env_start + src)
  
  'this_len' will be positive and we pass that check.


I also don't like the truncation of the result to 'int this_len'



A quick example to reproduce it:
New kernels /proc/<pid>/stat include 'mm->env_start', third number from
the end.

To read the .text area from 0x00400000:
 0x00400000 - (mm->env_start == 140733359794601) = negative_offset

$ ./mem_environ /proc/$(pidof cat)/environ 140733359794601 | hexdump -C -v
00000000  7f 45 4c 46 02 01 01 00  00 00 00 00 00 00 00 00  |.ELF............|
00000010  02 00 3e 00 01 00 00 00  a0 17 40 00 00 00 00 00  |..>.......@.....|
00000020  40 00 00 00 00 00 00 00  40 c5 00 00 00 00 00 00  |@.......@.......|
...

mem_environ is just a program that calculats the negative offset,
open(/proc/<pid>/environ), lseek() and read().

The source is attached, just run this command to test it:
$ ./mem_environ /proc/self/environ 0x0 | hexdump -C -v

In rare cases it will not work, I don't know why.

> >  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);
> > +	if (!ret)
> > +		/* OK to pass negative loff_t, we can catch out-of-range */
> > +		file->f_mode |= FMODE_UNSIGNED_OFFSET;
> > +
> > +	return ret;
> 
> I guess you can set FMODE_UNSIGNED_OFFSET unconditionally, it doesn't
> matter if __mem_open() fails. But I won't insist.
Sure.

> Oleg.
> 
Thanks Oleg. BTW should I resend the patch with a better changelog entry ?

I'll also add another patch to check the offsets inside environ_read().

-- 
tixxdz
http://opendz.org

[-- Attachment #2: mem_environ.c --]
[-- Type: text/x-c, Size: 1765 bytes --]

/*
* /proc/<pid>/environ like /proc/<pid>/mem
* 
* Author: Djalal Harouni   tixxdz  opendz.org
* License: GPLv2
* 
* 
* (mm->env_start + src) will point to your page.
* src is the offset
* For 64bits: A negative offset.
* For 32bits: Did not test, can we wrap ?
*
*/

#define _LARGEFILE64_SOURCE
#define _GNU_SOURCE

#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

#define SYS_lseek       8

extern char **environ;

/* use **environ against a non -fPIC elf */
static inline loff_t get_offset(unsigned long env_addr)
{
	unsigned long load_addr = 0x00400000;
	return (load_addr - env_addr);
}

static loff_t kernel_lseek(int fd, loff_t offset)
{
	return syscall(SYS_lseek, fd, offset, SEEK_SET);
}

static int leak(char *proc_file, unsigned long env_start)
{
	int ret;
	int i, fd;
	char buf[4096];
	loff_t offset = 0;

	memset(buf, 0, sizeof(buf));

	ret = -1;
	fd = open(proc_file, O_RDONLY);
	if (fd == -1) {
		perror("open");
		return ret;
	}

	if (env_start)
		offset = get_offset(env_start);

	if (!offset)
		/* really ? */
		offset = get_offset((unsigned long)*environ);

	if (kernel_lseek(fd, offset) == (off_t) -1) {
		perror("lseek");
		return ret;
	}

	ret = read(fd, buf, sizeof(buf));
	if (ret == -1) {
		perror("read");
		return ret;
	}
	close(fd);

	for (i = 0; i < sizeof(buf); i++)
		printf("%c", buf[i]);
	return 0;
}

int main(int argc, char **argv)
{
	unsigned long env_addr = 0;

	if (argc < 3) {
		printf("%s  /proc/<pid>/environ env_addr\n"
		"    /proc/<pid>/environ.\n"
		"    env_addr: start of environment\n", argv[0]);
		return 0;
	}

	env_addr = (unsigned long) strtoul(argv[2], NULL, 0);
	return leak(argv[1], env_addr);
}

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

[Oleg Nesterov]

Hi Djalal,

On 07/23, Djalal Harouni wrote:
>
> Hi Oleg,
>
> On Sun, Jul 22, 2012 at 10:00:49PM +0200, Oleg Nesterov wrote:
> >
> > Probablt the patch makes sense, but I can't understand the changelog...
> >
> > > Allowing negative offsets on /proc/<pid>/environ can turn it to act like
> > > /proc/<pid>/mem. A negative offset will pass the
> > > fs/read_write.c:lseek_execute() and the environ_read() checks and will
> > > point to another VMA.
> >
> > which VMA?
> It depends on the offset. Please see below.
>
> > environ_read() can only read the memory from [env_start, env_end], and
> > it should check *ppos anyway to ensure it doesn't read something else.
> Yes I agree, but currently that's not the case, there are no checks on *ppos.
                                                  ^^^^^^^^^^^^^^^^^^^
There is, unless I missed something, just it is buggy, no?

> So if you pass a negative offset you will be able to read from an arbitrary
> address.
>
> [...snip...]
>
>   inside environ_read() there is only a one check:
>
>   int this_len = mm->env_end - (mm->env_start + src);
>
>   if (this_len <= 0)
>     break;
>
>
>   Here 'src' is 'src = *ppos' the negative offset converted to unsigned long
>   and (mm->env_start + src) can overflow and point to another VMA.
>
>   int this_len = mm->env_end - (mm->env_start + src)
>
>   'this_len' will be positive and we pass that check.

OK, thanks, but doesn't this mean that this check should be fixed
to avoid the overflow, no matter what *ppos is?

With or without FMODE_UNSIGNED_OFFSET change. And perhaps it is
possible to trigger the overflow even with the positive *ppos,
because:

> I also don't like the truncation of the result to 'int this_len'

Yes.

> BTW should I resend the patch with a better changelog entry ?

Up to you, but I think this makes sense ;)

> I'll also add another patch to check the offsets inside environ_read().

Yes, agreed, but please see above.

Please correct me, but afaics this patch should come 1st and fix the bug.
FMODE_UNSIGNED_OFFSET change can be considered as a cleanup after that.

What do you think?

Oleg.

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

[Djalal Harouni]

On Mon, Jul 23, 2012 at 05:49:27PM +0200, Oleg Nesterov wrote:
> Hi Djalal,
> 
> On 07/23, Djalal Harouni wrote:
> >
> > Hi Oleg,
> >
> > On Sun, Jul 22, 2012 at 10:00:49PM +0200, Oleg Nesterov wrote:
> > >
> > > Probablt the patch makes sense, but I can't understand the changelog...
> > >
> > > > Allowing negative offsets on /proc/<pid>/environ can turn it to act like
> > > > /proc/<pid>/mem. A negative offset will pass the
> > > > fs/read_write.c:lseek_execute() and the environ_read() checks and will
> > > > point to another VMA.
> > >
> > > which VMA?
> > It depends on the offset. Please see below.
> >
> > > environ_read() can only read the memory from [env_start, env_end], and
> > > it should check *ppos anyway to ensure it doesn't read something else.
> > Yes I agree, but currently that's not the case, there are no checks on *ppos.
>                                                   ^^^^^^^^^^^^^^^^^^^
> There is, unless I missed something, just it is buggy, no?
Right.

> > So if you pass a negative offset you will be able to read from an arbitrary
> > address.
> >
> > [...snip...]
> >
> >   inside environ_read() there is only a one check:
> >
> >   int this_len = mm->env_end - (mm->env_start + src);
> >
> >   if (this_len <= 0)
> >     break;
> >
> >
> >   Here 'src' is 'src = *ppos' the negative offset converted to unsigned long
> >   and (mm->env_start + src) can overflow and point to another VMA.
> >
> >   int this_len = mm->env_end - (mm->env_start + src)
> >
> >   'this_len' will be positive and we pass that check.
> 
> OK, thanks, but doesn't this mean that this check should be fixed
> to avoid the overflow, no matter what *ppos is?
Yes, we must also fix it and check if we are in the [env_start, env_end]
range.

> With or without FMODE_UNSIGNED_OFFSET change. And perhaps it is
> possible to trigger the overflow even with the positive *ppos,
> because:
> 
> > I also don't like the truncation of the result to 'int this_len'
> 
> Yes.
> 
> > BTW should I resend the patch with a better changelog entry ?
> 
> Up to you, but I think this makes sense ;)
> 
> > I'll also add another patch to check the offsets inside environ_read().
> 
> Yes, agreed, but please see above.
> 
> Please correct me, but afaics this patch should come 1st and fix the bug.
> FMODE_UNSIGNED_OFFSET change can be considered as a cleanup after that.
> 
> What do you think?
I agree, that makes more sense and we do not hide the *ppos bug.

I'll resend the two patches soon (fix and check *ppos + this patch with an
updated changelog).

Thanks.

-- 
tixxdz
http://opendz.org