Re: Updated auth patch for 2.4.12 - kernel/sys.c

offerat_private

* frm chrisat_private "10/16/2001 07:34:42 PM -0700" | sed '1,$s/^/* /'

I'll handle these issues one-per-message to try and cut down on code
overload.

* see more comments below:
* 
** richard offer (offerat_private) wrote:
* 
*> ===== kernel/sys.c 1.18 vs edited =====
*> @@ -820,6 +813,10 @@
*>  	if (!p)
*>  		goto out;
*>  
*> +	err = security_ops->task_ops->setpgid(p, pgid);
*> +	if (err)
*> +		goto out;
*> +
*>  	if (p->p_pptr == current || p->p_opptr == current) {
*>  		err = -EPERM;
*>  		if (p->session != current->session)
*> @@ -843,10 +840,6 @@
*>  	}
*>  
*>  ok_pgid:
*> -	err = security_ops->task_ops->setpgid(p, pgid);
*> -	if (err)
*> -		goto out;
*> -
*>  	p->pgrp = pgid;
*>  	err = 0;
*>  out:
* 
* what are you trying to do here?

Its the old MAC before DAC vs DAC before MAC issue (nothing to do with
audit). Moving the hook before any kernel logic turns the hook into
authoritative without having to pass any errno or mangle the code path.
(Since this is not filesystem related, I made a rash assumption that Wirex
would have less problem with moving the hook before the DAC logic)

We can make it move it back to its original (post kernel checks) position
at the risk of making the code grottier :-)

If we did that, we'd want to pass the err value in (since this is one of
the few system calls that can return different errors) so we'd probably end
up with something like (untested)

	err = 0;
	if (p->p_pptr == current || p->p_opptr == current) {
		if (p->session != current->session)
			err = -EPERM;
		if (p->did_exec)
			err = -EACCES;
	} else if (p != current)
		goto out;
	if (p->leader)
		err = -EPERM;
	if (err == 0 && pgid != pid) {
		struct task_struct * tmp;
		for_each_task (tmp) {
			if (tmp->pgrp == pgid &&
			    tmp->session == current->session)
				goto ok_pgid;
		}
		goto out;
	}

ok_pgid:
	err = security_ops->task_ops->setpgid(err, p, pgid);
	if (err)
		goto out;

I can go either way.

* 
*> @@ -1123,7 +1116,7 @@
*>  asmlinkage long sys_setrlimit(unsigned int resource, struct rlimit
*>  *rlim) {
*>  	struct rlimit new_rlim, *old_rlim;
*> -	int retval;
*> +	int retval = 0;
*>  
*>  	if (resource >= RLIM_NLIMITS)
*>  		return -EINVAL;
*> @@ -1133,13 +1126,13 @@
*>  	if (((new_rlim.rlim_cur > old_rlim->rlim_max) ||
*>  	     (new_rlim.rlim_max > old_rlim->rlim_max)) &&
*>  	    !capable(CAP_SYS_RESOURCE))
*> -		return -EPERM;
*> -	if (resource == RLIMIT_NOFILE) {
*> +		retval = -EPERM;
*> +	if (!retval && resource == RLIMIT_NOFILE) {
*>  		if (new_rlim.rlim_cur > NR_OPEN || new_rlim.rlim_max > NR_OPEN)
* 
* ugh, i mentioned this before.  the '!retval &&' just looks bad ;-(
* however, in this case...you have no way to distiguish between i don't
* have the capability and you are trying to increase beyond hard kernel
* limit (NR_OPEN).  sure you can redo check in hook, but this seems bad.
* while the NR_OPEN check can return -EPERM, it is pretty close to
* functional logic (kind of like, is this filesystem read-only).
* 

I think the question comes down to, do we want to preserve pre-LSM code
execution order, or not ?

I could just as easily see the following

    ....
	if (((new_rlim.rlim_cur > old_rlim->rlim_max) ||
	     (new_rlim.rlim_max > old_rlim->rlim_max)) &&
	    !capable(CAP_SYS_RESOURCE))
		retval = -EPERM;
	if (resource == RLIMIT_NOFILE) {
		if (new_rlim.rlim_cur > NR_OPEN || new_rlim.rlim_max > NR_OPEN)
			retval = -EPERM;
	}
	retval = security_ops->task_ops->setrlimit(retval, resource, &new_rlim);
	....

But that risks going through a code path (the test for NR_OPEN) even if you
don't have privilege. It seems its safer to not undertake any other action
(which may or may not have side-effects) if you have an error.

(One could argue that if you hit the rlim_max > NR_OPEN that you should
return EINVAL (or some other code) rather than EPERM but that's a different
issue).

* 
* -chris
* 

*

-- 
-----------------------------------------------------------------------
Richard Offer                     Technical Lead, Trust Technology, SGI
"Specialization is for insects"
_______________________________________________________________________

_______________________________________________
linux-security-module mailing list
linux-security-moduleat_private
http://mail.wirex.com/mailman/listinfo/linux-security-module