Hi!
> This patch implements the revoke(2) and frevoke(2) system calls for
> all types of files. The operation is done in passes: first we replace
Do we need revoke()? open()+frevoke() should be fast enough, no?
--
Thanks for all the (sleeping) penguins.
At some point in time, I wrote:
> > This patch implements the revoke(2) and frevoke(2) system calls for
> > all types of files. The operation is done in passes: first we replace
On Sat, 5 Aug 2006, Pavel Machek wrote:
> Do we need revoke()? open()+frevoke() should be fast enough, no?
Not a speed issue, open can have side effects which we might want to
avoid. See comments from Alan and Ulrich in this thread.
Pekka
Pavel Machek wrote:
>
> > This patch implements the revoke(2) and frevoke(2) system calls for
> > all types of files. The operation is done in passes: first we replace
>
>
> Do we need revoke()? open()+frevoke() should be fast enough, no?
Why do we need [f]revoke at all? As it doesn't implement the
BSD semantic I can't see why it's better than fuser -k.
Ciao, ET.
On 8/7/06, Edgar Toernig <[email protected]> wrote:
> Why do we need [f]revoke at all? As it doesn't implement the
> BSD semantic I can't see why it's better than fuser -k.
Which part of the BSD semantics is that?
Pekka Enberg wrote:
>
> On 8/7/06, Edgar Toernig <[email protected]> wrote:
> > Why do we need [f]revoke at all? As it doesn't implement the
> > BSD semantic I can't see why it's better than fuser -k.
>
> Which part of the BSD semantics is that?
That which talks about character devices, in particular ttys.
NetBSD revoke(2):
|
| ... a read() from a character device file which has been revoked
| returns a count of zero (end of file), and a close() call will
| succeed.
|...
| revoke is normally used to prepare a terminal device for a new
| login session, preventing any access by a previous user of the
| terminal.
Irix revoke(2) even mentions:
|
| ERRORS:
| ...
| [EINVAL] The named file is not a character-special file.
It seems, revoke was intended to disable access to tty devices
from old processes in a controlled way. Sounds sane.
Your implementation is much cruder - it simply takes the fd
away from the app; any future use gives EBADF. As a bonus,
it works for regular files and even goes as far as destroying
all mappings of the file from all processes (even root processes).
IMVHO this is a disaster from a security and reliability point
of view.
So, the behaviour regarding ttys is completely different to
other implementations and for other types of fds the Linux
semantic seems unique (the man-pages of the other systems
are pretty silent about that).
A serious question: What do you need this feature of revoking
regular files (or block devices) for? Maybe my imagination
is lacking, but I can't find a use where fuser(1) (or similar
tools) wouldn't be as good or even better than revoke(2).
Ciao, ET.
On Mon, 7 Aug 2006, Edgar Toernig wrote:
>
> Your implementation is much cruder - it simply takes the fd
> away from the app; any future use gives EBADF. As a bonus,
> it works for regular files and even goes as far as destroying
> all mappings of the file from all processes (even root processes).
> IMVHO this is a disaster from a security and reliability point
> of view.
>
I can see the value in these system calls, but I agree that the
implementation is crude. "EBADF" is not something that applications are
taught to expect. Someone correct me if I'm wrong, but I can think of no
situation under which a file descriptor currently gets yanked out from
under your feet -- you should always have to formally abandon it with
close().
This kind of thing only looks proper if it leaves the file descriptor in
place and just returns errors / EOF when you attempt to access it.
Thanks,
Chase
Ar Llu, 2006-08-07 am 17:24 -0500, ysgrifennodd Chase Venters:
> implementation is crude. "EBADF" is not something that applications are
> taught to expect. Someone correct me if I'm wrong, but I can think of no
> situation under which a file descriptor currently gets yanked out from
> under your feet -- you should always have to formally abandon it with
> close().
The file descriptor is not pulled from under you, the access to it is.
This is exactly what occurs today when a tty is hung up. That could be
almost any fd because paths could be symlinks to a pty/tty pair...
In the tty case you get -ENXIO
Alan
Ar Llu, 2006-08-07 am 22:41 +0200, ysgrifennodd Edgar Toernig:
> It seems, revoke was intended to disable access to tty devices
> from old processes in a controlled way. Sounds sane.
Thats the root from which it comes but that alone is insufficient which
is why our vhangup is not enough.
> Your implementation is much cruder - it simply takes the fd
> away from the app; any future use gives EBADF. As a bonus,
It needs to give -ENXIO/0 as per BSD that much is clear.
> it works for regular files and even goes as far as destroying
> all mappings of the file from all processes (even root processes).
> IMVHO this is a disaster from a security and reliability point
> of view.
Actually its no different than if it didn't. The two are identical
behaviours.
To use revoke() I must own the file
If I own the file I can make it a symlink to a pty/tty pair
I can revoke a pty/tty pair
> A serious question: What do you need this feature of revoking
> regular files (or block devices) for? Maybe my imagination
> is lacking, but I can't find a use where fuser(1) (or similar
> tools) wouldn't be as good or even better than revoke(2).
On a typical non-SELinux system with a typical desktop configuration
(SELinux can effectively replace revoke) you need revoke on block
devices in order to guarantee security and on other char devices for
privacy. I'll provide some demonstrations after we have revoke in some
form in the kernel and the problems in question fixed.
There are specific cases where being able to revoke access to one of
your files is useful as well, particularly if you are moving it from
open permissions to private permissions. That one is to be honest much
less interesting and it is easy enough to make our revoke()
implementation return -EINVAL.
The driver only case actually makes it a lot easier because you only
need to set some kind of f_revoked flag on files owned by that device,
truncate the virtual memory mappings and then call the driver method.
The driver would then honour ->f_revoked in its own ioctl/read/write
methods or in the helpers.
Alan
Ar Llu, 2006-08-07 am 22:41 +0200, ysgrifennodd Edgar Toernig:
> > Your implementation is much cruder - it simply takes the fd
> > away from the app; any future use gives EBADF. As a bonus,
On 8/8/06, Alan Cox <[email protected]> wrote:
> It needs to give -ENXIO/0 as per BSD that much is clear.
I assume you mean devices only? EBADF makes sense for regular files,
except for close(2), maybe, for which zero is probably more
appropriate.
Pekka
Hi!
> > it works for regular files and even goes as far as destroying
> > all mappings of the file from all processes (even root processes).
> > IMVHO this is a disaster from a security and reliability point
> > of view.
>
> Actually its no different than if it didn't. The two are identical
> behaviours.
>
> To use revoke() I must own the file
> If I own the file I can make it a symlink to a pty/tty pair
> I can revoke a pty/tty pair
How can you symlink opened file?
--
Thanks for all the (sleeping) penguins.
Ar Maw, 2006-08-08 am 12:57 +0000, ysgrifennodd Pavel Machek:
> > To use revoke() I must own the file
> > If I own the file I can make it a symlink to a pty/tty pair
> > I can revoke a pty/tty pair
>
> How can you symlink opened file?
I make a symlink before running the app which opens it. Or if the app
doesn't open it I pass the file handle of a pty/tty pair to it.
Alan
Hi!
> > > To use revoke() I must own the file
> > > If I own the file I can make it a symlink to a pty/tty pair
> > > I can revoke a pty/tty pair
> >
> > How can you symlink opened file?
>
> I make a symlink before running the app which opens it. Or if the app
> doesn't open it I pass the file handle of a pty/tty pair to it.
Okay, I guess marginal app could do open, then fstat to make sure it
is not pty/tty, then proceed assuming it can't go away. But I see that
chances of _that_ happening are slim.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Alan Cox wrote:
>
> Ar Llu, 2006-08-07 am 22:41 +0200, ysgrifennodd Edgar Toernig:
> >
> > Your implementation is much cruder - it simply takes the fd
> > away from the app; any future use gives EBADF. As a bonus,
>
> It needs to give -ENXIO/0 as per BSD that much is clear.
Ah, OK. And not to forget select/poll. (What about SIGHUP?)
I'm not sure though, whether it's really necessary to allow the
owner of a file to revoke fds - I would feel better if only root
(or someone with the right caps) could revoke fds/mappings.
> To use revoke() I must own the file
> If I own the file I can make it a symlink to a pty/tty pair
> I can revoke a pty/tty pair
With the EIO/EOF behaviour that's not a problem - apps that deal
with ttys have to expect that condition.
> > A serious question: What do you need this feature of revoking
> > regular files (or block devices) for? Maybe my imagination
> > is lacking, but I can't find a use where fuser(1) (or similar
> > tools) wouldn't be as good or even better than revoke(2).
>
> On a typical non-SELinux system with a typical desktop configuration
> (SELinux can effectively replace revoke) you need revoke on block
> devices in order to guarantee security
Hmm... which apps have an open fd on block devices? Usually a
filesystem is mounted on the device and then there are no fds
to the block-dev involved. Or do you expect the "fuser -m"
behaviour from revoke? Afaics, that's not the case at the moment.
Which users have perms to access a block-dev anyway?
> There are specific cases where being able to revoke access to one of
> your files is useful as well, particularly if you are moving it from
> open permissions to private permissions. That one is to be honest much
> less interesting and it is easy enough to make our revoke()
> implementation return -EINVAL.
Hmm... then use fuser and kill the process instead of silently taking
away fds and mappings.
My summary: revoke on chars devs with EIO/EOF behaviour is ok.
revoke on blocks devs is questionable
revoke on regular files is wrong.
Ciao, ET.
Alan Cox wrote:
>
> Ar Llu, 2006-08-07 am 17:24 -0500, ysgrifennodd Chase Venters:
> > implementation is crude. "EBADF" is not something that applications are
> > taught to expect. Someone correct me if I'm wrong, but I can think of no
> > situation under which a file descriptor currently gets yanked out from
> > under your feet -- you should always have to formally abandon it with
> > close().
>
> The file descriptor is not pulled from under you, the access to it is.
> This is exactly what occurs today when a tty is hung up.
If I read the code correctly, the behaviour for hung up ttys is completely
different: read returns EOF, write returns EIO, select/poll/epoll return
ready, close works. As rather boring but totally sane behaviour for an fd.
But after revoke you get EBADF for any operation, even select or close.
The fd becomes nearly indistinguishable from a really closed fd (the only
difference is that the fd-number won't be reused (potentional DoS)).
And IMHO that's insane that a regular user may close fds in someone else's
processes (or munmap some of its memory). I already see people trying
to exploit bugs in system services:
for (;;) revoke("index.html");
for (;;) revoke("some_print_job");
for (;;) revoke("some_mail");
Ciao, ET.
Ar Mer, 2006-08-09 am 10:41 +0200, ysgrifennodd Edgar Toernig:
> If I read the code correctly, the behaviour for hung up ttys is completely
> different: read returns EOF, write returns EIO, select/poll/epoll return
> ready, close works. As rather boring but totally sane behaviour for an fd.
>
> But after revoke you get EBADF for any operation, even select or close.
Thats a detail of the proposed implementation that isn't hard to fix.
> And IMHO that's insane that a regular user may close fds in someone else's
> processes (or munmap some of its memory). I already see people trying
> to exploit bugs in system services:
I can do this already today. In fact the index.html one can be used to
crash certain products now depending on their configuration. Just do
while { true } do
cp some.html index.html
> index.html
done
with a shell that truncates on > and you'll be able to bus error them if
they mmap and are not well written.
These are not actually changes in behaviour. At any point I can shrink a
file I own and you get read -> 0, mmap access -> bus error. Write
behaviour is new but thats no different to filling the disk up or other
real write errors.
Ar Mer, 2006-08-09 am 10:41 +0200, ysgrifennodd Edgar Toernig:
> > If I own the file I can make it a symlink to a pty/tty pair
> > I can revoke a pty/tty pair
>
> With the EIO/EOF behaviour that's not a problem - apps that deal
> with ttys have to expect that condition.
Think about it a moment - I can symlink any file to a tty/pty pair so
any file I own you open might be a tty.
> Hmm... which apps have an open fd on block devices? Usually a
cdrecord, cd audio players, eject, ....
Alan Cox wrote:
>
> Ar Mer, 2006-08-09 am 10:41 +0200, ysgrifennodd Edgar Toernig:
> > > If I own the file I can make it a symlink to a pty/tty pair
> > > I can revoke a pty/tty pair
> >
> > With the EIO/EOF behaviour that's not a problem - apps that deal
> > with ttys have to expect that condition.
>
> Think about it a moment - I can symlink any file to a tty/pty pair so
> any file I own you open might be a tty.
Yes, OK. The EIO/EOF behaviour is fine. Even for regular files it's
not something extraordinary.
> > Hmm... which apps have an open fd on block devices? Usually a
>
> cdrecord, cd audio players, eject, ....
And killing them is not OK? "fuser -km /dev/cdrom" already covers both
cases, mounted somewhere and opened for special access.
Sorry if I sound a little bit anal. IMO, a generic revoke is a pretty
sharp sword which is given to ordinary users and I have a very uneasy
feeling. They can dig in the innards of other people's processes - a
clean headshot by root is something different ...
Ciao, ET.
Alan Cox wrote:
>
> Ar Mer, 2006-08-09 am 10:41 +0200, ysgrifennodd Edgar Toernig:
> >
> > But after revoke you get EBADF for any operation, even select or close.
>
> Thats a detail of the proposed implementation that isn't hard to fix.
Fine.
> > And IMHO that's insane that a regular user may close fds in someone else's
> > processes (or munmap some of its memory). I already see people trying
> > to exploit bugs in system services:
>
> I can do this already today. In fact the index.html one can be used to
> crash certain products now depending on their configuration. Just do
>
> while { true } do
> cp some.html index.html
> > index.html
> done
>
> with a shell that truncates on > and you'll be able to bus error them if
> they mmap and are not well written.
I wasn't aware of that (and I would definitely prefer a different behaviour).
But anyway, correct me if I'm wrong, revoke (V2) not simply removes the
pages from the mmaped area as truncating does (the vma stays); revoke
seems to completely remove the vma which is clearly a security bug.
Future mappings may silently get mapped into the area of the revoked
file without the app noticing it. It may then hand out data of the new
file still thinking it's sending the old one.
Ciao, ET.
Ar Mer, 2006-08-09 am 20:00 +0200, ysgrifennodd Edgar Toernig:
> And killing them is not OK? "fuser -km /dev/cdrom" already covers both
> cases, mounted somewhere and opened for special access.
fuser is quite easy to race, it doesn't handle all sorts of corner cases
like namespaces either. Its a crude blunt instrument that sometimes
works and is very slow.
> Sorry if I sound a little bit anal. IMO, a generic revoke is a pretty
> sharp sword which is given to ordinary users and I have a very uneasy
> feeling. They can dig in the innards of other people's processes - a
> clean headshot by root is something different ...
I can see your concern about arbitary files, but I'm not sure it holds
simply because the tricks already exist via other methods.
Ar Mer, 2006-08-09 am 20:00 +0200, ysgrifennodd Edgar Toernig:
> But anyway, correct me if I'm wrong, revoke (V2) not simply removes the
> pages from the mmaped area as truncating does (the vma stays); revoke
> seems to completely remove the vma which is clearly a security bug.
> Future mappings may silently get mapped into the area of the revoked
> file without the app noticing it. It may then hand out data of the new
> file still thinking it's sending the old one.
I agree with that point 100%.
On 8/9/06, Alan Cox <[email protected]> wrote:
> I can see your concern about arbitary files, but I'm not sure it holds
> simply because the tricks already exist via other methods.
Agreed. We should support close(2) and munmap(2), though.
Ar Mer, 2006-08-09 am 20:00 +0200, ysgrifennodd Edgar Toernig:
> > But anyway, correct me if I'm wrong, revoke (V2) not simply removes the
> > pages from the mmaped area as truncating does (the vma stays); revoke
> > seems to completely remove the vma which is clearly a security bug.
> > Future mappings may silently get mapped into the area of the revoked
> > file without the app noticing it. It may then hand out data of the new
> > file still thinking it's sending the old one.
On 8/9/06, Alan Cox <[email protected]> wrote:
> I agree with that point 100%.
Agreed also. I already had a version that simply replaced the ->nopage
method of vma_ops but took what forced unmount patch had to get proper
->close dealings. But I completely agree that sane revocation should
allow close(2) and munmap(2) on the revoked fd and shared mapping.
I'll put them on my todo and in the meanwhile, you can find the latest
patches here: http://www.kernel.org/pub/linux/kernel/people/penberg/patches/revoke/
Thanks for taking the time to review the patch!
Pekka
Pekka Enberg wrote:
>
> I'll put them on my todo and in the meanwhile, you can find the latest
> patches here:
> http://www.kernel.org/pub/linux/kernel/people/penberg/patches/revoke/
>
> Thanks for taking the time to review the patch!
+ retry:
+ if (signal_pending(current)) {
+ err = -ERESTARTSYS;
+ goto out;
+ }
+
+ to_close = alloc_revoke_table(inode, to_exclude, &max_fds);
+ if (!to_close) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ read_lock(&tasklist_lock);
+
+ /*
+ * If someone forked while we were allocating memory, try again.
+ */
+ if (inode_fds(inode, to_exclude) > max_fds) {
+ read_unlock(&tasklist_lock);
+ goto retry;
+ }
It seems, the retry is leaking the to_close table.
Ciao, ET.
Pekka Enberg wrote:
>
> I'll put them on my todo and in the meanwhile, you can find the latest
> patches here:
> http://www.kernel.org/pub/linux/kernel/people/penberg/patches/revoke/
>
> Thanks for taking the time to review the patch!
+ err = close_files(this);
+
+ put_task_struct(this->owner);
+ if (err)
+ break;
+ }
+ if (err)
+ restore_files(&to_cleanup[i], nr_fds-i);
I think, the error path is wrong as it tries to restore "this"
which means the now invalid filp (close always closes, even in
case of errors) is put back into the fd-table; and, the task
struct is put twice. I think, you should ignore errors on close.
(But I guess, this part of revoke gets rewritten anyway to match
BSD behaviour.) I wonder, if revoke should really abort when
there's an error from one fd or better continue and try its best.
restore_files:
+ spin_lock(&files->file_lock);
+ fdt = files_fdtable(files);
+ rcu_assign_pointer(fdt->fd[this->fd], this->file);
+ FD_SET(this->fd, fdt->close_on_exec);
+ spin_unlock(&files->file_lock);
+ put_files_struct(files);
+ }
+
+ put_task_struct(this->owner);
This sets close_on_exec unconditionally, even if it wasn't set
before. Hm..., if a cloned thread is able to exec, it seems a
little bit dangerous to restore the fd-table with filps that
were valid some time ago - the fd-table may have changed in the
meantime... But maybe I simply missed something...
Ciao, ET.
Edgar Toernig wrote:
[...]
> I wasn't aware of that (and I would definitely prefer a different behaviour).
>
> But anyway, correct me if I'm wrong, revoke (V2) not simply removes the
> pages from the mmaped area as truncating does (the vma stays); revoke
> seems to completely remove the vma which is clearly a security bug.
> Future mappings may silently get mapped into the area of the revoked
> file without the app noticing it. It may then hand out data of the new
> file still thinking it's sending the old one.
>
One could remap to /dev/null - the file would then be free to be
umounted, but the app could get confused. Or map inaccessible
pages, so the app segfault on the next access.
Helge Hafting