2007-05-23 19:06:38

by Andreas Gruenbacher

[permalink] [raw]
Subject: Re: [AppArmor 01/41] Pass struct vfsmount to the inode_create LSM hook

On Thursday 12 April 2007 12:12, Al Viro wrote:
> On Thu, Apr 12, 2007 at 02:08:10AM -0700, [email protected] wrote:
> > This is needed for computing pathnames in the AppArmor LSM.
>
> Which is an argument against said LSM in current form.

The fundamental model of AppArmor is to perform access checks based on the
location of a file in the filesystem namespace, i.e., the pathname, and this
can only be done with both the dentry and vfsmount. My understanding was that
there was agreement at the last kernel summit that pathname based approaches
should be allowed.

> > - error = security_inode_create(dir, dentry, mode);
> > + error = security_inode_create(dir, dentry, nd ? nd->mnt : NULL, mode);
>
> is a clear sign that interface is wrong.

No. There are callers of vfs_create() that use a NULL nameidata, and that's
what causes the problem here. Struct nameidata is pretty big, and so we don't
want to allocate temporary nameidata objects all over the place. So to me the
above NULL check seems the lesser evil.

One way to deal with the nameidata size problem is to split it up into one
part for the real path lookups, and a much smaller part that only contains
the dentry, vfsmount, and intent flags. This would allow to pass around the
smaller nameidata much more consistently.

John has posted patches for that on May 14; subject [RFD Patch 0/4]. Feedback
appreciated.

In several places, the NULL nameidata is wrong because we can't check the
vfsmount flags or intent. Not having this information is causing problems for
nfs too for example -- it's not an AppArmor specific problem.

> Leaving aside the general idiocy of "we prohibit to do something with file
> if mounted here, but if there's another mountpoint, well, we just miss", an
> API of that kind is just plain wrong. Either you can live without seeing
> vfsmount in that method (in which case you shouldn't pass it at all), or you
> have a hole.

This is backwards from what AppArmor does. The policy defines which paths may
be accessed; all paths not explicitly listed are denied. If files are mounted
at multiple locations, then the policy may allow access to some locations but
not to others. That's not a hole.

In fact this is not much different from traditional permissions on parent
directories: even if the same files are mounted at several locations, parent
directory permissions may allow accessing only some of those locations.

Thanks,
Andreas


2007-05-24 01:29:14

by James Morris

[permalink] [raw]
Subject: Re: [AppArmor 01/41] Pass struct vfsmount to the inode_create LSM hook

On Wed, 23 May 2007, Andreas Gruenbacher wrote:

> This is backwards from what AppArmor does. The policy defines which paths may
> be accessed; all paths not explicitly listed are denied. If files are mounted
> at multiple locations, then the policy may allow access to some locations but
> not to others. That's not a hole.

I don't know what else you'd call it.

Would you mind providing some concrete examples of how such a model would
be useful?


- James
--
James Morris
<[email protected]>

2007-05-24 09:17:23

by Andreas Gruenbacher

[permalink] [raw]
Subject: Re: [AppArmor 01/41] Pass struct vfsmount to the inode_create LSM hook

On Thursday 24 May 2007 03:28, James Morris wrote:
> On Wed, 23 May 2007, Andreas Gruenbacher wrote:
> > This is backwards from what AppArmor does. The policy defines which paths
> > may be accessed; all paths not explicitly listed are denied. If files are
> > mounted at multiple locations, then the policy may allow access to some
> > locations but not to others. That's not a hole.
>
> I don't know what else you'd call it.

AppArmor doesn't label files; it's a different model from SELinux. Its policy
defines which processes may access which paths. Even if for some reson the
same files were visible elsewhere, the policy wouldn't cover those other
paths, and so accessing them would be denied. So again, that's not a security
hole.

> Would you mind providing some concrete examples of how such a model would
> be useful?

The model is explained, with examples, in the technical documentation at
http://forgeftp.novell.com//apparmor/LKML_Submission-May_07/.

Thanks,
Andreas

2007-05-24 12:52:01

by Tetsuo Handa

[permalink] [raw]
Subject: Re: [AppArmor 01/41] Pass struct vfsmount to the inode_create LSMhook

Hello.

I think bind mounts were discussed when shared subtree
( http://lwn.net/Articles/159092/ ) was introduced.

For systems that allow users mount their CD/DVDs freely,
bind mounts are used and labeling files is a convenient way
to deny accessing somebody else's files.

But systems that don't allow users mount their CD/DVDs freely,
bind mounts needn't to be used and using pathnames is a convenient way
to deny accessing somebody else's files.

Pathname based access control/auditing system
works if the system doesn't use bind mounts.

However, there are distributions (e.g. Debian Etch)
that always use bind mounts. In such distributions,
pathname based access control/auditing system doesn't work.

This is not the fault of distributions nor
pathname based access control/auditing system.
It is possible to solve by passing vfsmount to VFS and LSM functions.

SELinux users are having a lot of trouble because pathnames in audit logs
are not always complete.
AppArmor users are having a lot of trouble because pathnames which
a process requested are ambiguous when bind mounts are used.

Being able to report pathnames that a process requested is not surprising
when considering user friendliness.
I beleive passing vfsmount makes both users happy.

Thanks.