Re: [PATCH 1/3] Make cap default

[Kurt Garloff <garloff@suse.de>]

Hi Tony,

On Sun, Jul 03, 2005 at 03:51:52PM -0700, Tony Jones wrote:
> On Sun, Jul 03, 2005 at 05:43:33PM +0200, Kurt Garloff wrote:
> 
> > Note that we could think of getting rid of dummy; however, it's
> > still used as fallback for stubs that are not implemented by an
> > LSM. I did not want to change this with this patch set, though
> > I'd like to see it done if everyone agrees it's a good idea.
> 
> I think this needs to happen (dummy goes away or it's contents are replaced
> by cap routines as appropriate).

I agree, but it's not trivial, as we need to review existing LSMs:
For whatever security_ops function that they don't implement and
where dummy and capability differ, we'll need to fix up.

Before someone goes there, we should agree that it's a good idea.

Currently, capability has only a small subset of functions implemented
and still gets the other ones patched in from dummy through verify()/
security_fixup_ops().

> Currently in security_init() verify(&dummy_security_ops) calling 
> security_fixup_ops is a no-op in terms of modifying dummy. In your patch as
> you suggest verify(&capability_security_ops) now patches routines from dummy 
> into this default capability_security_ops.  The code is already too baroque,
> no point making it more complex.

The alternative would be making capability_security_ops complete.
The patching could be avoided completely by using something like
this in security.h:
	if (security_ops->OPERATION == NULL)
		return cap_OPERTAION(x,y,z)

Or, even better, after patches 2a, 2b, 3 have been applied:

#  define COND_SECURITY(seop, def)			\
	(security_opt->seop == NULL) ||			\
	 security_ops == &capability_security_ops)?	\
	 def: security_ops->seop

The latter has the very nice side effect that we DON'T need to code
all the {return 0;}; and {}; functions into capability.c, but rather
use the return values we know already, because they are part of
security.h anyway for the !defined(CONFIG_SECURITY) case.

The more I think about it, the better I like that approach.
It will also be a nice performance improvement again:
Instead of unconditionally calling a function pointer (which
includes passing all the argument setup, cleanup, ...), we just
have one branch (that will be correctly predicted after the second
occurence) and know the return value immediately. Nice.
Allowing such things was one of my motivations of cleaning up
security.h.

But before, we really need to make sure all LSMs work with 
security_fixup_ops() patching in capability ops and not dummy ops.

> I think the necessary functions need to be present in this new base structure
> and security_fixup_ops patches from it into whatever new ops a caller passes, 
> dummy goes away. As a bonus, at this point your capability_ops struct in
> capability.c can really be different :-)

I don't consider this a bonus. Loading capability should not change
the behaviour IMVHO.[*]
The fact that the capability LSM may still be compiled, despite being
pointless (it's the default now), is that you may load it as secondary
LSM. I clarified this in the comment.

[*] Well, we could consider enhancing capability in the future and
leaving the old behaviour default for both !defined(CONFIG_SECURITY)
and the no-LSM-loaded case and have slightly enhanced semantics by
loading capability. I would prefer renaming the LSM then however.

> --- linux-2.6.10.orig/security/commoncap.c
> +++ linux-2.6.10/security/commoncap.c
> [snip]
> > +EXPORT_SYMBOL(capability_security_ops);
> > +/* Note: If the capability security module is loaded, we do NOT register
> > + * the capability_security_ops but a second structure that has the
> > + * identical entries. The reason is that this way,
> > + * - we could stack on top of capability if it was stackable
> > + * - a loaded capability module will prevent others to register, which
> > + *   is the previous behaviour; if capabilities are used as default (not
> > + *   because the module has been loaded), we allow the replacement.
> > + */
> > +#endif
> 
> The intent here is to make it past the 
> 	if (security_ops != &capability_security_ops)
> check in security.c::register_security so that it is possible to actually
> register capability (capability_ops) subsequently as a module should you
> so desire, no?

Correct.

> The above description doesn't impart that.  Plus why would you want to stack
> capability on top of this base capability, even if it it supported stacking?

I could have disabled the creation of the capability LSM completely, but
who knows whether or not capability could be useful as secondary LSM
(or as part of some stack using stacker)? I can't exclude that ...

I have improved the comment, please review attached patch.

> --- linux-2.6.10.orig/security/capability.c
> +++ linux-2.6.10/security/capability.c
> 
> +/* Note: If the capability security module is loaded, we do NOT register
> + * the capability_security_ops but a second structure capability_ops
> + * that has the identical entries. The reasons:
> + * - we could stack on top of capability if it was stackable
> + * - a loaded capability module will prevent others to register, which
> + *   is the previous behaviour; if capabilities are used as default (not
> + *   because the module has been loaded), we allow the replacement.
> 
> Ditto for this comment.

Yep, changed as well.

> 
> >  static int __init capability_init (void)
> >  {
> > +	memcpy(&capability_ops, &capability_security_ops, sizeof(capability_ops));
> >  	if (capability_disable) {
> >  		printk(KERN_INFO "Capabilities disabled at initialization\n");
> >  		return 0;
> >  	}
> 
> No point doing the memcpy if capability_disable is true.

We could move it below the check.
I've done so, please review attached patch.
(Sidenote: Patch 3 will have a trivial reject after the changes here.)

Thanks a lot for your comments!
-- 
Kurt Garloff, Director SUSE Labs, Novell Inc.

From: Kurt Garloff <garloff@suse.de>
Subject: Default to capability rather than dummy if no LSM is loaded
References: SUSE40217, SUSE39439

If a kernel is compiled with CONFIG_SECURITY to enable LSM, the
default behaviour changes unless the admin loads capability.
This is undesirable. This patch makes capability the default.
capability can still be compiled as module and be loaded as LSM.
If loaded as primary LSM, it won't change anything. But it may
also be loaded as secondary LSM and stacked on top of another
LSM (if the other LSM allows this or if stacker is used).

Note that we could think of getting rid of dummy; however, it's
still used as fallback for stubs that are not implemented by an
LSM. I did not want to change this with this patch set, though
I'd like to see it done as a subsequent step if everyone agrees 
it's a good idea. We'd need to review existing LSMs and see what
changes they might need.

This is patch 1/3 of the LSM overhaul.

Signed-off-by: Kurt Garloff <garloff@suse.de>
 Makefile     |    8 +++-----
 capability.c |   47 +++++++++++++++++++++++------------------------
 commoncap.c  |   37 +++++++++++++++++++++++++++++++++++++
 security.c   |   21 ++++++++++++---------
 4 files changed, 75 insertions(+), 38 deletions(-)

Index: linux-2.6.12/security/security.c
===================================================================
--- linux-2.6.12.orig/security/security.c
+++ linux-2.6.12/security/security.c
@@ -21,8 +21,9 @@
 #define SECURITY_FRAMEWORK_VERSION	"1.0.0"
 
 /* things that live in dummy.c */
-extern struct security_operations dummy_security_ops;
 extern void security_fixup_ops(struct security_operations *ops);
+/* default security ops */
+extern struct security_operations capability_security_ops;
 
 struct security_operations *security_ops;	/* Initialized to NULL */
 
@@ -55,13 +56,15 @@ int __init security_init(void)
 	printk(KERN_INFO "Security Framework v" SECURITY_FRAMEWORK_VERSION
 	       " initialized\n");
 
-	if (verify(&dummy_security_ops)) {
+	if (verify(&capability_security_ops)) {
 		printk(KERN_ERR "%s could not verify "
-		       "dummy_security_ops structure.\n", __FUNCTION__);
+		       "capability_security_ops structure.\n", __FUNCTION__);
 		return -EIO;
 	}
 
-	security_ops = &dummy_security_ops;
+	security_ops = &capability_security_ops;
+
+	/* Initialize compiled-in security modules */
 	do_security_initcalls();
 
 	return 0;
@@ -87,7 +90,7 @@ int register_security(struct security_op
 		return -EINVAL;
 	}
 
-	if (security_ops != &dummy_security_ops)
+	if (security_ops != &capability_security_ops)
 		return -EAGAIN;
 
 	security_ops = ops;
@@ -104,18 +107,18 @@ int register_security(struct security_op
  *
  * If @ops does not match the valued previously passed to register_security()
  * an error is returned.  Otherwise the default security options is set to the
- * the dummy_security_ops structure, and 0 is returned.
+ * the capability_security_ops structure, and 0 is returned.
  */
 int unregister_security(struct security_operations *ops)
 {
 	if (ops != security_ops) {
 		printk(KERN_INFO "%s: trying to unregister "
-		       "a security_opts structure that is not "
+		       "a security_ops structure that is not "
 		       "registered, failing.\n", __FUNCTION__);
 		return -EINVAL;
 	}
 
-	security_ops = &dummy_security_ops;
+	security_ops = &capability_security_ops;
 
 	return 0;
 }
Index: linux-2.6.12/security/commoncap.c
===================================================================
--- linux-2.6.12.orig/security/commoncap.c
+++ linux-2.6.12/security/commoncap.c
@@ -325,6 +325,41 @@ int cap_vm_enough_memory(long pages)
 	return __vm_enough_memory(pages, cap_sys_admin);
 }
 
+#ifdef CONFIG_SECURITY
+struct security_operations capability_security_ops = {
+	.ptrace =			cap_ptrace,
+	.capget =			cap_capget,
+	.capset_check =			cap_capset_check,
+	.capset_set =			cap_capset_set,
+	.capable =			cap_capable,
+	.settime =			cap_settime,
+	.netlink_send =			cap_netlink_send,
+	.netlink_recv =			cap_netlink_recv,
+
+	.bprm_apply_creds =		cap_bprm_apply_creds,
+	.bprm_set_security =		cap_bprm_set_security,
+	.bprm_secureexec =		cap_bprm_secureexec,
+
+	.inode_setxattr =		cap_inode_setxattr,
+	.inode_removexattr =		cap_inode_removexattr,
+
+	.task_post_setuid =		cap_task_post_setuid,
+	.task_reparent_to_init =	cap_task_reparent_to_init,
+
+	.syslog =                       cap_syslog,
+
+	.vm_enough_memory =             cap_vm_enough_memory,
+};
+
+EXPORT_SYMBOL(capability_security_ops);
+/* Note: capability_security_ops is the default for security_ops
+ * now which gets used if no LSM is loaded.
+ * If capability is loaded, a copy of capability_security_ops
+ * is registered, so we know whether or not we use a non-default
+ * security_ops. If we don't, replacement is not possible.
+ */
+#endif
+
 EXPORT_SYMBOL(cap_capable);
 EXPORT_SYMBOL(cap_settime);
 EXPORT_SYMBOL(cap_ptrace);
Index: linux-2.6.12/security/Makefile
===================================================================
--- linux-2.6.12.orig/security/Makefile
+++ linux-2.6.12/security/Makefile
@@ -5,15 +5,13 @@
 obj-$(CONFIG_KEYS)			+= keys/
 subdir-$(CONFIG_SECURITY_SELINUX)	+= selinux
 
-# if we don't select a security model, use the default capabilities
-ifneq ($(CONFIG_SECURITY),y)
+# We always need commoncap as it's default
 obj-y		+= commoncap.o
-endif
 
 # Object file lists
 obj-$(CONFIG_SECURITY)			+= security.o dummy.o
 # Must precede capability.o in order to stack properly.
 obj-$(CONFIG_SECURITY_SELINUX)		+= selinux/built-in.o
-obj-$(CONFIG_SECURITY_CAPABILITIES)	+= commoncap.o capability.o
-obj-$(CONFIG_SECURITY_ROOTPLUG)		+= commoncap.o root_plug.o
+obj-$(CONFIG_SECURITY_CAPABILITIES)	+= capability.o
+obj-$(CONFIG_SECURITY_ROOTPLUG)		+= root_plug.o
 obj-$(CONFIG_SECURITY_SECLVL)		+= seclvl.o
Index: linux-2.6.12/security/capability.c
===================================================================
--- linux-2.6.12.orig/security/capability.c
+++ linux-2.6.12/security/capability.c
@@ -24,30 +24,28 @@
 #include <linux/ptrace.h>
 #include <linux/moduleparam.h>
 
-static struct security_operations capability_ops = {
-	.ptrace =			cap_ptrace,
-	.capget =			cap_capget,
-	.capset_check =			cap_capset_check,
-	.capset_set =			cap_capset_set,
-	.capable =			cap_capable,
-	.settime =			cap_settime,
-	.netlink_send =			cap_netlink_send,
-	.netlink_recv =			cap_netlink_recv,
-
-	.bprm_apply_creds =		cap_bprm_apply_creds,
-	.bprm_set_security =		cap_bprm_set_security,
-	.bprm_secureexec =		cap_bprm_secureexec,
-
-	.inode_setxattr =		cap_inode_setxattr,
-	.inode_removexattr =		cap_inode_removexattr,
-
-	.task_post_setuid =		cap_task_post_setuid,
-	.task_reparent_to_init =	cap_task_reparent_to_init,
-
-	.syslog =                       cap_syslog,
-
-	.vm_enough_memory =             cap_vm_enough_memory,
-};
+/* Note: Capabilities are default now, even if CONFIG_SECURITY
+ * is enabled and no LSM is loaded. (Previously, the dummy
+ * functions would have been called in that case which resulted
+ * in a slightly unusable system.)
+ * The capability LSM may still be compiled and loaded; it won't
+ * make a difference though except for slowing down some operations
+ * a tiny bit and (more severly) for disallowing loading another LSM.
+ * To have it as LSM may still be useful: It could be stacked on top
+ * of another LSM (if the other LSM allows this or if the stacker
+ * is used).
+ * If the capability LSM is loaded, we do NOT register the 
+ * capability_security_ops but a second structure capability_ops
+ * that has identical entries. We need to differentiate
+ * between capabilities used as default and used as LSM as in
+ * the latter case replacing it by just loading another LSM is
+ * not possible.
+ */
+
+/* Struct from commoncaps */
+extern struct security_operations capability_security_ops;
+/* Struct to hold the copy */
+static struct security_operations capability_ops;
 
 #define MY_NAME __stringify(KBUILD_MODNAME)
 
@@ -64,6 +62,7 @@ static int __init capability_init (void)
 		printk(KERN_INFO "Capabilities disabled at initialization\n");
 		return 0;
 	}
+	memcpy(&capability_ops, &capability_security_ops, sizeof(capability_ops));
 	/* register ourselves with the security framework */
 	if (register_security (&capability_ops)) {
 		/* try registering with primary module */

Attachment: pgpYjZHuRKgv4.pgp
Description: PGP signature