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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120723010447.GA23410@dztty>
Date:	Mon, 23 Jul 2012 02:04:47 +0100
From:	Djalal Harouni <tixxdz@...ndz.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com,
	Al Viro <viro@...iv.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Vasiliy Kulikov <segoon@...nwall.com>,
	WANG Cong <xiyou.wangcong@...il.com>,
	Solar Designer <solar@...nwall.com>,
	Kees Cook <keescook@...omium.org>
Subject: Re: [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

View attachment "mem_environ.c" of type "text/x-c" (1766 bytes)