Re: [PATCH v4 2/2] LSM: add SafeSetID module that gates setid calls
From: Kees Cook <hidden>
Date: 2019-01-15 22:33:15
On Tue, Jan 15, 2019 at 1:50 PM [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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).
quoted hunk ↗ jump to hunk
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 <redacted> Thanks for the updates! -- Kees Cook