Re: [PATCH v4 2/2] LSM: add SafeSetID module that gates setid calls

[Kees Cook <keescook@chromium.org>]

On Tue, Jan 15, 2019 at 1:50 PM <mortonm@chromium.org> wrote:
> diff --git a/security/Kconfig b/security/Kconfig
> index 78dc12b7eeb3..9efc7a5e3280 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -236,6 +236,7 @@ source "security/tomoyo/Kconfig"
>  source "security/apparmor/Kconfig"
>  source "security/loadpin/Kconfig"
>  source "security/yama/Kconfig"
> +source "security/safesetid/Kconfig"
>
>  source "security/integrity/Kconfig"
>

In security-next, I'd expect "safesetid" to get added to "config LSM",
something like:

 config LSM
         string "Ordered list of enabled LSMs"
-        default "yama,loadpin,integrity,selinux,smack,tomoyo,apparmor"
+         default
"yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
       help
           A comma-separated list of LSMs, in initialization order.


> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> new file mode 100644
> index 000000000000..c38cab263362
> --- /dev/null
> +++ b/security/safesetid/lsm.c
> [...]
> +static struct security_hook_list safesetid_security_hooks[] = {
> +       LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid),
> +       LSM_HOOK_INIT(capable, safesetid_security_capable)
> +};
> +
> +static int __init safesetid_security_init(void)
> +{
> +       security_add_hooks(safesetid_security_hooks,
> +                          ARRAY_SIZE(safesetid_security_hooks), "safesetid");
> +
> +       return 0;
> +}

I think you need to add an "did I get initialized?" variable for the
securityfs init to check (see security/apparmor/apparmorfs.c).

> diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> new file mode 100644
> index 000000000000..bf78af9bf314
> --- /dev/null
> +++ b/security/safesetid/lsm.h
> [...]
> +static int __init safesetid_init_securityfs(void)
> +{
> +       int i;
> +       int ret;

And the init check would go here to skip tree creation if safesetid
isn't running.

> +
> +       safesetid_policy_dir = securityfs_create_dir("safesetid", NULL);
> +       if (!safesetid_policy_dir) {
> +               ret = PTR_ERR(safesetid_policy_dir);
> +               goto error;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(safesetid_files); ++i) {
> +               struct safesetid_file_entry *entry =
> +                       &safesetid_files[i];
> +               entry->dentry = securityfs_create_file(
> +                       entry->name, 0200, safesetid_policy_dir,
> +                       entry, &safesetid_file_fops);
> +               if (IS_ERR(entry->dentry)) {
> +                       ret = PTR_ERR(entry->dentry);
> +                       goto error;
> +               }
> +       }
> +
> +       return 0;
> +
> +error:
> +       safesetid_shutdown_securityfs();
> +       return ret;
> +}
> +fs_initcall(safesetid_init_securityfs);

After that, feel free to include:

Acked-by: Kees Cook <keescook@chromium.org>

Thanks for the updates!

-- 
Kees Cook