linux-kernel - Re: [TOMOYO #13 (mmotm 2008-11-19-02-19) 01/11] Introduce security_path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 02 Dec 2008 08:48:39 -0500
From:	Stephen Smalley <sds@...ho.nsa.gov>
To:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:	akpm@...ux-foundation.org, linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org, viro@...iv.linux.org.uk, hch@....de,
	crispin@...spincowan.com, casey@...aufler-ca.com,
	jmorris@...ei.org, takedakn@...data.co.jp, haradats@...data.co.jp
Subject: Re: [TOMOYO #13 (mmotm 2008-11-19-02-19) 01/11] Introduce
	security_path_clear() hook.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/