Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 2 Nov 2006 00:09:10 +1100 (EST)
From:      Bruce Evans <bde@zeta.org.au>
To:        Robert Watson <rwatson@freebsd.org>
Cc:        arch@freebsd.org, Pawel Jakub Dawidek <pjd@freebsd.org>
Subject:   Re: New in-kernel privilege API: priv(9)
Message-ID:  <20061101231230.B31271@delplex.bde.org>
In-Reply-To: <20061031144237.B87421@fledge.watson.org>
References:  <20061031092122.D96078@fledge.watson.org> <20061031144050.GC13359@garage.freebsd.pl> <20061031144237.B87421@fledge.watson.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 31 Oct 2006, Robert Watson wrote:

> On Tue, 31 Oct 2006, Pawel Jakub Dawidek wrote:
>
>> Here are few nits I found:
>
> Thanks!  Comments below.
>
>>> Index: sys/fs/hpfs/hpfs_vnops.c

It (all of `>>>') is too much inteface bloat for me to like.

>>> ===================================================================
>>> RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/fs/hpfs/hpfs_vnops.c,v
>>> retrieving revision 1.68
>>> diff -u -r1.68 hpfs_vnops.c
>>> --- sys/fs/hpfs/hpfs_vnops.c	17 Jan 2006 17:29:01 -0000	1.68
>>> +++ sys/fs/hpfs/hpfs_vnops.c	30 Oct 2006 17:07:55 -0000
>>> @@ -501,11 +501,12 @@
>>>  	if (vap->va_atime.tv_sec != VNOVAL || vap->va_mtime.tv_sec != VNOVAL) 
>>> {
>>>  		if (vp->v_mount->mnt_flag & MNT_RDONLY)
>>>  			return (EROFS);
>>> -		if (cred->cr_uid != hp->h_uid &&
>>> -		    (error = suser_cred(cred, SUSER_ALLOWJAIL)) &&
>>> -		    ((vap->va_vaflags & VA_UTIMES_NULL) == 0 ||
>>> -		    (error = VOP_ACCESS(vp, VWRITE, cred, td))))
>>> -			return (error);
>>> +		if (vap->va_vaflags & VA_UTIMES_NULL) {
>>> +			error = VOP_ACCESS(vp, VADMIN, cred, td);
>>> +			if (error)
>>> +				error = VOP_ACCESS(vp, VWRITE, cred, td);
>>> +		} else
>>> +			error = VOP_ACCESS(vp, VADMIN, cred, td);
>> 
>> Eliminating privilege check here was intentional? Doesn't it change 
>> functionality? I found the same check in few other places, so it's probably 
>> intentional, but worth checking still.
>
> Yeah, it took my quite a while to puzzle through what the security check here 
> is supposed to accomplish, but once I did, the correct structure became more 
> clear.
> 
> The key here is that VOP_ACCESS() will also perform a privilege 
> check, so the use of privilege in the existing code appears to be redundant. 
> The logic here should essentially read:
> ...

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 :-).
The bug is that the VADMIN change had not reached many poorly maintained
file systems.

> - To update the time stamp to the current time (VA_UTIMES_NULL), you must
>  either be the owner or have write access.  The correct error value is
>  EACCES, so we try the write check only if the owner check fails, so that 
> the
>  error from the write check overrides the owner result.
>
> - To update the time stamp to any other time, you must be the owner.  The
>  error here is EPERM on failure.
>
> Given this description, do you think the change is right?

Yes, but the code is unnecessarily convoluted.  After reviewing the above,
I changed my version of the corresponding code in ffs to:

% Index: ufs_vnops.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_vnops.c,v
% retrieving revision 1.239
% diff -u -2 -r1.239 ufs_vnops.c
% --- ufs_vnops.c	7 Apr 2004 03:47:20 -0000	1.239
% +++ ufs_vnops.c	1 Nov 2006 11:43:10 -0000
% @@ -578,8 +629,17 @@
%  		 * the file or be the super-user.
%  		 */
% -		if ((error = VOP_ACCESS(vp, VADMIN, cred, td)) &&
% -		    ((vap->va_vaflags & VA_UTIMES_NULL) == 0 ||
% -		    (error = VOP_ACCESS(vp, VWRITE, cred, td))))
% +		error = VOP_ACCESS(vp, VADMIN, cred, td);
% +		if (error && (vap->va_vaflags & VA_UTIMES_NULL) != 0)
% +			error = VOP_ACCESS(vp, VWRITE, cred, td);
% +		if (error)
%  			return (error);
% ...

I had a fixup to error = EPERM for one case in here, but it this is
unnecessary since VADMIN always (I hope) gives EPERM on error.

In ffs, this is just a style change.  The comment before the code should
probably be removed now that the code is slightly clearer.  It does little
more than echo the code, and doesn't mention the most unclear detail --
that we are depending on VADMIN/VWRITE always causing VOP_ACCESS() to
give the error code that we want.  The comment also does less than echo
the code, since it still says "super_user", but the code hasn't said
suser since rev.1.151.

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

>>> --- sys/fs/msdosfs/msdosfs_vfsops.c	26 Sep 2006 04:12:45 -0000	1.153
>>> +++ sys/fs/msdosfs/msdosfs_vfsops.c	30 Oct 2006 17:07:55 -0000
>> [...]
>>> @@ -293,17 +294,17 @@
>>>  			 * If upgrade to read-write by non-root, then verify
>>>  			 * that user has necessary permissions on the device.
>>>  			 */
>>> -			if (suser(td)) {
>>> -				devvp = pmp->pm_devvp;
>>> -				vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
>>> -				error = VOP_ACCESS(devvp, VREAD | VWRITE,
>>> -						   td->td_ucred, td);
>>> -				if (error) {
>>> -					VOP_UNLOCK(devvp, 0, td);
>>> -					return (error);
>>> -				}
>>> +			devvp = pmp->pm_devvp;
>>> +			vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
>>> +			error = VOP_ACCESS(devvp, VREAD | VWRITE,
>>> +			   td->td_ucred, td);
>>> +			if (error)
>>> +				error = priv_check(td, PRIV_VFS_MOUNT_PERM);
>>> +			if (error) {
>>>  				VOP_UNLOCK(devvp, 0, td);
>>> +				return (error);
>>>  			}
>>> +			VOP_UNLOCK(devvp, 0, td);
>> 
>> This doesn't seem right to me. If VOP_ACCESS() returns an error if call 
>> priv_check(), I think you wanted to call it when it return success, no?
>> 
>> I'd change it to:
>> 
>> 			devvp = pmp->pm_devvp;
>> 			vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
>> 			error = VOP_ACCESS(devvp, VREAD | VWRITE,
>> 			   td->td_ucred, td);
>> 			VOP_UNLOCK(devvp, 0, td);
>> 			if (!error)
>> 				error = priv_check(td, PRIV_VFS_MOUNT_PERM);
>> 			if (error)
>> 				return (error);
>
> Again, this is a tricky case in the original code.  I believe the intended 
> logic here, in English, is:
>
> - In order to mount on a device vnode, you must be able to read and write to
>  it.  This is subject to the normal definition of read and write privilege,
>  so the superuser can override on that basis (as part of VOP_ACCESS).
>
> - However, if you don't have read and write access, you can also use 
> privilege
>  (PRIV_VFS_MOUNT_PERM) to override that.
>
> This is expressed in the original code by first checking for privilege, and 
> if that fails, then looking for read/write access.  This doesn't make a lot 
> of sense because the read/write check is also exempted by privilege, but was 
> probably "a little faster" because suser() was cheaper than VOP_ACCESS().
>
> The reason we call priv_check() in the error case is that we only want to 
> check for privilege if the normal access control check fails, in which case 
> we override the normal access control error check with the result of the 
> privilege check.
>
> Given this description, does the new code still make sense?

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.

Here are some small points that I noticed:

>>> Index: sys/dev/fdc/fdc.c
>>> ===================================================================
>>> RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/dev/fdc/fdc.c,v
>>> retrieving revision 1.313
>>> diff -u -r1.313 fdc.c
>>> --- sys/dev/fdc/fdc.c	8 Sep 2006 21:46:00 -0000	1.313
>>> +++ sys/dev/fdc/fdc.c	30 Oct 2006 17:07:54 -0000
>>> @@ -1489,8 +1490,9 @@
>>>   		return (0);
>>> 
>>>   	case FD_CLRERR:
>>> -		if (suser(td) != 0)
>>> -			return (EPERM);
>>> +		error = priv_check(td, PRIV_DRIVER);
>>> +		if (error)
>>> +			return (error);
>>>   		fd->fdc->fdc_errs = 0;
>>>   		return (0);

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

>>> 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.

>>> Index: sys/fs/msdosfs/msdosfs_vnops.c
>>> ===================================================================
>>> RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/fs/msdosfs/msdosfs_vnops.c,v
>>> retrieving revision 1.164
>>> diff -u -r1.164 msdosfs_vnops.c
>>> --- sys/fs/msdosfs/msdosfs_vnops.c	24 Oct 2006 11:14:05 -0000	1.164
>>> +++ sys/fs/msdosfs/msdosfs_vnops.c	30 Oct 2006 17:07:55 -0000
On Tue, 31 Oct 2006, Robert Watson wrote:

> On Tue, 31 Oct 2006, Pawel Jakub Dawidek wrote:
>
>> Here are few nits I found:
>
> Thanks!  Comments below.
>
>>> Index: sys/fs/hpfs/hpfs_vnops.c

It (all of `>>>') is too much inteface bloat for me to like.

>>> ===================================================================
>>> RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/fs/hpfs/hpfs_vnops.c,v
>>> retrieving revision 1.68
>>> diff -u -r1.68 hpfs_vnops.c
>>> --- sys/fs/hpfs/hpfs_vnops.c	17 Jan 2006 17:29:01 -0000	1.68
>>> +++ sys/fs/hpfs/hpfs_vnops.c	30 Oct 2006 17:07:55 -0000
>>> @@ -501,11 +501,12 @@
>>>  	if (vap->va_atime.tv_sec != VNOVAL || vap->va_mtime.tv_sec != VNOVAL) 
>>> {
>>>  		if (vp->v_mount->mnt_flag & MNT_RDONLY)
>>>  			return (EROFS);
>>> -		if (cred->cr_uid != hp->h_uid &&
>>> -		    (error = suser_cred(cred, SUSER_ALLOWJAIL)) &&
>>> -		    ((vap->va_vaflags & VA_UTIMES_NULL) == 0 ||
>>> -		    (error = VOP_ACCESS(vp, VWRITE, cred, td))))
>>> -			return (error);
>>> +		if (vap->va_vaflags & VA_UTIMES_NULL) {
>>> +			error = VOP_ACCESS(vp, VADMIN, cred, td);
>>> +			if (error)
>>> +				error = VOP_ACCESS(vp, VWRITE, cred, td);
>>> +		} else
>>> +			error = VOP_ACCESS(vp, VADMIN, cred, td);
>> 
>> Eliminating privilege check here was intentional? Doesn't it change 
>> functionality? I found the same check in few other places, so it's probably 
>> intentional, but worth checking still.
>
> Yeah, it took my quite a while to puzzle through what the security check here 
> is supposed to accomplish, but once I did, the correct structure became more 
> clear.
> 
> The key here is that VOP_ACCESS() will also perform a privilege 
> check, so the use of privilege in the existing code appears to be redundant. 
> The logic here should essentially read:
> ...

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 :-).
The bug is that the VADMIN change had not reached many poorly maintained
file systems.

> - To update the time stamp to the current time (VA_UTIMES_NULL), you must
>  either be the owner or have write access.  The correct error value is
>  EACCES, so we try the write check only if the owner check fails, so that 
> the
>  error from the write check overrides the owner result.
>
> - To update the time stamp to any other time, you must be the owner.  The
>  error here is EPERM on failure.
>
> Given this description, do you think the change is right?

Yes, but the code is unnecessarily convoluted.  After reviewing the above,
I changed my version of the corresponding code in ffs to:

% Index: ufs_vnops.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_vnops.c,v
% retrieving revision 1.239
% diff -u -2 -r1.239 ufs_vnops.c
% --- ufs_vnops.c	7 Apr 2004 03:47:20 -0000	1.239
% +++ ufs_vnops.c	1 Nov 2006 11:43:10 -0000
% @@ -578,8 +629,17 @@
%  		 * the file or be the super-user.
%  		 */
% -		if ((error = VOP_ACCESS(vp, VADMIN, cred, td)) &&
% -		    ((vap->va_vaflags & VA_UTIMES_NULL) == 0 ||
% -		    (error = VOP_ACCESS(vp, VWRITE, cred, td))))
% +		error = VOP_ACCESS(vp, VADMIN, cred, td);
% +		if (error && (vap->va_vaflags & VA_UTIMES_NULL) != 0)
% +			error = VOP_ACCESS(vp, VWRITE, cred, td);
% +		if (error)
%  			return (error);
% ...

I had a fixup to error = EPERM for one case in here, but it this is
unnecessary since VADMIN always (I hope) gives EPERM on error.

In ffs, this is just a style change.  The comment before the code should
probably be removed now that the code is slightly clearer.  It does little
more than echo the code, and doesn't mention the most unclear detail --
that we are depending on VADMIN/VWRITE always causing VOP_ACCESS() to
give the error code that we want.  The comment also does less than echo
the code, since it still says "super_user", but the code hasn't said
suser since rev.1.151.

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

>>> --- sys/fs/msdosfs/msdosfs_vfsops.c	26 Sep 2006 04:12:45 -0000	1.153
>>> +++ sys/fs/msdosfs/msdosfs_vfsops.c	30 Oct 2006 17:07:55 -0000
>> [...]
>>> @@ -293,17 +294,17 @@
>>>  			 * If upgrade to read-write by non-root, then verify
>>>  			 * that user has necessary permissions on the device.
>>>  			 */
>>> -			if (suser(td)) {
>>> -				devvp = pmp->pm_devvp;
>>> -				vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
>>> -				error = VOP_ACCESS(devvp, VREAD | VWRITE,
>>> -						   td->td_ucred, td);
>>> -				if (error) {
>>> -					VOP_UNLOCK(devvp, 0, td);
>>> -					return (error);
>>> -				}
>>> +			devvp = pmp->pm_devvp;
>>> +			vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
>>> +			error = VOP_ACCESS(devvp, VREAD | VWRITE,
>>> +			   td->td_ucred, td);
>>> +			if (error)
>>> +				error = priv_check(td, PRIV_VFS_MOUNT_PERM);
>>> +			if (error) {
>>>  				VOP_UNLOCK(devvp, 0, td);
>>> +				return (error);
>>>  			}
>>> +			VOP_UNLOCK(devvp, 0, td);
>> 
>> This doesn't seem right to me. If VOP_ACCESS() returns an error if call 
>> priv_check(), I think you wanted to call it when it return success, no?
>> 
>> I'd change it to:
>> 
>> 			devvp = pmp->pm_devvp;
>> 			vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
>> 			error = VOP_ACCESS(devvp, VREAD | VWRITE,
>> 			   td->td_ucred, td);
>> 			VOP_UNLOCK(devvp, 0, td);
>> 			if (!error)
>> 				error = priv_check(td, PRIV_VFS_MOUNT_PERM);
>> 			if (error)
>> 				return (error);
>
> Again, this is a tricky case in the original code.  I believe the intended 
> logic here, in English, is:
>
> - In order to mount on a device vnode, you must be able to read and write to
>  it.  This is subject to the normal definition of read and write privilege,
>  so the superuser can override on that basis (as part of VOP_ACCESS).
>
> - However, if you don't have read and write access, you can also use 
> privilege
>  (PRIV_VFS_MOUNT_PERM) to override that.
>
> This is expressed in the original code by first checking for privilege, and 
> if that fails, then looking for read/write access.  This doesn't make a lot 
> of sense because the read/write check is also exempted by privilege, but was 
> probably "a little faster" because suser() was cheaper than VOP_ACCESS().
>
> The reason we call priv_check() in the error case is that we only want to 
> check for privilege if the normal access control check fails, in which case 
> we override the normal access control error check with the result of the 
> privilege check.
>
> Given this description, does the new code still make sense?

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.

Here are some small points that I noticed:

>>> Index: sys/dev/fdc/fdc.c
>>> ===================================================================
>>> RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/dev/fdc/fdc.c,v
>>> retrieving revision 1.313
>>> diff -u -r1.313 fdc.c
>>> --- sys/dev/fdc/fdc.c	8 Sep 2006 21:46:00 -0000	1.313
>>> +++ sys/dev/fdc/fdc.c	30 Oct 2006 17:07:54 -0000
>>> @@ -1489,8 +1490,9 @@
>>>   		return (0);
>>> 
>>>   	case FD_CLRERR:
>>> -		if (suser(td) != 0)
>>> -			return (EPERM);
>>> +		error = priv_check(td, PRIV_DRIVER);
>>> +		if (error)
>>> +			return (error);
>>>   		fd->fdc->fdc_errs = 0;
>>>   		return (0);

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

>>> 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.

>>> Index: sys/fs/msdosfs/msdosfs_vnops.c
>>> ===================================================================
>>> RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/fs/msdosfs/msdosfs_vnops.c,v
>>> retrieving revision 1.164
>>> diff -u -r1.164 msdosfs_vnops.c
>>> --- sys/fs/msdosfs/msdosfs_vnops.c	24 Oct 2006 11:14:05 -0000	1.164
>>> +++ sys/fs/msdosfs/msdosfs_vnops.c	30 Oct 2006 17:07:55 -0000
>>> @@ -404,9 +405,12 @@
>>>   	if (vap->va_flags != VNOVAL) {
>>>   		if (vp->v_mount->mnt_flag & MNT_RDONLY)
>>>   			return (EROFS);
>>> -		if (cred->cr_uid != pmp->pm_uid &&
>>> -		    (error = suser_cred(cred, SUSER_ALLOWJAIL)))
>>> -			return (error);
>>> +		if (cred->cr_uid != pmp->pm_uid) {
>>> +			error = priv_check_cred(cred, PRIV_VFS_ADMIN,
>>> +			    SUSER_ALLOWJAIL);
>>> +			if (error)
>>> +				return (error);
>>> +		}
>>>   		/*
>>>   		 * We are very inconsistent about handling unsupported
>>>   		 * attributes.  We ignored the access time and the
>>> ,,,

msdosfs has a lot of hacks related mapping a set of attributes in into a
smaller set of attributes without always failing because a 1-1 map is
impossible.  It uses suser* a lot for this, but perhaps it shouldn't.
The privilege for breaking the mapping should probably be different
from generic VFS_ADMIN privilege.

>>> Index: sys/kern/kern_descrip.c
>>> ===================================================================
>>> RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/kern/kern_descrip.c,v
>>> retrieving revision 1.298
>>> diff -u -r1.298 kern_descrip.c
>>> --- sys/kern/kern_descrip.c	24 Sep 2006 02:29:53 -0000	1.298
>>> +++ sys/kern/kern_descrip.c	30 Oct 2006 17:07:55 -0000
>>> @@ -1351,8 +1352,8 @@
>>>   	sx_xlock(&filelist_lock);
>>> 
>>>   	if ((openfiles >= maxuserfiles &&
>>> -	     suser_cred(td->td_ucred, SUSER_RUID) != 0) ||
>>> -	    openfiles >= maxfiles) {
>>> +	     priv_check_cred(td->td_ucred, PRIV_MAXFILES, SUSER_RUID) != 0)
>>> +	    || openfiles >= maxfiles) {

New style bugs: 5-char continuation indent, and || not at end of line.

>>>   		if (ppsratecheck(&lastfail, &curfail, 1)) {
>>>   			printf("kern.maxfiles limit exceeded by uid %i, please see tuning(7).\n",
>>>   				td->td_ucred->cr_ruid);

Grosser old style bugs.

>>> 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.

>>> @@ -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

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

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061101231230.B31271>