check capabilities in open()

check capabilities in open()

[Vasiliy Kulikov]

Hi,

I've found that some drivers check process capabilities via capable() in
open(), not in ioctl()/write()/etc.

I cannot find answer in POSIX, but IMO process expects that file
descriptors of priviledged user and file descriptors of the same
file/device are the same in priviledge aspect. Driver should deny/allow
open() and deny/allow ioctl() based on user priviledges. The path how
the process gained this fd doesn't matter.

So I think these 2 examples should be equal:

1) root process opened the file and then dropped its priviledges

2) nonroot process opened the file

Currently gained fds are different in priviledge aspect.


If you think these are bugs, I can move capable() checking down to
ioctl()/write()/read()/etc.



This is the full list of such drivers:

drivers/staging/comedi/comedi_fops.c
drivers/oprofile/event_buffer.c
drivers/s390/char/vmcp.c
drivers/s390/char/zcore.c
drivers/net/ppp_generic.c
drivers/scsi/3w-sas.c
drivers/scsi/pmcraid.c
drivers/scsi/megaraid.c
drivers/scsi/megaraid/megaraid_sas.c
drivers/scsi/megaraid/megaraid_mm.c
drivers/char/mem.c
drivers/char/tty_io.c
drivers/char/agp/frontend.c
drivers/char/apm-emulation.c


This is coccinelle script to find that:

@ r1 @
identifier fops;
identifier openx;
@@

struct file_operations fops = {
...
.open       = openx,
...
};


@@
identifier r1.openx;
@@

openx(...)
{
...
*capable(...)
...
}

Re: check capabilities in open()

[Al Viro]

On Sat, Jul 24, 2010 at 08:07:01PM +0400, Vasiliy Kulikov wrote:
> Hi,
> 
> I've found that some drivers check process capabilities via capable() in
> open(), not in ioctl()/write()/etc.
> 
> I cannot find answer in POSIX, but IMO process expects that file
> descriptors of priviledged user and file descriptors of the same
> file/device are the same in priviledge aspect. Driver should deny/allow
> open() and deny/allow ioctl() based on user priviledges. The path how
> the process gained this fd doesn't matter.
> 
> So I think these 2 examples should be equal:
> 
> 1) root process opened the file and then dropped its priviledges
> 
> 2) nonroot process opened the file

They most certainly should _not_.  Consider the following mechanism:
	process A authenticates itself to process B
	B is convinced to open a file that wouldn't be readable for A.
	B passes descriptor to A.
	A reads from it.
You are breaking that.

Re: check capabilities in open()

[Vasiliy Kulikov]

On Sat, Jul 24, 2010 at 19:23 +0100, Al Viro wrote:
> On Sat, Jul 24, 2010 at 08:07:01PM +0400, Vasiliy Kulikov wrote:
> > Hi,
> > 
> > I've found that some drivers check process capabilities via capable() in
> > open(), not in ioctl()/write()/etc.
> > 
> > I cannot find answer in POSIX, but IMO process expects that file
> > descriptors of priviledged user and file descriptors of the same
> > file/device are the same in priviledge aspect. Driver should deny/allow
> > open() and deny/allow ioctl() based on user priviledges. The path how
> > the process gained this fd doesn't matter.
> > 
> > So I think these 2 examples should be equal:
> > 
> > 1) root process opened the file and then dropped its priviledges
> > 
> > 2) nonroot process opened the file
> 
> They most certainly should _not_.  Consider the following mechanism:
> 	process A authenticates itself to process B
> 	B is convinced to open a file that wouldn't be readable for A.
> 	B passes descriptor to A.
> 	A reads from it.
> You are breaking that.

No, I mean that if driver allowed process to open the file, gained fd
should be the same. I say that if process A has _opened_ file, its fd should be the same
that convinced from B.

In your example and current implementation process A allowed to open
file, but it is not the same if B opens file and passes fd to A.


Example from drivers/char/apm-emulation.c:

static int apm_open(struct inode * inode, struct file * filp)
{
    ...
    as->suser = capable(CAP_SYS_ADMIN);
    ...
}

apm_ioctl(struct file *filp, u_int cmd, u_long arg)
{
    ...
	if (!as->suser || !as->writer)
		return -EPERM;
    ...
}

Root can open apm file (as->suser would be true), pass it to
unpriviledged process and it would be able to suspend the system
(as->suser would be still true).

Unpriviledged process can also open apm file (as->suser would be 0), but
would not be able to suspend the system.

Also patalogical case :) unpriviledged process passes fd to root process
and root process cannot suspend the system.




Btw, the list of such drivers is much smaller, some of them just return
-EPERM and open() fails, it is OK. I'll resend more precise list soon.

Re: check capabilities in open()

[Vasiliy Kulikov]

On Sun, Jul 25, 2010 at 09:45 +0400, Vasiliy Kulikov wrote:
> Btw, the list of such drivers is much smaller, some of them just return
> -EPERM and open() fails, it is OK. I'll resend more precise list soon.

The list is tiny:

arch/x86/kernel/apm_32.c
drivers/char/agp/frontend.c
drivers/char/apm-emulation.c

Aslo comment from drivers/cahr/apm-emulation.c:

/*
 * XXX - this is a tiny bit broken, when we consider BSD
 * process accounting.  If the device is opened by root, we
 * instantly flag that we used superuser privs. Who knows,
 * we might close the device immediately without doing a
 * privileged operation -- cevans
 */

Re: check capabilities in open()

[Ted Ts'o]

On Sun, Jul 25, 2010 at 09:45:11AM +0400, Vasiliy Kulikov wrote:
> 
> Root can open apm file (as->suser would be true), pass it to
> unpriviledged process and it would be able to suspend the system
> (as->suser would be still true).

The flip side of this --- and why these devices were deliberately
coded that way, is a setuid root program can open the apm file, and
then drop its root privileges for safety, so that if there is a buffer
overrun in the program, the attacker doesn't get root privileges.
This is quite common in Unix/Linux implementation pattern; let a
setuid program do wha it needs to do as root in terms of opening
specific file descriptors, and then have it drop its privileges.

So the way they are written is quite correct.  And it's consistent
with a device file which is owned by root, mode 600.  A setuid root
program can open the device, but once it is opened, the file
descriptor continues to have access to the program even if its
privileges are dropped, or the file descriptor is passed to another
program.  (This is also sometimes done, deliberately; for example the
original Berkely lpr/lpd program was written where the user would run
lpr, and pass a file descriptor to the lpd daemon, which was not
running as root, but as an unprivileged system process.  This allowed
the lpd daemon to have access to the file, even though it might not be
allowed to open a file that was mode 600.)

The reason why the apm device needed to sample the suser() bit is that
it can be opened by root and non-root processes, but it wanted to
extend the Unix/Linux paradigm that privileges are tested at open()
time.

So this is a not a bug, but quite deliberately, by design.

Regards,

						- Ted

Re: check capabilities in open()

[Vasiliy Kulikov]

On Mon, Jul 26, 2010 at 07:23 -0400, Ted Ts'o wrote:
> The reason why the apm device needed to sample the suser() bit is that
> it can be opened by root and non-root processes, but it wanted to
> extend the Unix/Linux paradigm that privileges are tested at open()
> time.
Yes, it's exactly that I mean, check at open() time and grand high or
less priviledges.

> 
> So this is a not a bug, but quite deliberately, by design.
If it is explicitly designed to check UID at open() time and to have 2
kinds of file descriptors - priviledged and nonpriviledged, I'm fine
with this.

I wanted kernel community to draw attention because this moment was
not obviously for me and I thought it was a design flaw. Now I'm pleased
with your explanation, thank you.