Re: Adding a security parameter to VFS functions
Linus Torvalds <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding a security parameter to VFS functions
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 <[EMAIL PROTECTED]> SUSE Labs, SUSE Linux Products GmbH - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding a security parameter to VFS functions
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 - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding a security parameter to VFS functions
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. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding a security parameter to VFS functions
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 - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding a security parameter to VFS functions
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
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding a security parameter to VFS functions
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 - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding a security parameter to VFS functions
Casey Schaufler <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding a security parameter to VFS functions
--- David Howells <[EMAIL PROTECTED]> 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
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Adding a security parameter to VFS functions
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
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
