Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
From: Kees Cook
Date: Thu Sep 13 2018 - 00:19:29 EST
On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> Two proposed security modules require the ability to
> share security blobs with existing "major" security modules.
> These modules, S.A.R.A and LandLock, provide significantly
> different services than SELinux, Smack or AppArmor. Using
> either in conjunction with the existing modules is quite
> reasonable. S.A.R.A requires access to the cred blob, while
> LandLock uses the cred, file and inode blobs.
>
> The use of the cred, file and inode blobs has been
> abstracted in preceding patches in the series. This
> patch teaches the affected security modules how to access
> the part of the blob set aside for their use in the case
> where blobs are shared. The configuration option
> CONFIG_SECURITY_STACKING identifies systems where the
> blobs may be shared.
>
> The mechanism for selecting which security modules are
> active has been changed to allow non-conflicting "major"
> security modules to be used together. At this time the
> TOMOYO module can safely be used with any of the others.
> The two new modules would be non-conflicting as well.
>
> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> ---
> Documentation/admin-guide/LSM/index.rst | 14 +++--
> include/linux/lsm_hooks.h | 2 +-
> security/Kconfig | 81 +++++++++++++++++++++++++
> security/apparmor/include/cred.h | 8 +++
> security/apparmor/include/file.h | 9 ++-
> security/apparmor/include/lib.h | 4 ++
> security/apparmor/lsm.c | 8 ++-
> security/security.c | 30 ++++++++-
> security/selinux/hooks.c | 3 +-
> security/selinux/include/objsec.h | 18 +++++-
> security/smack/smack.h | 19 +++++-
> security/smack/smack_lsm.c | 17 +++---
> security/tomoyo/common.h | 12 +++-
> security/tomoyo/tomoyo.c | 3 +-
> 14 files changed, 200 insertions(+), 28 deletions(-)
>
> diff --git a/Documentation/admin-guide/LSM/index.rst b/Documentation/admin-guide/LSM/index.rst
> index 9842e21afd4a..d3d8af174042 100644
> --- a/Documentation/admin-guide/LSM/index.rst
> +++ b/Documentation/admin-guide/LSM/index.rst
> @@ -17,10 +17,16 @@ MAC extensions, other extensions can be built using the LSM to provide
> specific changes to system operation when these tweaks are not available
> in the core functionality of Linux itself.
>
> -The Linux capabilities modules will always be included. This may be
> -followed by any number of "minor" modules and at most one "major" module.
> -For more details on capabilities, see ``capabilities(7)`` in the Linux
> -man-pages project.
> +The Linux capabilities modules will always be included. For more details
> +on capabilities, see ``capabilities(7)`` in the Linux man-pages project.
> +
> +Security modules that do not use the security data blobs maintained
> +by the LSM infrastructure are considered "minor" modules. These may be
> +included at compile time and stacked explicitly. Security modules that
> +use the LSM maintained security blobs are considered "major" modules.
> +These may only be stacked if the CONFIG_LSM_STACKED configuration
> +option is used. If this is chosen all of the security modules selected
> +will be used.
>
> A list of the active security modules can be found by reading
> ``/sys/kernel/security/lsm``. This is a comma separated list, and
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 416b20c3795b..dddcced54fea 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2079,7 +2079,7 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
> #define __lsm_ro_after_init __ro_after_init
> #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
>
> -extern int __init security_module_enable(const char *module);
> +extern bool __init security_module_enable(const char *lsm, const bool stacked);
> extern void __init capability_add_hooks(void);
> #ifdef CONFIG_SECURITY_YAMA
> extern void __init yama_add_hooks(void);
> diff --git a/security/Kconfig b/security/Kconfig
> index 22f7664c4977..ed48025ae9e0 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -36,6 +36,28 @@ config SECURITY_WRITABLE_HOOKS
> bool
> default n
>
> +config SECURITY_STACKING
> + bool "Security module stacking"
> + depends on SECURITY
> + help
> + Allows multiple major security modules to be stacked.
> + Modules are invoked in the order registered with a
> + "bail on fail" policy, in which the infrastructure
> + will stop processing once a denial is detected. Not
> + all modules can be stacked. SELinux, Smack and AppArmor are
> + known to be incompatible. User space components may
> + have trouble identifying the security module providing
> + data in some cases.
> +
> + If you select this option you will have to select which
> + of the stackable modules you wish to be active. The
> + "Default security module" will be ignored. The boot line
> + "security=" option can be used to specify that one of
> + the modules identifed for stacking should be used instead
> + of the entire stack.
> +
> + If you are unsure how to answer this question, answer N.
I don't see a good reason to make this a config. Why shouldn't this
always be enabled?
> +
> config SECURITY_LSM_DEBUG
> bool "Enable debugging of the LSM infrastructure"
> depends on SECURITY
> @@ -250,6 +272,9 @@ source security/yama/Kconfig
>
> source security/integrity/Kconfig
>
> +menu "Security Module Selection"
> + visible if !SECURITY_STACKING
> +
> choice
> prompt "Default security module"
> default DEFAULT_SECURITY_SELINUX if SECURITY_SELINUX
> @@ -289,3 +314,59 @@ config DEFAULT_SECURITY
>
> endmenu
>
> +menu "Security Module Stack"
> + visible if SECURITY_STACKING
> +
> +choice
> + prompt "Stacked 'extreme' security module"
> + default SECURITY_SELINUX_STACKED if SECURITY_SELINUX
> + default SECURITY_SMACK_STACKED if SECURITY_SMACK
> + default SECURITY_APPARMOR_STACKED if SECURITY_APPARMOR
I don't think any of this is needed either: we already have the
CONFIG_DEFAULT_SECURITY_* choice.
> [...]
> diff --git a/security/apparmor/include/cred.h b/security/apparmor/include/cred.h
> index a90eae76d7c1..be7575adf6f0 100644
> --- a/security/apparmor/include/cred.h
> +++ b/security/apparmor/include/cred.h
> @@ -25,7 +25,11 @@
>
> static inline struct aa_label *cred_label(const struct cred *cred)
> {
> +#ifdef CONFIG_SECURITY_STACKING
> + struct aa_label **blob = cred->security + apparmor_blob_sizes.lbs_cred;
> +#else
> struct aa_label **blob = cred->security;
> +#endif
Without the config, then there's no need for all these #ifdefs.
> -#define file_ctx(X) ((struct aa_file_ctx *)(X)->f_security)
> +static inline struct aa_file_ctx *file_ctx(struct file *file)
> +{
> +#ifdef CONFIG_SECURITY_STACKING
> + return file->f_security + apparmor_blob_sizes.lbs_file;
> +#else
> + return file->f_security;
> +#endif
> +}
This define->inline should have been part of an earlier patch.
> /* struct aa_file_ctx - the AppArmor context the file was opened in
> * @lock: lock to update the ctx
> diff --git a/security/apparmor/include/lib.h b/security/apparmor/include/lib.h
> index 6505e1ad9e23..bbe9b384d71d 100644
> --- a/security/apparmor/include/lib.h
> +++ b/security/apparmor/include/lib.h
> @@ -16,6 +16,7 @@
>
> #include <linux/slab.h>
> #include <linux/fs.h>
> +#include <linux/lsm_hooks.h>
>
> #include "match.h"
>
> @@ -55,6 +56,9 @@ const char *aa_splitn_fqname(const char *fqname, size_t n, const char **ns_name,
> size_t *ns_len);
> void aa_info_message(const char *str);
>
> +/* Security blob offsets */
> +extern struct lsm_blob_sizes apparmor_blob_sizes;
> +
> /**
> * aa_strneq - compare null terminated @str to a non null terminated substring
> * @str: a null terminated string
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 15716b6ff860..36d8386170e8 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1553,7 +1553,9 @@ static int __init apparmor_init(void)
> int error;
>
> if (!finish) {
> - if (apparmor_enabled && security_module_enable("apparmor"))
> + if (apparmor_enabled &&
> + security_module_enable("apparmor",
> + IS_ENABLED(CONFIG_SECURITY_APPARMOR_STACKED)))
> security_add_blobs(&apparmor_blob_sizes);
> else
> apparmor_enabled = false;
> @@ -1561,7 +1563,9 @@ static int __init apparmor_init(void)
> return 0;
> }
>
> - if (!apparmor_enabled || !security_module_enable("apparmor")) {
> + if (!apparmor_enabled ||
> + !security_module_enable("apparmor",
> + IS_ENABLED(CONFIG_SECURITY_APPARMOR_STACKED))) {
> aa_info_message("AppArmor disabled by boot time parameter");
> apparmor_enabled = false;
> return 0;
I don't think any of these changes are needed either with the loss of
the config.
> diff --git a/security/security.c b/security/security.c
> index 2501cdcbebff..06bed74d1ed0 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -36,6 +36,7 @@
>
> /* Maximum number of letters for an LSM name string */
> #define SECURITY_NAME_MAX 10
> +#define MODULE_STACK "(stacking)"
>
> struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
> @@ -48,7 +49,11 @@ static struct lsm_blob_sizes blob_sizes;
>
> /* Boot-time LSM user choice */
> static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
> +#ifdef CONFIG_SECURITY_STACKING
> + MODULE_STACK;
> +#else
> CONFIG_DEFAULT_SECURITY;
> +#endif
>
> static void __init do_security_initcalls(void)
> {
> @@ -169,6 +174,7 @@ static int lsm_append(char *new, char **result)
> /**
> * security_module_enable - Load given security module on boot ?
> * @module: the name of the module
> + * @stacked: indicates that the module wants to be stacked
> *
> * Each LSM must pass this method before registering its own operations
> * to avoid security registration races. This method may also be used
> @@ -184,9 +190,29 @@ static int lsm_append(char *new, char **result)
> *
> * Otherwise, return false.
> */
> -int __init security_module_enable(const char *module)
> +bool __init security_module_enable(const char *lsm, const bool stacked)
> {
> - return !strcmp(module, chosen_lsm);
> +#ifdef CONFIG_SECURITY_STACKING
> + /*
> + * Module defined on the command line security=XXXX
> + */
> + if (strcmp(chosen_lsm, MODULE_STACK)) {
> + if (!strcmp(lsm, chosen_lsm)) {
> + pr_info("Command line sets the %s security module.\n",
> + lsm);
> + return true;
> + }
> + return false;
> + }
> + /*
> + * Module configured as stacked.
> + */
> + return stacked;
> +#else
> + if (strcmp(lsm, chosen_lsm) == 0)
> + return true;
> + return false;
> +#endif
> }
I don't see the need for this? security_module_enable() will already
specify if it's been selected as the "primary" LSM.
The only change I think is needed here is to treat tomoyo differently.
It can now operate independently, like the "minor" LSMs. So we have
three types of LSMs now: "conflicting", "non-conflicting", and
"minor".
The only differences are how they get initialized. Major use
security_initcall() and minor use explicit calls to $lsm_add_hooks().
Yama is always enabled if built in. Loadpin uses a module parameter
("loadpin.enabled") and checks it when loadpin_add_hooks() is called.
To split tomoyo away from the other security modules, we need to track
its enablement _separately_ from the conflicting LSMs.
i.e. choose_lsm() needs to change, or tomoyo needs to use a module
parameter ("tomoyo.enabled"), or a __setup() call like apparmor and
selinux do for enablement. (Note that apparmor and selinux check
_both_ their __setup() and security_module_enable() values...)
For example (untested, likely whitespace damaged):
diff --git a/security/tomoyo/Kconfig b/security/tomoyo/Kconfig
index 404dce66952a..4edc8e733245 100644
--- a/security/tomoyo/Kconfig
+++ b/security/tomoyo/Kconfig
@@ -14,6 +14,14 @@ config SECURITY_TOMOYO
found at <http://tomoyo.sourceforge.jp/>.
If you are unsure how to answer this question, answer N.
+config SECURITY_TOMOYO_ENABLED
+ bool "Enable TOMOYO at boot"
+ depends on SECURITY_TOMOYO
+ default SECURITY_TOMOYO
+ help
+ If selected, TOMOYO will be enabled at boot. If not selected, it
+ can be enabled at boot with the kernel parameter "tomoyo.enabled=1".
+
config SECURITY_TOMOYO_MAX_ACCEPT_ENTRY
int "Default maximal count for learning mode"
default 2048
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 9f932e2d6852..8dc9ef2096ab 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -531,6 +531,8 @@ static struct security_hook_list tomoyo_hooks[]
__lsm_ro_after_init = {
/* Lock for GC. */
DEFINE_SRCU(tomoyo_ss);
+static int enabled = IS_ENABLED(CONFIG_SECURITY_TOMOYO_ENABLED);
+
/**
* tomoyo_init - Register TOMOYO Linux as a LSM module.
*
@@ -540,7 +542,7 @@ static int __init tomoyo_init(void)
{
struct cred *cred = (struct cred *) current_cred();
- if (!security_module_enable("tomoyo"))
+ if (!enabled)
return 0;
/* register ourselves with the security framework */
security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
@@ -550,4 +552,8 @@ static int __init tomoyo_init(void)
return 0;
}
+/* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
+module_param(enabled, int, 0);
+MODULE_PARM_DESC(enabled, "Tomoyo enabled at boot");
+
security_initcall(tomoyo_init);
(I prefer LSMs do enablement with module params so that they leave
their namespace open for other runtime configuration. I think
"apparmor=" and "selinux=" for enable/disable isn't good to
replicate.)
We will quickly encounter "LSM ordering" as an issue, but not in this
case yet: there's no new LSM. Ordering is preserved even with my
suggestion: major order is controlled by Makefile link ordering, and
minor is controlled by explicit ordering in security/security.c's
add_hooks() calls.
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 6617abb51732..be14540ce09c 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3493,18 +3493,16 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
> {
> struct smack_known *skp = smk_of_task_struct(p);
> char *cp;
> - int slen;
>
> - if (strcmp(name, "current") != 0)
> + if (strcmp(name, "current") == 0) {
> + cp = kstrdup(skp->smk_known, GFP_KERNEL);
> + if (cp == NULL)
> + return -ENOMEM;
> + } else
> return -EINVAL;
>
> - cp = kstrdup(skp->smk_known, GFP_KERNEL);
> - if (cp == NULL)
> - return -ENOMEM;
> -
> - slen = strlen(cp);
> *value = cp;
> - return slen;
> + return strlen(cp);
> }
This refactoring seems out of place?
-Kees
--
Kees Cook
Pixel Security