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
--- 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]
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
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
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
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
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
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.
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
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