New in-kernel privilege API: priv(9)

[Robert Watson]

On Thu, 2 Nov 2006, Bruce Evans wrote:

> I prefer the old code.  The privilege check is the normal access control, 
> and this is unlikely to change since user mounts are so inscure that they 
> are usually turned off by vfs.usermount so attempting them doesn't get this 
> far.

The structure I've been trying to move towards is one in which privilege is 
checked for only when required.  This will, eventually, allow us to audit the 
use of privilege "as exercised".  The old BSD accounting system used to try to 
do this, but it didn't actually work since privilege was rather in excess of 
the minimal requirements, so many things were marked as requiring privilege 
that didn't.  To do this, the structure generally has to start to resemble:

error = my_normal_checks();
if (error) {
 	/*
 	 * Invoke privilege to exempt the thread from the normal policy.
 	 */
 	error = priv_check(td, PRIV_FOO);
 		if (error)
 			return (error);
}

> Clearing error indicators and statistics counters and the like could be 
> under a less generic privilege.

Yes, this is a good idea.  I'll have to check through and see what other 
circumstances in which this happens -- I suspect most stats are protected by 
the sysctl privilege check, so we may need to think about how further to 
refine that.  sysctl is used in rather diverse ways -- for example, during 
bgfsck to manipulate inode reference counts -- so some more work is generally 
required there.

>>>> Index: sys/dev/ofw/ofw_console.c
>>>> ===================================================================
>>>> RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/dev/ofw/ofw_console.c,v
>>>> retrieving revision 1.34
>>>> diff -u -r1.34 ofw_console.c
>>>> --- sys/dev/ofw/ofw_console.c	30 May 2006 07:56:57 -0000	1.34
>>>> +++ sys/dev/ofw/ofw_console.c	30 Oct 2006 17:07:55 -0000
>>>> @@ -140,7 +140,8 @@
>>>>   		ttyconsolemode(tp, 0);
>>>>
>>>>   		setuptimeout = 1;
>>>> -	} else if ((tp->t_state & TS_XCLUDE) && suser(td)) {
>>>> +	} else if ((tp->t_state & TS_XCLUDE) &&
>>>> +	    priv_check(td, PRIV_TTY_EXCLUSIVE)) {
>>>>   		return (EBUSY);
>>>>   	}
>>>> 
>
> There sure are a lot of these.  The conversion to use ttyopen() isn't as 
> complete as I thought.

Yes.

> This was apparently clear to someone named rwatson when he replaced the
> "cred->cr_uid != ip->i_uid && suser_xxx(...)" check by
> VOP_ACCESS(... VADMIN) in ffs in ffs_vnops.c 1.151 (2000/10/19), without
> changing the logical expression, but with adding a large comment :-).

Yes, what a nefarious fellow!

> The bug is that the VADMIN change had not reached many poorly maintained 
> file systems.

Hmm.  I'll investigate this further.

> The style bugs (boolean tests on non-boolean `error' should be fixed more 
> globally.

In general, I've tried to move code to distinguishing ints used as booleans 
and as values, but in this patch I've tried to avoid changing code style where 
possible, since it touches so much code.

>>>> Index: sys/kern/tty.c
>>>> ===================================================================
>>>> RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/kern/tty.c,v
>>>> retrieving revision 1.262
>>>> diff -u -r1.262 tty.c
>>>> --- sys/kern/tty.c	26 Oct 2006 21:42:20 -0000	1.262
>>>> +++ sys/kern/tty.c	30 Oct 2006 17:07:55 -0000
>>>> @@ -1169,9 +1170,9 @@
>>>>   		splx(s);
>>>>   		break;
>>>>   	case TIOCSTI:			/* simulate terminal input */
>>>> -		if ((flag & FREAD) == 0 && suser(td))
>>>> +		if ((flag & FREAD) == 0 && priv_check(td, PRIV_TTY_STI))
>>>>   			return (EPERM);
>
> suser() isn't really boolean, so boolean comparison of it with 0 is a
> style bug.  Here the new and old error value returned is ignored replaced
> by EPERM, which is always (?) the same, but other code in even this file
> users the return value.
>
>>>> -		if (!isctty(p, tp) && suser(td))
>>>> +		if (!isctty(p, tp) && priv_check(td, PRIV_TTY_STI))
>>>>   			return (EACCES);
>
> It's strange that this changes the error to a different one.

Yes.  In general, EPERM is the right error to return for a decision that 
involves being the owner of an object or having superuser privilege.  The only 
real exception to this is in discretionary access control (permissions, etc) 
where the permissions (whatever) on the object deny access, in which case 
EACCES is returned.

>>>> @@ -3340,7 +3342,7 @@
>>>>   	ct = dev->si_drv2;
>>>>   	switch (cmd) {
>>>>   	case TIOCSETA:
>>>> -		error = suser(td);
>>>> +		error = priv_check(td, PRIV_TTY_SETA);
>>>>   		if (error != 0)
>>>>   			return (error);
>>>>   		*ct = *(struct termios *)data;
>
> This isn't really a TTY_SETA privilege, but privilege to change the .init
> and .lock devices.  This should have been controlled by device access
> privilege, but the modes and permissions of these devices have always
> been bogus:
> - for cua*.* they are uucp:dialer 660, but should be more like root:wheel
>  644, with an FWRITE check for the "set" ioctls instead of the priv check.
> - for tty*.*, they are root:wheel 600, but should be more like root:wheel
>  644

Hmm.

> I got bored of even paging through the diffs here :-).

Positively mind-numbing stuff, but something where the details really 
matter...

Robert N M Watson
Computer Laboratory
University of Cambridge