Messages in this thread | | | Subject | Re: [TOMOYO #13 (mmotm 2008-11-19-02-19) 01/11] Introduce security_path_clear() hook. | From | Stephen Smalley <> | Date | Tue, 02 Dec 2008 08:48:39 -0500 |
| |
On Tue, 2008-12-02 at 19:39 +0900, Tetsuo Handa wrote: > Hello. > > Stephen Smalley wrote: > > On Thu, 2008-11-20 at 20:25 +0900, Tetsuo Handa wrote: > > > plain text document attachment (introduce-security_path_clear.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 seems very fragile and unmaintainable to me. The fact that you > > even need a security_path_clear() hook suggests that something is wrong > > with the other security_path* hooks. I'd suggest that you explicitly > > pass the result of the security_path* hooks down to the security_inode* > > hooks instead. What do others think? > > You are recommending us to pass variables required for security_inode_*() via > stack memory rather than private hash, aren't you?
To be precise, I was recommending passing the return value of security_path* down to security_inode* explicitly rather than doing it implicitly as you presently do. Thereby making the actual control flow and relationship between the security_path* and security_inode* hooks evident. However, I guess that is moot given your statements below.
> I think there are two problems. > > One is that the variable passed via stack memory won't be used by SELinux and > SMACK and "CONFIG_SECURITY=n kernels", which will be a waste of stack memory.
I'm more concerned with the hook interface being understandable and maintainable.
> The other one is that TOMOYO will need another variable for telling how the > security_inode_*() are called. Passing the variable via stack memory requires > modification of all vfs_*() calls, but TOMOYO doesn't check requests issued > by (e.g.) stackable filesystems.
I'm not clear on why that requires a separate argument; if the caller is passing in the access decision result as an input, then certain callers (e.g. stackable filesystems) can always pass 0 (success).
> By the way, this security_path_clear() is intended to be able to return DAC's > error code in priority to MAC's error code, but there are two problems for > TOMOYO. > One is that pathnames which will be later denied by DAC are appended by > TOMOYO's learning mode (i.e. garbage entries appears in the learned policy). > The other is that warning messages on pathnames which will be later denied by > DAC are generated by TOMOYO's enforcing mode. > > Thus, it will be preferable for TOMOYO to "do MAC checks after DAC checks" > rather than to "return DAC's error in priority to MAC's error while doing MAC > checks before DAC checks".
It sounds like the existing security_path* hooks are not adequate for your needs then, and that patch should not in fact be merged. Yes?
> To do so, "security_path_*() should be replaced by security_path_set(vfsmount)" > and "let security_inode_*() do MAC checks using the result of > security_path_set()" and "let security_path_clear() clear the result of > security_path_set() in case security_inode_*() was not called". > > So, I think storing the pathname of "struct vfsmount" in the form of "char *" > into private hash at security_path_set() and clearing the private hash at > security_path_clear() should be most preferable.
Then I guess you need to redo your patches along those lines and re-submit them. Likely starting with just a patch adding the security_path_set/clear hooks, posted to lsm and fsdevel.
-- Stephen Smalley National Security Agency
| |