Re: [TOMOYO #11 (linux-next) 01/11] Introduce new LSM hooks where vfsmount is available.

[Kentaro Takeda <takedakn@nttdata.co.jp>]

Miklos Szeredi wrote:
>> (6) Introducing new LSM hooks.
>>  (this patch)
>>
>>  We understand that adding new LSM hooks which receive "struct
>>  vfsmount" outside VFS helper functions is the most straightforward
>>  approach. This approach has less impact to existing LSM module and
>>  no impact to VFS helper functions.
> 
> AppArmor will need a few additional hooks, but the ones added by this
> patch look OK.  One thing I'd prefer is if there were two different
> hooks for truncate and ftruncate:
> 
>    int (*path_truncate) (struct path *path, ...);
> 
> and
> 
>    int (*file_truncate) (struct file *file, ...);
When you submit AppArmor, you can introduce security_file_truncate() 
and replace security_path_truncate() with security_file_truncate() 
since TOMOYO can get "struct path *path" from 
"struct file *file"->f_path .

> security_path_truncate() is missing from do_coredump() in exec.c.  Is
> this intentional?
Yes.
TOMOYO checks only requests from userspace, not from kernel (e.g. 
filesystem). Since do_coredump() performs request from kernel, we 
don't insert security_path_truncate() in do_coredump() .

> Also seems to be missing:
> 
>  - security_path_mknod() from do_create() in ipc/mqueue.c
TOMOYO doesn't check IPC for now.

>  - security_path_mknod() from unix_bind() in net/unix/af_unix.c
There is security_path_mknod() call in unix_bind(). Please see below.
;-)

--- linux-next.orig/net/unix/af_unix.c
+++ linux-next/net/unix/af_unix.c
@@ -828,7 +828,12 @@ static int unix_bind(struct socket *sock
 		err = mnt_want_write(nd.path.mnt);
 		if (err)
 			goto out_mknod_dput;
+		err = security_path_mknod(&nd.path, dentry, mode, 0);
+		if (err)
+			goto out_mknod_drop_write;
 		err = vfs_mknod(nd.path.dentry->d_inode, dentry, mode, 0);
+		security_path_clear();
+out_mknod_drop_write:
 		mnt_drop_write(nd.path.mnt);
 		if (err)
 			goto out_mknod_dput;

>  - security_path_unlink() from sys_mq_unlink() in ipc/mqueue.c
Same reason as security_path_mknod() from do_create() in ipc/mqueue.c .

> The hooks are also not called from nfsd, I presume that's intentional?
Same reason as do_coredump() . Requests from nfsd are not from 
userspace.

Since AppArmor checks both reqests from userspace and kernel, 
AppArmor will need to insert security_path_*() to more locations. 
Then TOMOYO will want a mechanism for dinstinguishing requests from 
userspace and ones from kernel.

>> (6.1) Introducing security_path_clear() hook.
>>  (this patch)
>>
>>  To perform DAC performed in vfs_foo() before MAC, we let
>>  security_path_foo() save a result into our own hash table and
>>  return 0, and let security_inode_foo() return the saved
>>  result. Since security_inode_foo() is not always called after
>>  security_path_foo(), we need security_path_clear() to clear the
>>  hash table.
> 
> This is not a good interface, IMO.  It's easy to forget (e.g. two
> places in open.c), and hard to detect.
> 
> And is it necessary at all?  Saving the result in the per-task
> security context and destroying it at exit should be an equivalent
> solution.
Though current kernel has current->security, CRED patchset by David moves 
security field from current to current->cred . Since current->cred is shared by 
multiple processes, we'll not be able to implement per-task security. Please see 
http://lkml.org/lkml/2008/10/2/7 in detail.

Regards,