Adding a security parameter to VFS functions

Adding a security parameter to VFS functions

[David Howells]

Hi Linus, Al,

Would you object greatly to functions like vfs_mkdir() gaining a security
parameter?  What I'm thinking of is this:

	int vfs_mkdir(struct inode *dir, struct dentry *dentry, int mode,
		      struct security *security)

Where the security context is the state of the context at the time the call
was issued:

	struct security {
		uid_t			fsuid;
		git_t			fsgid;
		struct group_info	*group_info;
		void			*security;
		struct key		*session_keyring;
		struct key		*process_keyring;
		struct key		*thread_keyring;

And perhaps:

		struct audit_context	*audit_context;
		seccomp_t		seccomp;
	};

This would, for the most part, be a temporary affair, being set up by such as
sys_mkdir()/sys_mkdirat() from data held in task_struct.

This information would then be passed into the filesystem and LSM layers so
that files, directories, etc. can be created, opened, deleted, or otherwise
mangled based on these security items, rather than the one in whichever
task_struct is current.


The reason for doing this would be to support an act-as interface, so that
services such as nfsd and cachefiles could act with different security details
to the ones attached to the task.  This would have a couple of potential
benefits:

 (1) nfsd threads don't have to keep changing their security contexts.

 (2) cachefiles can act on behalf of a process without changing its security
     context.


Note that I/O operations such as read, write and ioctl would *not* be passed
this data as the file struct should contain the relevant security information.
Similarly, page I/O operations would also not need alteration as the VMA
covering the region points to a file struct, which holds the appropriate
security.

David

Re: Adding a security parameter to VFS functions

[Casey Schaufler]

--- David Howells <dhowells@redhat.com> wrote:

> 
> Hi Linus, Al,
> 
> Would you object greatly to functions like vfs_mkdir() gaining a security
> parameter?

Could you describe how this compares to the proposal that the
AppArmor developers suggested recently? I expect that we can 
reduce the amount of discussion required, and maybe avoid some
confusion if you could do that.

Thank you.

> What I'm thinking of is this:
> 
> 	int vfs_mkdir(struct inode *dir, struct dentry *dentry, int mode,
> 		      struct security *security)
> 
> Where the security context is the state of the context at the time the call
> was issued:
> 
> 	struct security {
> 		uid_t			fsuid;
> 		git_t			fsgid;
> 		struct group_info	*group_info;
> 		void			*security;
> 		struct key		*session_keyring;
> 		struct key		*process_keyring;
> 		struct key		*thread_keyring;
> 
> And perhaps:
> 
> 		struct audit_context	*audit_context;
> 		seccomp_t		seccomp;
> 	};
> 
> This would, for the most part, be a temporary affair, being set up by such as
> sys_mkdir()/sys_mkdirat() from data held in task_struct.
> 
> This information would then be passed into the filesystem and LSM layers so
> that files, directories, etc. can be created, opened, deleted, or otherwise
> mangled based on these security items, rather than the one in whichever
> task_struct is current.
> 
> 
> The reason for doing this would be to support an act-as interface, so that
> services such as nfsd and cachefiles could act with different security
> details
> to the ones attached to the task.  This would have a couple of potential
> benefits:
> 
>  (1) nfsd threads don't have to keep changing their security contexts.
> 
>  (2) cachefiles can act on behalf of a process without changing its security
>      context.
> 
> 
> Note that I/O operations such as read, write and ioctl would *not* be passed
> this data as the file struct should contain the relevant security
> information.
> Similarly, page I/O operations would also not need alteration as the VMA
> covering the region points to a file struct, which holds the appropriate
> security.
> 
> David
> 
> 
> 


Casey Schaufler
casey@schaufler-ca.com

Re: Adding a security parameter to VFS functions

[David Howells]

Casey Schaufler <casey@schaufler-ca.com> wrote:

> 
> Could you describe how this compares to the proposal that the
> AppArmor developers suggested recently? I expect that we can 
> reduce the amount of discussion required, and maybe avoid some
> confusion if you could do that.

I don't know what that is.

David

Re: Adding a security parameter to VFS functions

[Andreas Gruenbacher]

On Wednesday 15 August 2007 18:23, Casey Schaufler wrote:
> > Hi Linus, Al,
> > 
> > Would you object greatly to functions like vfs_mkdir() gaining a security
> > parameter?
> 
> Could you describe how this compares to the proposal that the
> AppArmor developers suggested recently? I expect that we can 
> reduce the amount of discussion required, and maybe avoid some
> confusion if you could do that.

That's from one of those patches:

-int vfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
+int vfs_mkdir(struct inode *dir, struct dentry *dentry, struct vfsmount *mnt,
+             int mode)

We need the vfsmount in the LSM hooks in addition to the dentry in order to 
figure out where in the filesystem namespace we are. The various vfs_ 
functions are the ones calling the LSM hooks. (The same could be achieved 
passing a struct path instead.)

-- Andreas

Re: Adding a security parameter to VFS functions

[Andreas Gruenbacher]

On Wednesday 15 August 2007 13:40, David Howells wrote:
> 
> Hi Linus, Al,
> 
> Would you object greatly to functions like vfs_mkdir() gaining a security
> parameter?  What I'm thinking of is this:
> 
> 	int vfs_mkdir(struct inode *dir, struct dentry *dentry, int mode,
> 		      struct security *security)
> 
> Where the security context is the state of the context at the time the call
> was issued:
> 
> 	struct security {
> 		uid_t			fsuid;
> 		git_t			fsgid;
> 		struct group_info	*group_info;
> 		void			*security;
> 		struct key		*session_keyring;
> 		struct key		*process_keyring;
> 		struct key		*thread_keyring;
> 
> And perhaps:
> 
> 		struct audit_context	*audit_context;
> 		seccomp_t		seccomp;
> 	};
> 
> This would, for the most part, be a temporary affair, being set up by such
> as sys_mkdir()/sys_mkdirat() from data held in task_struct.

That's additional setup work unless that struct can be embedded in 
task_struct. We would be complicating the common / fast / local case to 
simplify the not-so-common case or cases.

-- Andreas

Re: Adding a security parameter to VFS functions

[Linus Torvalds]

On Wed, 15 Aug 2007, David Howells wrote:
> 
> Would you object greatly to functions like vfs_mkdir() gaining a security
> parameter?  What I'm thinking of is this:
> 
> 	int vfs_mkdir(struct inode *dir, struct dentry *dentry, int mode,
> 		      struct security *security)

I personally consider this an affront to everythign that is decent.

Why the *hell* would mkdir() be so magical as to need something like that?

Make it something sane, like a "struct nameidata" instead, and make it at 
least try to look like the path creation that is done by "open()".  Or 
create a "struct file *" or something.

I can imagine having "mkdir()" being passed similar data as "open()" (ie 
"lookup()"), but I cannot _possibly_ imagine it ever being valid to pass 
in something totally made-up to just mkdir(), and nothing else. There's 
something fundamentally wrong there.

What makes mkdir() so magical?

Also, what about all the other ops? Why is mkdir() special, but not 
"mknod()"? Why is "mkdir()" special, but not "rmdir()"? Really, none of 
this seems to make any sense unless you describe what is so magical about 
mkdir().

		Linus

Re: Adding a security parameter to VFS functions

[Kyle Moffett]

On Aug 16, 2007, at 18:57:24, Linus Torvalds wrote:
> On Wed, 15 Aug 2007, David Howells wrote:
>> Would you object greatly to functions like vfs_mkdir() gaining a  
>> security parameter?  What I'm thinking of is this:
>> int vfs_mkdir(struct inode *dir, struct dentry *dentry, int mode,  
>> struct security *security)
>
> I personally consider this an affront to everythign that is decent.
>
> Why the *hell* would mkdir() be so magical as to need something  
> like that?

Not speaking directly for David, but I believe the reason is for  
background kernel code which needs to do filesystem access during a  
thread's execution with *completely* different security context from  
that of the thread.  Examples should be reasonably obvious; kNFSd is  
one, but it also includes anything where the kernel would poke  
directly into the filesystem, such as network filesystem cachefiles.

> Make it something sane, like a "struct nameidata" instead, and make  
> it at least try to look like the path creation that is done by "open 
> ()".  Or create a "struct file *" or something.
>
> I can imagine having "mkdir()" being passed similar data as "open 
> ()" (ie "lookup()"), but I cannot _possibly_ imagine it ever being  
> valid to pass in something totally made-up to just mkdir(), and  
> nothing else. There's something fundamentally wrong there.

I would offer the suggestion of using the described "struct security"  
in-place in the task struct, in place of using all those fields  
individually.  That would be, in effect the "default" security  
context for any given task, if "NULL" is passed to the appropriate  
vfs function.  For CacheFiles and kNFSd, they could each allocate  
their own during initialization or new-connection and pass that to  
any "mkdir()", etc that they do on behalf of a given client.


> What makes mkdir() so magical?
>
> Also, what about all the other ops? Why is mkdir() special, but not  
> "mknod()"? Why is "mkdir()" special, but not "rmdir()"? Really,  
> none of this seems to make any sense unless you describe what is so  
> magical about mkdir().

I think mkdir() was just an example he was using, probably because it  
was the first VFS call he needed to set a security context on.   Next  
would come anything which CacheFiles or NFSd call on the underlying  
filesystem.

Cheers,
Kyle Moffett

Re: Adding a security parameter to VFS functions

[Al Viro]

On Thu, Aug 16, 2007 at 03:57:24PM -0700, Linus Torvalds wrote:
> I personally consider this an affront to everythign that is decent.
> 
> Why the *hell* would mkdir() be so magical as to need something like that?
> 
> Make it something sane, like a "struct nameidata" instead, and make it at 
> least try to look like the path creation that is done by "open()".  Or 
> create a "struct file *" or something.

No.  I agree that mkdir/mknod/etc. should all take the same thing.
*However*, it should not be nameidata or file.  Why?  Because it
should not contain vfsmount.  Object creation should not *care*
where fs is mounted.  At all.  The same goes for open(), BTW - letting
it see the reference to vfsmount via nameidata is bloody wrong and
I really couldn't care less what kinds of perversions apparmour crowd
might like - pathnames simply do not belong there.

Said that, I agree that we need uniform prototypes for all these guys
and I can see arguments both for and against having creds passed by
reference stored in whatever struct we are passing (for: copy-on-write
refcounted cred objects would almost never have to be modified or
copied and normally we would just pick the current creds reference
from task_struct and assing it to a single field in whatever we pass
instead of nameidata; against: might be conceptually simpler to have
all that data directly in that struct and a helper filling it in).

Which way we do that and which struct we end up passing is a very good
question, but I want to make it very clear: having object creation/removal/
renaming methods[1] depend on which vfsmount we are dealing with is *wrong*.
IOW, I strongly object against the use of nameidata here.

Re: Adding a security parameter to VFS functions

[Andreas Gruenbacher]

On Friday 17 August 2007 01:34, Al Viro wrote:
> On Thu, Aug 16, 2007 at 03:57:24PM -0700, Linus Torvalds wrote:
> > I personally consider this an affront to everythign that is decent.
> > 
> > Why the *hell* would mkdir() be so magical as to need something like that?
> > 
> > Make it something sane, like a "struct nameidata" instead, and make it at 
> > least try to look like the path creation that is done by "open()".  Or 
> > create a "struct file *" or something.
> 
> No.  I agree that mkdir/mknod/etc. should all take the same thing.
> *However*, it should not be nameidata or file.  Why?  Because it
> should not contain vfsmount.  Object creation should not *care*
> where fs is mounted.  At all. The same goes for open(), BTW - letting it see
> the reference to vfsmount via nameidata is bloody wrong and I really
> couldn't care less what kinds of perversions apparmour crowd might like -
> pathnames simply do not belong there.

At the filesystem layer I mostly agree with you -- ordinary filesystems don't 
have a real business with vfsmounts. At the vfs / lsm layer it's a different 
story though. The vfs is not only about physical object creation alone, but 
also about lookups and security checks (lookup related or not). Consider an 
example:

The same filesystem is bind mounted at /foo/mnt and /bar/mnt. A process tries 
to create /foo/mnt/file or /bar/mnt/file. Depending on the permissions 
of /foo and /bar, the operations may succeed or fail independently. Both 
paths point at the same file, but they may grant different permissions (and 
nobody in their right mind would require them to be equivalent).

The checks we are doing in LSM hooks like security_inode_mknod are pretty 
similar -- we are taking the entire paths to objects into account, and so 
different paths to the same object may result in different permissions. The 
major difference is this:

 * The vfs performs lookups in forward direction, and incrementally. Arbitrary
   amounts of time can pass from when a lookup starts at the root to when it
   reaches a final object. (Think of things like mkdirat, but also the pwd,
   which is basically the same as a directory file handle). The vfs doesn't
   care when directories above the current lookup position become
   inaccessible, renamed, or moved.

 * We need to determine whether an object may be accessed at the time of the
   access, based on the location of the object in the filesystem namespace
   (i.e., dentry and vfsmount). And *this* is why we need the vfsmounts
   in the LSM hooks (and also in the vfs_* functions which are calling the
   hooks) [*].

We are not arguing to pass the vfsmount down the inode operations. At least
some of the LSM hooks are already being passed the vfsmounts, so we are not
even asking for something entirely new and unheard of.

By not letting the LSM hooks know about the vfsmounts you are effectively 
making pathname-based access control impossible. I would expect arguments 
more technically sound than "bloody wrong" and "couldn't care less" for 
killing an entire category of LSMs that lots of people find rather useful.


Thanks,
Andreas


[*] In the current implementation we are computing the entire paths to objects
    with d_path(). We cannot do this incrementally during lookups because
   directories along the path to an object can be renamed or moved around
   any time before the actual access. It doesn't seem hard to add caching so
   that we'll have to do this much less frequently, and maybe we can sometimes
   determine that the pwd hasn't changed and start checking from there, but
   all of this wouldn't change the fundamental model or the slow path / worst
   case.

-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE Linux Products GmbH

Re: Adding a security parameter to VFS functions

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > Would you object greatly to functions like vfs_mkdir() gaining a security
> > parameter?  What I'm thinking of is this:
> ...
> Why the *hell* would mkdir() be so magical as to need something like that?

If you look again, you'll notice that I said "functions like vfs_mkdir()".  I
was I using vfs_mkdir() as an example.  I didn't mean to apply this to
directory creation only.  It would need to apply to all the vfs_*() entry
points called by nfsd and cachefiles.

David