Re: [RFC PATCH] selinux: add a fallback to defcontext for native labeling
On 10/02/2018 02:48 PM, Taras Kondratiuk wrote: Quoting Stephen Smalley (2018-09-21 07:40:58) If we set the inode sid to the superblock def_sid on an invalid context, then we lose the association to the original context value. The support for deferred mapping of contexts requires allocating a new SID for the invalid context and storing that SID in the inode, so that if the context later becomes valid upon a policy update/reload, the inode SID will refer to the now valid context. To combine the two, we would need security_context_to_sid_core() to save the def_sid in the context structure for invalid contexts, and change sidtab_search_core() to use that value instead of SECINITSID_UNLABELED for invalid SIDs. Then the inode would be treated as having the defcontext for access control and getxattr() w/o CAP_MAC_ADMIN purposes, but a subsequent policy update/reload that makes the context valid would automatically cause subsequent accesses to the inode to start using the original context value for access control and getxattr() purposes. I think that's the behavior you want. While implementing the change I've realized that storing default context for sidtab_search_core() in the context structure is not enough to achieve the desired behavior. The same invalid context may exist in two mounts with different 'defcontext', so default context can't be a property of a context structure. One way to address it is to propagate default context to sidtab_search() all the way from inode hooks. But that will be a bit intrusive. Something like avc_has_perm_default() will need to be added. Other way is to check for context validity in inode hooks and provide a default context to avc_has_perm() if the inode's sid is invalid. But this may have performance implication since validity check will be done each time in fast path. Do you see any other options? I think the original approach is still best. The def_sid can be part of the context structure; you just need to update context_cmp() to compare it (as part of the first return statement) so that the same invalid context in two mounts with different defcontext= values will allocate different SIDs. Likewise context_cpy() needs to copy the def_sid. As a separate matter, you should also modify security_context_to_sid_force() to pass down the sbsec->def_sid so that when userspace with CAP_MAC_ADMIN sets an invalid context on an inode, the default context will be used in the same manner. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [RFC PATCH] selinux: add a fallback to defcontext for native labeling
Quoting Stephen Smalley (2018-09-21 07:40:58) > If we set the inode sid to the superblock def_sid on an invalid > context, then we lose the association to the original context value. > The support for deferred mapping of contexts requires allocating a new > SID for the invalid context and storing that SID in the inode, so that > if the context later becomes valid upon a policy update/reload, the > inode SID will refer to the now valid context. > To combine the two, we would need security_context_to_sid_core() to > save the def_sid in the context structure for invalid contexts, and > change sidtab_search_core() to use that value instead of > SECINITSID_UNLABELED for invalid SIDs. Then the inode would be > treated as having the defcontext for access control and getxattr() w/o > CAP_MAC_ADMIN purposes, but a subsequent policy update/reload that > makes the context valid would automatically cause subsequent accesses > to the inode to start using the original context value for access > control and getxattr() purposes. I think that's the behavior you > want. > While implementing the change I've realized that storing default context for sidtab_search_core() in the context structure is not enough to achieve the desired behavior. The same invalid context may exist in two mounts with different 'defcontext', so default context can't be a property of a context structure. One way to address it is to propagate default context to sidtab_search() all the way from inode hooks. But that will be a bit intrusive. Something like avc_has_perm_default() will need to be added. Other way is to check for context validity in inode hooks and provide a default context to avc_has_perm() if the inode's sid is invalid. But this may have performance implication since validity check will be done each time in fast path. Do you see any other options? ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: fix race when removing selinuxfs entries
On Tue, Oct 02, 2018 at 01:18:30PM +0200, Ondrej Mosnacek wrote: No. With the side of Hell, No. The bug is real, but this is not the way to fix it. First of all, it's still broken - e.g. mount something on a subdirectory and watch what that thing will do to it. And anyone who has permission to reload policy _will_ have one for mount. And yes, debugfs_remove() suffers the same problem. Frankly, the only wish debugfs_remove() implementation inspires is to shoot it before it breeds. Which is the second problem with that approach - such stuff really shouldn't be duplicated in individual filesystems. Don't copy (let along open-code) d_walk() in there. The guts of dcache tree handling are subtle enough (and had more than enough locking bugs over the years) to make spreading the dependencies on its details all over the tree an invitation for massive PITA down the road. I have beginnings of patch doing that for debugfs; the same thing should be usable for selinuxfs as well. However, the main problem with selinuxfs wrt policy loading is different - what to do if sel_make_policy_nodes() fails halfway through? And what to do with accesses to the unholy mix of old and new nodes we currently have left behind? Before security_load_policy() we don't know which nodes to create; after it we have nothing to fall back onto. It looks like we need to split security_load_policy() into "load", "switch over" and "free old" parts, so that the whole thing would look like load create new directory trees, unconnected to the root so they couldn't be reached by lookups until we are done switch over move the new directory trees in place kill the old trees (using that invalidate+genocide carefully combination) free old data structures with failures upon the first two steps handled by killing whatever detached trees we'd created (if any) and freeing the new data structures. However, I'd really like to have the folks familiar with selinux guts to comment upon the feasibility of the above. AFAICS, nobody has ever seriously looked at that code wrt graceful error handling, etc.[*], so I'm not happy with making inferences from what the existing code is doing. If you are interested in getting selinuxfs into sane shape, that would be a good place to start. As for the kernel-side rm -rf (which is what debugfs_remove() et.al. are trying to be)... * it absolutely needs to be in fs/*.c - either dcache or libfs. It's too closely tied to dcache guts to do otherwise. * as the first step it needs to do d_invalidate(), to get rid of anything that might be mounted on it and to prevent new mounts from appearing. It's rather tempting to unhash everything in the victim tree at the same time, but that needs to be done with care - I'm still not entirely happy with the solution I've got in that area. Alternative is to unhash them on the way out of subtree. simple_unlink()/simple_rmdir() are wrong there - we don't want to bother with the parent's timestamps as we go, for one thing; that should be done only once to parent of the root of that subtree. For another, we bloody well enforce the emptiness ourselves, so this simple_empty() is pointless (especially since we have no choice other than ignoring it anyway). BTW, another selinuxfs unpleasantness is, the things like sel_write_enforce() don't have any exclusion against themselves, let alone the policy reloads. And quite a few of them obviously expect that e.g. permission check is done against the same policy the operation will apply to, not the previous one. That one definitely needs selinux folks involved. [*] not too unreasonably so - anyone who gets to use _that_ as an attack vector has already won, so it's not a security problem pretty much by definition and running into heavy OOM at the time of policy reload is almost certainly going to end up with the userland parts of the entire thing not handling failures gracefully. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 21/19] LSM: Cleanup and fixes from Tetsuo Handa
On Wed, Sep 26, 2018 at 2:57 PM, Casey Schaufler wrote: > lsm_early_cred()/lsm_early_task() are called from only __init functions. > > lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c . > > lsm_early_inode() should be avoided because it is not appropriate to > call panic() when lsm_early_inode() is called after __init phase. > > Since all free hooks are called when one of init hooks failed, each > free hook needs to check whether init hook was called. > > The original changes are from Tetsuo Handa. I have made minor > changes in some places, but this is mostly his code. > > Signed-off-by: Casey Schaufler > --- > include/linux/lsm_hooks.h | 6 ++ > security/security.c | 27 --- > security/selinux/hooks.c | 5 - > security/selinux/include/objsec.h | 2 ++ > security/smack/smack_lsm.c| 8 +++- > 5 files changed, 19 insertions(+), 29 deletions(-) I've split this across the various commits they touch: Infrastructure management of the cred security blob LSM: Infrastructure management of the file security LSM: Infrastructure management of the inode security LSM: Infrastructure management of the task security LSM: Blob sharing support for S.A.R.A and LandLock Based on these changes, I've uploaded the "v4.1", or "Casey is on vacation", tree here: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=lsm/blob-sharing-v4.1 I'm going to work on a merged series for the "arbitrary ordering" and "blob-sharing" trees next... -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH security-next v3 07/29] LSM: Convert security_initcall() into DEFINE_LSM()
On 09/24/2018 05:18 PM, Kees Cook wrote:
> Instead of using argument-based initializers, switch to defining the
> contents of struct lsm_info on a per-LSM basis. This also drops
> the final use of the now inaccurate "initcall" naming.
>
> Cc: John Johansen
> Cc: James Morris
> Cc: "Serge E. Hallyn"
> Cc: Paul Moore
> Cc: Stephen Smalley
> Cc: Casey Schaufler
> Cc: Tetsuo Handa
> Cc: Mimi Zohar
> Cc: linux-security-mod...@vger.kernel.org
> Cc: selinux@tycho.nsa.gov
> Signed-off-by: Kees Cook
> ---
> include/linux/lsm_hooks.h | 6 --
> security/apparmor/lsm.c| 4 +++-
> security/integrity/iint.c | 4 +++-
> security/selinux/hooks.c | 4 +++-
> security/smack/smack_lsm.c | 4 +++-
> security/tomoyo/tomoyo.c | 4 +++-
> 6 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index ad04761e5587..02ec717189f9 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2045,11 +2045,13 @@ struct lsm_info {
>
> extern struct lsm_info __start_lsm_info[], __end_lsm_info[];
>
> -#define security_initcall(lsm)
> \
> +#define DEFINE_LSM(lsm)
> \
> static struct lsm_info __lsm_##lsm \
> __used __section(.lsm_info.init)\
> __aligned(sizeof(unsigned long))\
> - = { .init = lsm, }
> + = { \
> +
> +#define END_LSM}
>
I am with Tetsuo on this one, I really don't like the END_LSM thing.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 20/19] LSM: Correct file blob free empty blob check
On Wed, Sep 26, 2018 at 2:57 PM, Casey Schaufler wrote:
> Instead of checking if the kmem_cache for file blobs
> has been initialized check if the blob is NULL. This
> allows non-blob using modules to do other kinds of
> clean up in the security_file_free hooks.
>
> Signed-off-by: Casey Schaufler
Reviewed-by: Kees Cook
This looks like it should get folded into "LSM: Infrastructure
management of the file security".
-Kees
> ---
> security/security.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index e7c8506041f1..76f7dc49b63c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1202,14 +1202,13 @@ void security_file_free(struct file *file)
> {
> void *blob;
>
> - if (!lsm_file_cache)
> - return;
> -
> call_void_hook(file_free_security, file);
>
> blob = file->f_security;
> - file->f_security = NULL;
> - kmem_cache_free(lsm_file_cache, blob);
> + if (blob) {
> + file->f_security = NULL;
> + kmem_cache_free(lsm_file_cache, blob);
> + }
> }
>
> int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long
> arg)
> --
> 2.17.1
>
>
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[PATCH] selinux: fix race when removing selinuxfs entries
Letting the following set of commands run long enough on a multi-core
machine causes soft lockups in the kernel:
(cd /sys/fs/selinux/; while true; do find >/dev/null 2>&1; done) &
(cd /sys/fs/selinux/; while true; do find >/dev/null 2>&1; done) &
(cd /sys/fs/selinux/; while true; do find >/dev/null 2>&1; done) &
while true; do load_policy; echo -n .; sleep 0.1; done
The problem is that sel_remove_entries() calls d_genocide() to remove
the whole contents of certain subdirectories in /sys/fs/selinux/. This
function is apparently only intended for removing filesystem entries
that are no longer accessible to userspace, because it doesn't follow
the rule that any code removing entries from a directory must hold the
write lock on the directory's inode RW semaphore (formerly this was a
mutex, see 9902af79c01a ("parallel lookups: actual switch to rwsem")).
In order to fix this, I had to open-code d_genocide() again, adding the
necessary parent inode locking. I based the new implementation on
d_walk() (fs/dcache.c), the original code from before ad52184b705c
("selinuxfs: don't open-code d_genocide()"), and with a slight
inspiration from debugfs_remove() (fs/debugfs/inode.c).
The new code no longer triggers soft lockups with the above reproducer
and it also gives no warnings when run with CONFIG_PROVE_LOCKING=y.
Note that I am adding Fixes: on the commit that replaced the open-coded
version with d_genocide(), but the bug is present also in the original
code that dates back to pre-2.6 times... This patch obviously doesn't
apply to this older code so pre-4.0 kernels will need a dedicated patch
(just adding the parent inode locks should be sufficient there).
Link: https://github.com/SELinuxProject/selinux-kernel/issues/42
Fixes: ad52184b705c ("selinuxfs: don't open-code d_genocide()")
Cc: # 4.0+
Cc: Stephen Smalley
Cc: Al Viro
Signed-off-by: Ondrej Mosnacek
---
security/selinux/selinuxfs.c | 88 ++--
1 file changed, 85 insertions(+), 3 deletions(-)
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index f3a5a138a096..4ac9b17e24d1 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1316,10 +1316,92 @@ static const struct file_operations
sel_commit_bools_ops = {
.llseek = generic_file_llseek,
};
-static void sel_remove_entries(struct dentry *de)
+/* removes all children of the given dentry (but not itself) */
+static void sel_remove_entries(struct dentry *root)
{
- d_genocide(de);
- shrink_dcache_parent(de);
+ struct dentry *parent = root;
+ struct dentry *child;
+ struct list_head *next;
+
+ spin_lock(&parent->d_lock);
+
+repeat:
+ next = parent->d_subdirs.next;
+
+ while (next != &parent->d_subdirs) {
+ child = list_entry(next, struct dentry, d_child);
+ next = next->next;
+
+ spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
+
+ if (!list_empty(&child->d_subdirs)) {
+ /* Current entry has children, we need to remove those
+* first. We unlock the parent but keep the child locked
+* (it will become the new parent). */
+ spin_unlock(&parent->d_lock);
+
+ /* we need to re-lock the child (new parent) in a
+* special way (see d_walk() in fs/dcache.c) */
+ spin_release(&child->d_lock.dep_map, 1, _RET_IP_);
+ parent = child;
+ spin_acquire(&parent->d_lock.dep_map, 0, 1, _RET_IP_);
+
+ goto repeat;
+ }
+
+resume:
+ if (child->d_inode) {
+ /* acquire a reference to the child */
+ dget_dlock(child);
+
+ /* drop all locks (someof the callees will try to
+* acquire them) */
+ spin_unlock(&child->d_lock);
+ spin_unlock(&parent->d_lock);
+
+ /* IMPORTANT: we need to hold the parent's inode lock
+* because some functions (namely dcache_readdir) assume
+* holding this lock means children won't be removed */
+ inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
+
+ /* now unlink the child */
+ if (d_is_dir(child))
+ simple_rmdir(d_inode(parent), child);
+ else
+ simple_unlink(d_inode(parent), child);
+ d_delete(child);
+
+ inode_unlock(parent->d_inode);
+
+ /* drop our extra reference */
+ dput(child);
+
+ /* re-lock only the parent */
+ spin_lock(&parent->d_lock);
+
