Re: [RFC][PATCH 4/11] security: AppArmor - Core access controls

[Tony Jones]

On Thu, Apr 20, 2006 at 06:40:10AM -0500, Serge E. Hallyn wrote:
> Quoting Al Viro (viro@xxxxxxxxxxxxxxxx):
> > 
> > > +static int _aa_perm_dentry(struct aaprofile *active, struct dentry *dentry,
> > > +			   int mask, const char **pname)
> > > +{
> > > +	char *name = NULL, *failed_name = NULL;
> > > +	struct aa_path_data data;
> > > +	int error = 0, failed_error = 0, path_error,
> > > +	    complain = PROFILE_COMPLAIN(active);
> > > +
> > > +	/* search all paths to dentry */
> > > +
> > > +	aa_path_begin(dentry, &data);
> > > +	do {
> > > +		name = aa_path_getname(&data);
> > > +		if (name) {
> > > +			/* error here is 0 (success) or +ve (mask of perms) */
> > > +			error = aa_file_perm(active, name, mask);
> > > +
> > > +			/* access via any path is enough */
> > > +			if (complain || error == 0)
> > > +				break; /* Caller must free name */
> > > +
> > > +			/* Already have an path that failed? */
> > > +			if (failed_name) {
> > > +				aa_put_name(name);
> > > +			} else {
> > > +				failed_name = name;
> > > +				failed_error = error;
> > > +			}
> > > +		}
> > > +	} while (name);
> > 
> > Is that a joke?  Are you really proposing to do _that_ on anything resembling
> > a hot path?

Unfortunately Al, no it's not a joke. We've been asked to publish performance 
numbers by Serge as part of another thread. We plan to do so shortly. Of 
course results are likely going to be related to the complexity of the 
namespace the benchmark operates within. Suggestions of benchmarks that
significantly exercise namespaces are more than welcome.

We are no fan of this code either but the fact is that vfsmounts are passed
inconsistently to the LSM.  Of course this isn't an issue of LSM just not
taking available data, rather of the information not being available in the
VFS at the point the hook is invoked.   Going out on a limb here, to fully 
support read-only bind mounts would seem to require similar changes - but 
with a more limited scope - cases like security_inode_create and 
security_inode_link likely still wouldn't have the necessary information to 
fully eliminate the above fuglyness. Perhaps one hook cannot be made to provide
both useful inode and name information.

> > BTW, the problems here really have nothing to do with namespaces or
> > lazy umount, seeing that it's whitelisting.  Moderate amount of bindings
> > will kill you here.  So much that I suspect that one-time overhead of
> > creating a namespace and umounting / remounting noexec / etc. on
> > execve() will be cheaper than all this crap.
> 
> I guess this would require per-vfsmount flags (i.e. mount --bind -o ro)
> to be implemented, but IIUC the suggestion is
> 
> given a policy
> 
> /bin/stty {
> 	/bin/stty r
> }
> 
> during execve AA would unshare(CLONE_NEWNS), remount / readonly and
> noexec,  and mount /bin/stty into place with exec privs.  I guess
> getting /bin/stty into place shouldn't be much of a challenge (i.e.
> just do the operations in the order
> 	mkdir /.tmp123
> 	mount --bind -o ro,noexec / /.tmp123
> 	mount --bind /bin/stty /.tmp123/bin/stty
> 	mount --bind /.tmp123 /
> )
> but implementing the 'ux' exec permission which apparmor currently has
> (i.e. giving the ability for stty to then execute /bin/login without
> restrictions) could be more challenging.
> 
> This also might beg for sys_unshare() (and corresponding code in clone)
> to have it's own security_vfs_unshare() hook, rather than being globbed
> in with CAP_SYS_ADMIN.

Are we referring here to the idea of giving each confined task it's own
namespace upon exec?  An interesting idea for sure.  The exec portion you
mention above is pretty trivial.  How to handle directories, scratch space
(the ability of a confined task to write selected temp files) is less clear.
Also one of the most powerful aspects of AppArmor (at least if the users are
to be believed :-) is the ability for policy to contain path name expansion
(globbing). For instance, it is very useful to grant one web application 
access to /var/www/**.html and another access to /var/www/**.pl.

But I think passing vfsmounts fully into LSM and closing the cases where a
nameidata can be NULL is an alternative plan B.  Something we are willing to
put effort into helping achieve.

I believe what users want is a system which offers good practical security
together with ease of expressiveness in policy (so that it may be actually
maintained by other than distribution vendors). We strongly believe that 
AppArmor provides this and think it is important to persue changes to LSM 
(and the VFS) as necessary.  However there is clearly an undeniable elegance 
to the per-confined-task namespace idea. I have my concerns about whether it 
can achieve close to the same expressiveness as current AppArmor policy (one 
of AppArmor's clear advantages over SELinux) but it is clearly important that 
the namespace idea is explored. Just not to the exclusion of also exploring 
rework of the LSM/VFS.

Tony
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/