Re: [AppArmor 38/41] AppArmor: Module and LSM hooks

[John Johansen <jjohansen@suse.de>]

On Thu, Apr 12, 2007 at 11:21:01AM +0100, Alan Cox wrote:
> > +
> > +	/**
> > +	 * parent can ptrace child when
> > +	 * - parent is unconfined
> > +	 * - parent is in complain mode
> > +	 * - parent and child are confined by the same profile
> > +	 */
> 
> Your profiles are name based. That means the same profile in a different
> namespace does different things. It would be a very odd case where it
> mattered but surely the parent ptrace child rule should also require that
> the parent and child are in the same namespace when using apparmor name
> based security.
> 
you are right we should be requiring parent and child are in the same
namespace.  This has been fixed.

> > +static int apparmor_capget(struct task_struct *task,
> > +			    kernel_cap_t *effective,
> > +			    kernel_cap_t *inheritable,
> > +			    kernel_cap_t *permitted)
> > +{
> > +	return cap_capget(task, effective, inheritable, permitted);
> > +}
> 
> Pointless function should go away.
> 
yes we had a few of those thanks for pointing it out.

> > +static int apparmor_sysctl(struct ctl_table *table, int op)
> > +{
> > +	int error = 0;
> > +
> > +	if ((op & 002) && !capable(CAP_SYS_ADMIN))
> > +		error = aa_reject_syscall(current, GFP_KERNEL,
> > +					  "sysctl (write)");
> > +
> > +	return error;
> 
> The usual file permission security override is DAC not ADMIN. What is the
> logic of this choice.
> 
This was a very course grain check that was done to restrict access to
sysctl's that could be potentially used to elevated priledge.  The check
is inconsistent with AppArmor's model and we should be modelling
sysctl accesses as pathname access, and then we could be using standard
mediation.

thanks for the review
john

Attachment: pgp1t0kPuPP7K.pgp
Description: PGP signature