Re: [TOMOYO #13 (mmotm 2008-11-19-02-19) 01/11] Introduce security_path_clear() hook.

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

Stephen Smalley wrote:
> 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 so too.

>> 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.
I see.

>> 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).
If we use stack memory to pass the access decision result from security_path_*() 
to security_inode_*(), this method seems possible.

>> 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?
Sorry for confusing you. security_path_*() hooks are adequate for TOMOYO 
functionality itself. But they are inadequate for performing DAC before MAC, 
which we eventually want to do. We've reached this "passing vfsmount's pathname" 
approach after proposing the previous patch. The new approach is a little 
divergence of the existing approach, which Serge has patiently advised us. 
It should be more suitable for DAC-before-MAC than the previous patch, we think.

>> 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.
I'll post it soon.

If we pass the access decision result through stack memory, we don't need 
security_path_clear() as you mentioned. However, if we pass the vfsmount's 
pathname, security_path_clear() is still needed in order to free the pathname.

Regards,