Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20120122202159.GA16281@openwall.com>
Date: Mon, 23 Jan 2012 00:21:59 +0400
From: Solar Designer <solar@...nwall.com>
To: oss-security@...ts.openwall.com
Subject: Re: CVE request: kernel: proc: clean up and fix /proc/<pid>/mem handling

On Sun, Jan 22, 2012 at 09:52:27PM +0400, Solar Designer wrote:
> On Wed, Jan 18, 2012 at 10:25:55AM +0800, Eugene Teo wrote:
> > This changes it to do the permission checks at open time, and instead of
> > tracking the process, it tracks the VM at the time of the open.  That
> > simplifies the code a lot, but does mean that if you hold the file
> > descriptor open over an execve(), you'll continue to read from the _old_ VM.
> 
> I see it in the revised code, but I don't get it.  What does "the old
> VM" mean after an execve()?  The code stores the mm pointer in
> file->private_data, but is this stored pointer even valid after an
> execve()?  (The code blindly assumes so, only checking for non-NULL.)

OK, here's my current understanding: the old VM is preserved
(refcounted) precisely because someone holds /proc/<pid>/mem open.
The new mem_open() calls mm_access(), which calls get_task_mm().
So at the time of mem_read() / mem_write() the pointer is valid even if
the process passed an execve().

However, I think this opens up a new security problem (albeit a
relatively minor one): RLIMIT_NPROC * RLIMIT_AS bypass.  Previously, a
user with RLIMIT_NPROC set was sort of limited to consuming this much
memory (plus shm and plus various in-kernel data structures related to
the user's processes).  Now the user's memory consumption via processes'
address space is not limited by RLIMIT_NPROC anymore.

Am I missing something?  If not, I think we need to patch that.  Maybe
have RLIMIT_NPROC apply even to such "zombie VMs" (confusing and
tricky).  Maybe re-consider the entire approach to fixing the original
issue addressed with commit e268337dfe26dfc7efd422a804dbb27977a3cccc.
That is, revert this commit and fix the issue differently (likely by
adding full privilege checks at time of read and write - in addition to
re-introducing the self_exec_id checks, which are also needed even along
with full checks of the caller's privileges).

Alexander

Powered by blists - more mailing lists

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.