Signed-off-by: Kentaro Takeda <takedakn@nttdata.co.jp>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Toshiharu Harada <haradats@nttdata.co.jp>
---
 security/tomoyo/tomoyo.c |  341 +++++++++++++++++++++++++++++++++++++++++++++++
 security/tomoyo/tomoyo.h |   97 +++++++++++++
 2 files changed, 438 insertions(+)

--- /dev/null
+++ linux-2.6.27-rc7-mm1/security/tomoyo/tomoyo.c
@@ -0,0 +1,341 @@
+/*
+ * security/tomoyo/tomoyo.c
+ *
+ * LSM hooks for TOMOYO Linux.
+ *
+ * Copyright (C) 2005-2008  NTT DATA CORPORATION
+ *
+ * Version: 2.2.0-pre   2008/09/24
+ *
+ */
+
+#include <linux/security.h>
+#include "common.h"
+#include "tomoyo.h"
+#include "realpath.h"
+#include <linux/audit.h>
+#include <linux/device_cgroup.h>
+
+static int tmy_cred_prepare(struct cred *new, const struct cred *old, gfp_t gfp)
+{
+	new->security = old->security;
+	return 0;
+}
+
+static int tmy_bprm_check_security(struct linux_binprm *bprm)
+{
+	struct domain_info *next_domain = NULL;
+	int retval;
+	/*
+	 * If called by do_execve() (i.e. bprm->sh_bang == 0),
+	 * I do execute permission check.
+	 */
+	if (bprm->sh_bang)
+		return 0;
+	tmy_load_policy(bprm->filename);
+	retval = tmy_find_next_domain(bprm, &next_domain);
+	if (!retval)
+		bprm->cred->security = next_domain;
+	return retval;
+}
+
+static int tmy_sysctl(struct ctl_table *table, int op)
+{
+	int error;
+	char *name;
+
+	if ((op & 6) == 0)
+		return 0;
+
+	name = sysctlpath_from_table(table);
+	if (!name)
+		return -ENOMEM;
+
+	error = tmy_check_file_perm(name, op & 6, "sysctl");
+	tmy_free(name);
+
+	return error;
+}
+
+/* Copied from fs/namei.c */
+static inline int may_create(struct inode *dir, struct dentry *child)
+{
+	if (child->d_inode)
+		return -EEXIST;
+	if (IS_DEADDIR(dir))
+		return -ENOENT;
+	return inode_permission(dir, MAY_WRITE | MAY_EXEC);
+}
+
+/* Copied from fs/namei.c */
+static inline int check_sticky(struct inode *dir, struct inode ...
[ message continues ]

So IMO there is some major badness here in the form of copying all of
those functions out of fs/namei.c.  I think we need to discuss
case-by-case whether using the functions is appropriate (and hence
they should be made non-static in fs/namei.c), or whether the intended





likewise...  (except in the case of fifo/sock, devcgroup should not be
consulted as I'm not sure it'll handle that properly - have you tested

-serge
--

[ message continues ]

Indeed.

To perform DAC before MAC, cloning DAC code (like this patch) or some
modifications against existing kernel code (such as non-static
may_open()) are needed.

This posting is the result of our intention that all changes should
be within security/* . We are now aiming TOMOYO Linux to be merged
without messing up the existing kernel code. (Also we put the code of
singly linked list TOMOYO Linux uses not in include/linux/list.h but
in security/tomoyo/common.h to avoid recomplilation.)

We are ready to remove DAC code in TOMOYO Linux LSM module for now.
(But DAC should be performed before MAC, it's our future work.) Since
DAC is performed after security_path_*() hooks, this approach has no
Not tested yet... But I don't think problem occurs.

Regards,

--

[ message continues ]

I see.  Good point.

Unfortunately I think that is a shortcoming in the security_path_*
patchset.  Unfortunate bc that is going to be a pain to work out.

But I do think it needs to be worked out in the core code, not in
Tomoyo (and each lsm using security_path_*).  So for starters,
both vfs_mknod and vfs_create do may_create, so just pull that
into the callers.  Now Al or Christoph may yell NO due to the
intended layering (which i'm not clear on), in which case the
--

[ message continues ]

Do you mean that we should move DAC code to all the caller of vfs_* ? 
If we move DAC code to the caller of vfs_*(), we need not to 
introduce seucrity_path_*() because we can move security_inode_*() 
together. Furthermore, each filesystem must perform DAC by itself. It 
There are two approaches to perform DAC before MAC using 
security_path_*(). One is cloning DAC functions in 
security/security.c . The other is modifying fs/namei.c to make DAC 
functions visible to security/security.c . Which approach is 
preferable?

The attached patch is an implementation of the former approach. If 
CONFIG_SECURITY_PATH is not defined, cloned DAC functions will not be 
compiled.

Regards,

---
Subject: vfs: introduce new LSM hooks where vfsmount is available.

----- What is this patch for? -----

There are security_inode_*() LSM hooks for attribute-based MAC, but they are not
suitable for pathname-based MAC because they don't receive "struct vfsmount"
information.

----- How this patch was developed? -----

Two pathname-based MACs, AppArmor and TOMOYO Linux, are trying to merge
upstream. But because of "struct vfsmount" problem, they have been unable to
merge upstream.

Here are the list of approaches and the reasons of denial.

(1) Not using LSM
 http://lwn.net/Articles/277833/

 This approach was rejected because security modules should use LSM because the
 whole idea behind LSM was to have a single set of hooks for all security
 modules; if every module now adds its own set of hooks, that purpose will have
 been defeated and the kernel will turn into a big mess of security hooks.

(2) Retrieving "struct vfsmount" from "struct task_struct".
 http://lkml.org/lkml/2007/11/5/388

 Since "struct task_struct" contains list of "struct vfsmount",
 "struct vfsmount" which corresponds to "struct dentry" can be retrieved from
 the list unless "mount --bind" is used.

 This approach turned out to cause a critical problem that getting namespace_sem
 lock from security_inode_*() ...
[ message continues ]

That's not reasonable, is it.

The rule thus far has been 'DAC before MAC'.  Question to all:  do we
insist on keeping it that way?

If the answer is yes, then the security_path_hooks patch is inherently
wrong.

If the answer is no, then Kentaro doesn't need to resort to this
ugliness to try and get may_delete() called before his MAC code, only to
have may_delete() called a second time from the vfs_* functions.

--

[ message continues ]

It isn't a hard rule; there are already some hooks that occur before the
DAC checking, e.g. setattr, because the DAC checking happens in the fs
code as part of the inode op.  But when possible, we prefer DAC before
MAC for SELinux so that we don't get noise in the audit logs from
harmless application/library probing that would be denied by DAC anyway.
Same issue would seemingly apply for learning modes of TOMOYO or

-- 
Stephen Smalley
National Security Agency

--

[ message continues ]

Since SELinux won't be using the security_path hooks, it won't be
affected by this, though, right?

Though if we start down the path of mixing dac+mac with _path hooks then

--

[ message continues ]

Indeed, since learning mode of TOMOYO is inherently the same feature 
as auditing, it is affected by noise. But it doesn't matter so much. 
The main problem is the difference of error code between DAC and MAC. 
DAC errors such as ENOENT or EROFS should be returned to callers 
first so that callers will not be surprised by MAC errors such as 
EPERM.

Regards,

--

[ message continues ]

I have always believed that MAC should come first, then DAC, because
MAC may care if you can see the mode bits. The current DAC before MAC
is an artifact of the desire for the LSM to behave cleanly as a
strictly additional mechanism. From an ideal security perspective
MAC should be first, but the pragmatic DAC first isn't going to cause
too much grief. If Tomoyo wants to do what I think is the right thing,
well, it's OK with me.



--

[ message continues ]

I'm OK with the MAC going first as well - but unless/until we convert the
rest of the kernel to do MAC-before-DAC, somebody better leave a comment:

/* Yes, this one spot *is* doing MAC-first intentionally */

or similar, just so we don't keep getting patches to "fix" it to DAC-first...

(And yes, newbie janitors *will* submit patches like that - how many times
have we had the 'ndiswrapper-taint-flag' flame war now?)
[ message continues ]

Current implementation is as follows.
- security_path_*: MAC before DAC
- security_inode_*: DAC before MAC
I can understand Casey and Valdis' MAC first approach from the ideal 
security perspective. However, from the pragmatic perspective, we 
prefer DAC before MAC approach as SELinux does. This approach doesn't 
change error code returned to callers if requested access is denied 
by DAC.

Regards,

--

[ message continues ]

I suppose you could do something like define both _path and _inode,
save away your result from the _path hook but always return 0, there,
then if you'd saved off an error and you make it to the _inode hook,
return the error there...

-serge
--

[ message continues ]

You mean do MAC checks in security_path_*() and return error code of 
security_path_*() in security_inode_*()? Then, method for passing the 
error code to security_inode_*() is a problem.

It was possible to store the error code into current->security->
something. But now, it is impossible to store the error code into 
current->cred->security->something because current->cred is shared by 
multiple processes. To solve this problem, we everytime need to copy 
current->cred in security_path_*() and we need a new hook called just 
after returning from vfs_* (like mnt_drop_write()) for clearing the 
error code. 

Or, another way is to pass the error code as a vfs_*() parameter.

What do you think these approaches?

Regards,

--

[ message continues ]

Just keep your own hash table.

-serge
--

[ message continues ]

I see, then we want one more LSM hook for clearing the hash table 
after returing from vfs_*().

foo() {
	error = security_path_foo(); /* save result in the hash table */
	error = vfs_foo(); /* fetch from the hash table in security_inode_*() */
	security_path_clear(); /* clear the hash table */
}

Is it acceptable?

Regards,


--

[ message continues ]

Why can't you just clear the value during security_inode_foo()?

Note I'm seeing this as a way for Tomoyo to temporarily (maybe) work
around the mis-placement of the security_path_foo() hooks.  I don't want
to add security_path_clear() hooks to "legitimize" the workaround.  I'd
rather Tomoyo and Apparmor folks keep looking for a better way to get
--

[ message continues ]

We need a new hook for clearing the value since security_inode_*() 
Hmm, I can understand your opinion. The best way for AppArmor and 
TOMOYO is to pass vfsmount to vfs_*() and security_inode_*() . This 
approach has no DAC-before-MAC problem. However, it is clearly 
opposed by Al because of layering. So, we are going forward 
security_path_*() approach, which Al advised us.

Since vfsmount is only available outside vfs_*() (and vfs_*() perform 
DAC), we cannot conceive another place now... Where do you think the 
right place to introduce security_path_*() hooks is?

Regards,

--

[ message continues ]

Heh, obviously you're right :)

So I'd recommend floating your security_path_clear() patch with a clear
description about the DAC-before-MAC property which you are maintaining.
Someone may come up with a better overall solution, but we're unlikely
to hear it until you try to push your patch.

--

[ message continues ]

Serge, thank you for your patient advisement. :)
Here is the patch with all changes against LSM interface.

Al, is this patch acceptable?

---
----- What is this patch for? -----

There are security_inode_*() LSM hooks for attribute-based MAC, but they are not
suitable for pathname-based MAC because they don't receive "struct vfsmount"
information.

----- How this patch was developed? -----

Two pathname-based MACs, AppArmor and TOMOYO Linux, are trying to merge
upstream. But because of "struct vfsmount" problem, they have been unable to
merge upstream.

Here are the list of approaches and the reasons of denial.

(1) Not using LSM
 http://lwn.net/Articles/277833/

 This approach was rejected because security modules should use LSM because the
 whole idea behind LSM was to have a single set of hooks for all security
 modules; if every module now adds its own set of hooks, that purpose will have
 been defeated and the kernel will turn into a big mess of security hooks.

(2) Retrieving "struct vfsmount" from "struct task_struct".
 http://lkml.org/lkml/2007/11/5/388

 Since "struct task_struct" contains list of "struct vfsmount",
 "struct vfsmount" which corresponds to "struct dentry" can be retrieved from
 the list unless "mount --bind" is used.

 This approach turned out to cause a critical problem that getting namespace_sem
 lock from security_inode_*() triggers AB-BA deadlock.

(3) Adding "struct vfsmount" parameter to VFS helper functions.
 http://lkml.org/lkml/2008/5/29/207

 This approach adds "struct vfsmount" to VFS helper functions (e.g. vfs_mkdir()
 and vfs_symlink()) and LSM hooks inside VFS helper functions. This approach is
 helpful for not only AppArmor and TOMOYO Linux 2.x but also SELinux and
 auditing purpose, for this approach allows existent LSM users to use pathnames
 in their access control and audit logs.

 This approach was rejected by Al Viro, the VFS maintainer, because he thinks
 individual filesystem should remain "struct ...
[ message continues ]