mprotect abuse in slim

mprotect abuse in slim

[Christoph Hellwig]

Folks, WTF were you smoking when writing the slim code?  Calling mprotect
from random code to rewoke write permissions on process is not okay.  As
is poking into the dile desriptor tables.  As is ding permission checks
based on d_path output.

Andrew, could you please just drop slim?  The code isn't any better than
the last few times they submitted this junk, and it still doesn't serve
any real-life purpose.

Re: mprotect abuse in slim

[Mimi Zohar]

SLIM implements dynamic process labels, so when a process
is demoted, we must be able to revoke write access to some
resources to which it has previously valid handles.
For example, if a shell reads an untrusted file, the
shell is demoted, and write access to more trusted files
revoked. Based on previous comments on lkml, we understand
that this is not really possible in general, so SLIM only
attempts to revoke access in certain simple cases.

Starting with the fdtable, would it help if we move the 
fdtable tweaking out of slim itself and into helpers?  Or
can you recommend another way to implement this functionality.

Mimi Zohar

Re: mprotect abuse in slim

[Arjan van de Ven]

> Starting with the fdtable, would it help if we move the 
> fdtable tweaking out of slim itself and into helpers?  Or
> can you recommend another way to implement this functionality.

Hi,

maybe this is a silly question, but do you revoke not only the current
fd entries, but also the ones that are pending in UNIX domain sockets
and that are already being sent to the process? If not.. then you might
as well not bother ;)

Greetings,
   Arjan van de Ven
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: mprotect abuse in slim

[Christoph Hellwig]

On Mon, Jan 08, 2007 at 07:07:25PM -0800, Arjan van de Ven wrote:
> 
> > Starting with the fdtable, would it help if we move the 
> > fdtable tweaking out of slim itself and into helpers?  Or
> > can you recommend another way to implement this functionality.
> 
> Hi,
> 
> maybe this is a silly question, but do you revoke not only the current
> fd entries, but also the ones that are pending in UNIX domain sockets
> and that are already being sent to the process? If not.. then you might
> as well not bother ;)

Exactly.  What these folks want is revoke (maybe more fine grained, but
that's not the point).  And guess what folks, revoke is not trivial,
otherwise we'd have it.  If you want to volunteer to implement a full-blown
revoke that's fine, but

  a) it belongs into core code
  b) needs to be done right

Re: mprotect abuse in slim

[Chris Wright]

* Christoph Hellwig (hch@infradead.org) wrote:
> On Mon, Jan 08, 2007 at 07:07:25PM -0800, Arjan van de Ven wrote:
> > maybe this is a silly question, but do you revoke not only the current
> > fd entries, but also the ones that are pending in UNIX domain sockets
> > and that are already being sent to the process? If not.. then you might
> > as well not bother ;)
> 
> Exactly.  What these folks want is revoke (maybe more fine grained, but
> that's not the point).  And guess what folks, revoke is not trivial,
> otherwise we'd have it.  If you want to volunteer to implement a full-blown
> revoke that's fine, but
> 
>   a) it belongs into core code
>   b) needs to be done right

Very much agreed.  There's way too many holes in half-way done revocation.

thanks,
-chris

Re: mprotect abuse in slim

[Serge E. Hallyn]

Quoting Christoph Hellwig (hch@infradead.org):
> On Mon, Jan 08, 2007 at 07:07:25PM -0800, Arjan van de Ven wrote:
> >
> > > Starting with the fdtable, would it help if we move the
> > > fdtable tweaking out of slim itself and into helpers?  Or
> > > can you recommend another way to implement this functionality.
> >
> > Hi,
> >
> > maybe this is a silly question, but do you revoke not only the current
> > fd entries, but also the ones that are pending in UNIX domain sockets
> > and that are already being sent to the process? If not.. then you might
> > as well not bother ;)
> 
> Exactly.  What these folks want is revoke (maybe more fine grained, but
> that's not the point).  And guess what folks, revoke is not trivial,
> otherwise we'd have it.  If you want to volunteer to implement a full-blown
> revoke that's fine, but
> 
>   a) it belongs into core code
>   b) needs to be done right

Whatever happened with Pekka's revoke submissions?  Did you lose
interest after
http://www.kernel.org/pub/linux/kernel/people/penberg/patches/revoke/2.6.19-rc1/revoke-2.6.19-rc1,
or was it decided that the approach was unworkable?

Now, what slim needs isn't "revoke all files for this inode",
but "revoke this task's write access to this fd".  So two functions
which could be useful are

	int fd_revoke_write(struct task_struct *tsk, int fd)
	int fd_revoke_write_iter(struct task_struct *tsk,
			(int *)need_revoke(struct task_struct *tsk, int fd))

Anyway I'll get going on rebasing Pekka's latest patch (pending
his reply) and providing the above two functions on top of that.

-serge

Re: mprotect abuse in slim

[Pekka J Enberg]

On Tue, 9 Jan 2007, Serge E. Hallyn wrote:
> Whatever happened with Pekka's revoke submissions?  Did you lose
> interest after
> http://www.kernel.org/pub/linux/kernel/people/penberg/patches/revoke/2.6.19-rc1/revoke-2.6.19-rc1,
> or was it decided that the approach was unworkable?

Lack of time. Also, I would love to hear comments on the way I am doing 
revoke on shared mappings. There are few open issues remaining, mainly, 
supporting munmap(2) for revoked mappings.

			Pekka

Re: mprotect abuse in slim

[Serge E. Hallyn]

Quoting Pekka J Enberg (penberg@cs.helsinki.fi):
> On Tue, 9 Jan 2007, Serge E. Hallyn wrote:
> > Whatever happened with Pekka's revoke submissions?  Did you lose
> > interest after
> > http://www.kernel.org/pub/linux/kernel/people/penberg/patches/revoke/2.6.19-rc1/revoke-2.6.19-rc1,
> > or was it decided that the approach was unworkable?
> 
> Lack of time.

Ok great - then it's not dead  :)

> Also, I would love to hear comments on the way I am doing
> revoke on shared mappings. There are few open issues remaining, mainly,
> supporting munmap(2) for revoked mappings.

Hmm, I wanted to test your revoke-munmap.c to see what you get right now
with munmap, but a quick port of your patch to yesterdays -git on s390
gives me an oops on do_revoke.  I'll have to straighten that out when I
get a chance.

But since it looks like you just munmap the region now, shouldn't a
subsequent munmap by the app just return -EINVAL?  that seems appropriate
to me.

thanks,
-serge

Re: mprotect abuse in slim

[Pekka Enberg]

On 1/10/07, Serge E. Hallyn <serue@us.ibm.com> wrote:
> But since it looks like you just munmap the region now, shouldn't a
> subsequent munmap by the app just return -EINVAL?  that seems appropriate
> to me.

Applications don't know about revoke and neither should they.
Therefore close(2) and munmap(2) must work the same way they would for
non-revoked inodes so that applications can release resources
properly.

                                         Pekka

Re: mprotect abuse in slim

[Serge E. Hallyn]

Quoting Pekka Enberg (penberg@cs.helsinki.fi):
> On 1/10/07, Serge E. Hallyn <serue@us.ibm.com> wrote:
> >But since it looks like you just munmap the region now, shouldn't a
> >subsequent munmap by the app just return -EINVAL?  that seems appropriate
> >to me.
> 
> Applications don't know about revoke and neither should they.
> Therefore close(2) and munmap(2) must work the same way they would for
> non-revoked inodes so that applications can release resources
> properly.
> 
>                                         Pekka

Right, but is returning -EINVAL to userspace on munmap a problem?
It may not have been expected before, but it shouldn't break
anything...

Thanks for the tw other patches - I'll give them a shot and check
out current munmap behavior just as soon as I get a chance.

thanks,
-serge

Re: mprotect abuse in slim

[Pekka Enberg]

On 1/11/07, Serge E. Hallyn <serue@us.ibm.com> wrote:
> Right, but is returning -EINVAL to userspace on munmap a problem?

Yes, because an application has no way of reusing the revoked mapping
range. The current patch should get this right, though.

On 1/11/07, Serge E. Hallyn <serue@us.ibm.com> wrote:
> Thanks for the tw other patches - I'll give them a shot and check
> out current munmap behavior just as soon as I get a chance.

I hacked the remaining open issues yesterday so please use this instead:

http://www.cs.helsinki.fi/u/penberg/linux/revoke/revoke-2.6.20-rc4

The one at kernel.org will be updated as well when mirroring catches up.

Re: mprotect abuse in slim

[Serge E. Hallyn]

Quoting Pekka Enberg (penberg@cs.helsinki.fi):
> On 1/11/07, Serge E. Hallyn <serue@us.ibm.com> wrote:
> >Right, but is returning -EINVAL to userspace on munmap a problem?
> 
> Yes, because an application has no way of reusing the revoked mapping
> range. The current patch should get this right, though.

Looks good.

> On 1/11/07, Serge E. Hallyn <serue@us.ibm.com> wrote:
> >Thanks for the tw other patches - I'll give them a shot and check
> >out current munmap behavior just as soon as I get a chance.
> 
> I hacked the remaining open issues yesterday so please use this instead:
> 
> http://www.cs.helsinki.fi/u/penberg/linux/revoke/revoke-2.6.20-rc4
> 
> The one at kernel.org will be updated as well when mirroring catches up.

Great, this patch (with the attached trivial patch to implement the
syscalls on my z guest) passes all of your testcases.

I'll give it a another, closer read over the weekend, and start
basing something to help slim on this code.

-serge


Subject: [PATCH] revoke: s390 architecture

Implement the revoke syscalls for s390.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 arch/s390/kernel/compat_wrapper.S |   11 +++++++++++
 arch/s390/kernel/syscalls.S       |    2 ++
 include/asm-s390/unistd.h         |    4 +++-
 3 files changed, 16 insertions(+), 1 deletions(-)

24ebe37142572d388569851edf4b919b7f97cc2f
diff --git a/arch/s390/kernel/compat_wrapper.S b/arch/s390/kernel/compat_wrapper.S
index 71e54ef..b5c2bfa 100644
--- a/arch/s390/kernel/compat_wrapper.S
+++ b/arch/s390/kernel/compat_wrapper.S
@@ -1665,3 +1665,14 @@ sys_getcpu_wrapper:
 	llgtr	%r3,%r3			# unsigned *
 	llgtr	%r4,%r4			# struct getcpu_cache *
 	jg	sys_getcpu
+
+	.globl sys_revokeat_wrapper
+sys_revokeat_wrapper:
+	lgfr	%r2,%r2			# int
+	llgtr	%r3,%r3			# const char *
+	jg	sys_revokeat
+
+	.globl sys_frevoke_wrapper
+sys_frevoke_wrapper:
+	llgfr	%r2,%r2			# unsigned int
+	jg	sys_frevoke
diff --git a/arch/s390/kernel/syscalls.S b/arch/s390/kernel/syscalls.S
index a4ceae3..85a6673 100644
--- a/arch/s390/kernel/syscalls.S
+++ b/arch/s390/kernel/syscalls.S
@@ -321,3 +321,5 @@ SYSCALL(sys_vmsplice,sys_vmsplice,compat
 NI_SYSCALL							/* 310 sys_move_pages */
 SYSCALL(sys_getcpu,sys_getcpu,sys_getcpu_wrapper)
 SYSCALL(sys_epoll_pwait,sys_epoll_pwait,sys_ni_syscall)
+SYSCALL(sys_revokeat,sys_revokeat,sys_revokeat_wrapper)
+SYSCALL(sys_frevoke,sys_frevoke,sys_frevoke_wrapper)
diff --git a/include/asm-s390/unistd.h b/include/asm-s390/unistd.h
index fb6fef9..6651cb1 100644
--- a/include/asm-s390/unistd.h
+++ b/include/asm-s390/unistd.h
@@ -250,8 +250,10 @@
 /* Number 310 is reserved for new sys_move_pages */
 #define __NR_getcpu		311
 #define __NR_epoll_pwait	312
+#define __NR_revokeat		313
+#define __NR_frevoke		314
 
-#define NR_syscalls 313
+#define NR_syscalls 315
 
 /* 
  * There are some system calls that are not present on 64 bit, some
-- 
1.1.6

Re: mprotect abuse in slim

[Pekka Enberg]

On 1/10/07, Serge E. Hallyn <serue@us.ibm.com> wrote:
> Now, what slim needs isn't "revoke all files for this inode",
> but "revoke this task's write access to this fd".  So two functions
> which could be useful are
>
>         int fd_revoke_write(struct task_struct *tsk, int fd)
>         int fd_revoke_write_iter(struct task_struct *tsk,
>                         (int *)need_revoke(struct task_struct *tsk, int fd))

This gets interesting. We probably need revokefs (which we use
internally as a substitute for revoke inodes) to be stacked on top of
the actual fs so that you can still read from the fd. But most of the
revocation is still the same, we need to watch out for fork(2) and
dup(2) and take down shared mappings.

Re: mprotect abuse in slim

[Serge E. Hallyn]

Quoting Pekka Enberg (penberg@cs.helsinki.fi):
> On 1/10/07, Serge E. Hallyn <serue@us.ibm.com> wrote:
> >Now, what slim needs isn't "revoke all files for this inode",
> >but "revoke this task's write access to this fd".  So two functions
> >which could be useful are
> >
> >        int fd_revoke_write(struct task_struct *tsk, int fd)
> >        int fd_revoke_write_iter(struct task_struct *tsk,
> >                        (int *)need_revoke(struct task_struct *tsk, int 
> >                        fd))
> 
> This gets interesting. We probably need revokefs (which we use
> internally as a substitute for revoke inodes) to be stacked on top of
> the actual fs so that you can still read from the fd. But most of the
> revocation is still the same, we need to watch out for fork(2) and
> dup(2) and take down shared mappings.

Hmm, would revokefs need to be explicitly stacked on top of the fs,
or could we just swap out fdt[fd] for the revokefs file, and have
the revokefs file's private data point to the original inode, with
it's write function returning an error, and read being passed through?

Do you (or hch) then have a problem with these functions (sitting either
in fs/revoke.c or fs/file_table.c) calling mprotect to remove the write
permission from the mmap'ed segment?  i.e. was the main objection to
mprotect being called from "just anywhere"?

-serge

Re: mprotect abuse in slim

[Pekka J Enberg]

On Fri, 12 Jan 2007, Serge E. Hallyn wrote:
> Hmm, would revokefs need to be explicitly stacked on top of the fs,
> or could we just swap out fdt[fd] for the revokefs file, and have
> the revokefs file's private data point to the original inode, with
> it's write function returning an error, and read being passed through?

Swapping the fds should be sufficient.

On Fri, 12 Jan 2007, Serge E. Hallyn wrote:
> Do you (or hch) then have a problem with these functions (sitting either
> in fs/revoke.c or fs/file_table.c) calling mprotect to remove the write
> permission from the mmap'ed segment?  i.e. was the main objection to
> mprotect being called from "just anywhere"?

I haven't seen the original patches so I don't knw what hch objected to. 
It is not enough that we do mprotect, tough. We also must make sure the 
task can't do mprotect on its own nor remap the memory region.

Re: mprotect abuse in slim

[Mimi Zohar]

Arjan van de Ven <arjan@infradead.org> wrote on 01/08/2007 10:07:25 PM:

> Hi,
> 
>maybe this is a silly question, but do you revoke not only the current
>fd entries, but also the ones that are pending in UNIX domain sockets
>and that are already being sent to the process? If not.. then you might
>as well not bother ;)
> 
>Greetings,
>   Arjan van de Ven

In the new slim code, which we haven't sent out but will soon, we added
the unix_may_send and unix_stream_connect hooks. The unix_may_send hook
prevents a demoted process from sending data on a higher integrity
socket; and the unix_may_connect hook prevents a process from
connecting to a lower integrity socket.  The integrity level of the
socket is based on the integrity of the file associated with the Unix
domain socket.

The bottom line is that although we don't demote the pending socket
and would allow it to connect at the higher integrity, we simply don't
allow anything of lesser integrity to be sent over it.

Mimi Zohar

Re: mprotect abuse in slim

[Valdis.Kletnieks]

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

On Mon, 08 Jan 2007 17:38:25 EST, Mimi Zohar said:

> revoked. Based on previous comments on lkml, we understand
> that this is not really possible in general, so SLIM only
> attempts to revoke access in certain simple cases.

Which, unfortunately, creates incredibly brittle code when some attacker
reads the SLIM source code and finds a way to force the non-simple case
you ignore.

This is an area where you really need to do it *right*, or not at all.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: mprotect abuse in slim

[Mimi Zohar]

Valdis.Kletnieks@vt.edu wrote on 01/09/2007 02:27:19 PM:

>Which, unfortunately, creates incredibly brittle code when some attacker
>reads the SLIM source code and finds a way to force the non-simple case
>you ignore.
> 
>This is an area where you really need to do it *right*, or not at all.

For the non-simple case, it isn't that we 'ignore' the revocation,
but we prevent the cause for demotion to occur.  The current status is
that for those cases we can revoke, we demote the process and do the
revocation, and for those cases which we can't revoke, we prevent the 
process from being demoted.

Mimi Zohar

Re: mprotect abuse in slim

[Pavel Machek]

Hi!

> SLIM implements dynamic process labels, so when a process
> is demoted, we must be able to revoke write access to some
> resources to which it has previously valid handles.
> For example, if a shell reads an untrusted file, the
> shell is demoted, and write access to more trusted files
> revoked. Based on previous comments on lkml, we understand
> that this is not really possible in general, so SLIM only
> attempts to revoke access in certain simple cases.

Are you saying that SLIM is useless by design because interested
parties can work around it?
							Pavel
-- 
Thanks for all the (sleeping) penguins.

Re: mprotect abuse in slim

[Mimi Zohar]

Pavel Machek <pavel@suse.cz> wrote on 01/11/2007 09:35:37 AM:

> Hi!
> 
> > SLIM implements dynamic process labels, so when a process
> > is demoted, we must be able to revoke write access to some
> > resources to which it has previously valid handles.
> > For example, if a shell reads an untrusted file, the
> > shell is demoted, and write access to more trusted files
> > revoked. Based on previous comments on lkml, we understand
> > that this is not really possible in general, so SLIM only
> > attempts to revoke access in certain simple cases.
> 
> Are you saying that SLIM is useless by design because interested
> parties can work around it?
>                      Pavel

Sorry that we were unclear about what happens in the case revocation
is not possible.  In those cases, the unsafe requested read or exec
that would normally trigger the demotion/revocation is denied, so
there is no way around the integrity model.

The goal of the low water mark integrity model is to be as transparent
as possible to the user. If the user asks for something to be done, we
allow it as much as possible, demoting the process as needed for
security.  If there is something that would need to be revoked, which
can't safely be revoked, then we deny the read/exec request, which is
secure, but possibly visible/annoying to the user. Fortunately in our
testing, there are only a few cases where this happens, and the
overall result is a model which is still much more transparent than
other models which don't allow demotion at all.

Mimi Zohar