diff mbox series

[v2] selinux: add permission checks for loading other kinds of kernel files

Message ID 20250211182159.37744-1-kippndavis.work@gmx.com (mailing list archive)
State New
Headers show
Series [v2] selinux: add permission checks for loading other kinds of kernel files | expand

Commit Message

kippndavis.work@gmx.com Feb. 11, 2025, 6:21 p.m. UTC 
From: "Kipp N. Davis" <kippndavis.work@gmx.com>

Although the LSM hooks for loading kernel modules were later generalized
to cover loading other kinds of files, SELinux didn't implement
corresponding permission checks, leaving only the module case covered.
Define and add new permission checks for these other cases.

Signed-off-by: Cameron K. Williams <ckwilliams.work@gmail.com>
Signed-off-by: Kipp N. Davis <kippndavis.work@gmx.com>

---
Changes in v2:
  - Removed the `-EACCES` return in default case in
    selinux_kernel_read_file() and selinux_kernel_load_from_file(),
    reverting previous fallback behavior.
  - Added a compile-time check in these	functions to catch new
    READING/LOADING_XXX	entries.

Thanks for your review! I've adjusted the default case so as to not
return an error and depart from pre-existing logic. I first tried to use
the pre-processor #errors but failed with the READING/LOADING_MAX_ID
enums, so I opted for BUILD_BUG_ON_MSG as a compile-time check with
those same enums instead to catch new entries.
---
 security/selinux/hooks.c            | 56 +++++++++++++++++++++++------
 security/selinux/include/classmap.h |  4 ++-
 2 files changed, 49 insertions(+), 11 deletions(-)

--
2.48.1

Comments

Christian Göttsche Feb. 17, 2025, 8:58 p.m. UTC | #1 
On Tue, 11 Feb 2025 at 19:22, <kippndavis.work@gmx.com> wrote:
>
> From: "Kipp N. Davis" <kippndavis.work@gmx.com>
>
> Although the LSM hooks for loading kernel modules were later generalized
> to cover loading other kinds of files, SELinux didn't implement
> corresponding permission checks, leaving only the module case covered.
> Define and add new permission checks for these other cases.
>
> Signed-off-by: Cameron K. Williams <ckwilliams.work@gmail.com>
> Signed-off-by: Kipp N. Davis <kippndavis.work@gmx.com>
>
> ---
> Changes in v2:
>   - Removed the `-EACCES` return in default case in
>     selinux_kernel_read_file() and selinux_kernel_load_from_file(),
>     reverting previous fallback behavior.
>   - Added a compile-time check in these functions to catch new
>     READING/LOADING_XXX entries.
>
> Thanks for your review! I've adjusted the default case so as to not
> return an error and depart from pre-existing logic. I first tried to use
> the pre-processor #errors but failed with the READING/LOADING_MAX_ID
> enums, so I opted for BUILD_BUG_ON_MSG as a compile-time check with
> those same enums instead to catch new entries.
> ---
>  security/selinux/hooks.c            | 56 +++++++++++++++++++++++------
>  security/selinux/include/classmap.h |  4 ++-
>  2 files changed, 49 insertions(+), 11 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 7b867dfec88b..67bf37693cd7 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4096,7 +4096,7 @@ static int selinux_kernel_module_request(char *kmod_name)
>                             SYSTEM__MODULE_REQUEST, &ad);
>  }
>
> -static int selinux_kernel_module_from_file(struct file *file)
> +static int selinux_kernel_load_from_file(struct file *file, u32 requested)
>  {
>         struct common_audit_data ad;
>         struct inode_security_struct *isec;
> @@ -4104,12 +4104,9 @@ static int selinux_kernel_module_from_file(struct file *file)
>         u32 sid = current_sid();
>         int rc;
>
> -       /* init_module */
>         if (file == NULL)
>                 return avc_has_perm(sid, sid, SECCLASS_SYSTEM,
> -                                       SYSTEM__MODULE_LOAD, NULL);
> -
> -       /* finit_module */
> +                                       requested, NULL);
>
>         ad.type = LSM_AUDIT_DATA_FILE;
>         ad.u.file = file;
> @@ -4123,7 +4120,7 @@ static int selinux_kernel_module_from_file(struct file *file)
>
>         isec = inode_security(file_inode(file));
>         return avc_has_perm(sid, isec->sid, SECCLASS_SYSTEM,
> -                               SYSTEM__MODULE_LOAD, &ad);
> +                               requested, &ad);
>  }
>
>  static int selinux_kernel_read_file(struct file *file,
> @@ -4131,10 +4128,32 @@ static int selinux_kernel_read_file(struct file *file,
>                                     bool contents)
>  {
>         int rc = 0;
> -
> +       BUILD_BUG_ON_MSG(READING_MAX_ID > 7,
> +                 "New kernel_read_file_id introduced; update SELinux!");
>         switch (id) {

Should READING_UNKNOWN be handled?
It seems not to be used currently in-tree, but maybe stay on the safe side?

> +       case READING_FIRMWARE:
> +               rc = selinux_kernel_load_from_file(contents ? file : NULL,
> +                               SYSTEM__FIRMWARE_LOAD);
> +               break;
>         case READING_MODULE:
> -               rc = selinux_kernel_module_from_file(contents ? file : NULL);
> +               rc = selinux_kernel_load_from_file(contents ? file : NULL,
> +                               SYSTEM__MODULE_LOAD);
> +               break;
> +       case READING_KEXEC_IMAGE:
> +               rc = selinux_kernel_load_from_file(contents ? file : NULL,
> +                               SYSTEM__KEXEC_IMAGE_LOAD);
> +               break;
> +       case READING_KEXEC_INITRAMFS:
> +               rc = selinux_kernel_load_from_file(contents ? file : NULL,
> +                               SYSTEM__KEXEC_INITRAMFS_LOAD);
> +               break;
> +       case READING_POLICY:
> +               rc = selinux_kernel_load_from_file(contents ? file : NULL,
> +                               SYSTEM__POLICY_LOAD);
> +               break;
> +       case READING_X509_CERTIFICATE:
> +               rc = selinux_kernel_load_from_file(contents ? file : NULL,
> +                               SYSTEM__X509_CERTIFICATE_LOAD);
>                 break;
>         default:
>                 break;
> @@ -4146,10 +4165,27 @@ static int selinux_kernel_read_file(struct file *file,
>  static int selinux_kernel_load_data(enum kernel_load_data_id id, bool contents)
>  {
>         int rc = 0;
> -
> +       BUILD_BUG_ON_MSG(LOADING_MAX_ID > 7,
> +        "New kernel_load_data_id introduced; update SELinux!");
>         switch (id) {

dito

> +       case LOADING_FIRMWARE:
> +               rc = selinux_kernel_load_from_file(NULL, SYSTEM__FIRMWARE_LOAD);
> +               break;
>         case LOADING_MODULE:
> -               rc = selinux_kernel_module_from_file(NULL);
> +               rc = selinux_kernel_load_from_file(NULL, SYSTEM__MODULE_LOAD);
> +               break;
> +       case LOADING_KEXEC_IMAGE:
> +               rc = selinux_kernel_load_from_file(NULL, SYSTEM__KEXEC_IMAGE_LOAD);
> +               break;
> +       case LOADING_KEXEC_INITRAMFS:
> +               rc = selinux_kernel_load_from_file(NULL, SYSTEM__KEXEC_INITRAMFS_LOAD);
> +               break;
> +       case LOADING_POLICY:
> +               rc = selinux_kernel_load_from_file(NULL, SYSTEM__POLICY_LOAD);
> +               break;
> +       case LOADING_X509_CERTIFICATE:
> +               rc = selinux_kernel_load_from_file(NULL,
> +                               SYSTEM__X509_CERTIFICATE_LOAD);
>                 break;
>         default:
>                 break;
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 03e82477dce9..cfac41d12f7d 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -63,7 +63,9 @@ const struct security_class_mapping secclass_map[] = {
>         { "process2", { "nnp_transition", "nosuid_transition", NULL } },
>         { "system",
>           { "ipc_info", "syslog_read", "syslog_mod", "syslog_console",
> -           "module_request", "module_load", NULL } },
> +           "module_request", "module_load", "firmware_load",
> +           "kexec_image_load", "kexec_initramfs_load", "policy_load",

In the SELinux world the word "policy" refers mostly to the SELinux policy.
Maybe rename the permission verb "policy_load" to
"security_policy_load" or similar? (it seems to be used by IMA,
loadpin and zram currently)

> +           "x509_certificate_load", NULL } },
>         { "capability", { COMMON_CAP_PERMS, NULL } },
>         { "filesystem",
>           { "mount", "remount", "unmount", "getattr", "relabelfrom",
> --
> 2.48.1
>
>
diff mbox series

Patch

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7b867dfec88b..67bf37693cd7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4096,7 +4096,7 @@  static int selinux_kernel_module_request(char *kmod_name)
 			    SYSTEM__MODULE_REQUEST, &ad);
 }

-static int selinux_kernel_module_from_file(struct file *file)
+static int selinux_kernel_load_from_file(struct file *file, u32 requested)
 {
 	struct common_audit_data ad;
 	struct inode_security_struct *isec;
@@ -4104,12 +4104,9 @@  static int selinux_kernel_module_from_file(struct file *file)
 	u32 sid = current_sid();
 	int rc;

-	/* init_module */
 	if (file == NULL)
 		return avc_has_perm(sid, sid, SECCLASS_SYSTEM,
-					SYSTEM__MODULE_LOAD, NULL);
-
-	/* finit_module */
+					requested, NULL);

 	ad.type = LSM_AUDIT_DATA_FILE;
 	ad.u.file = file;
@@ -4123,7 +4120,7 @@  static int selinux_kernel_module_from_file(struct file *file)

 	isec = inode_security(file_inode(file));
 	return avc_has_perm(sid, isec->sid, SECCLASS_SYSTEM,
-				SYSTEM__MODULE_LOAD, &ad);
+				requested, &ad);
 }

 static int selinux_kernel_read_file(struct file *file,
@@ -4131,10 +4128,32 @@  static int selinux_kernel_read_file(struct file *file,
 				    bool contents)
 {
 	int rc = 0;
-
+	BUILD_BUG_ON_MSG(READING_MAX_ID > 7,
+                 "New kernel_read_file_id introduced; update SELinux!");
 	switch (id) {
+	case READING_FIRMWARE:
+		rc = selinux_kernel_load_from_file(contents ? file : NULL,
+				SYSTEM__FIRMWARE_LOAD);
+		break;
 	case READING_MODULE:
-		rc = selinux_kernel_module_from_file(contents ? file : NULL);
+		rc = selinux_kernel_load_from_file(contents ? file : NULL,
+				SYSTEM__MODULE_LOAD);
+		break;
+	case READING_KEXEC_IMAGE:
+		rc = selinux_kernel_load_from_file(contents ? file : NULL,
+				SYSTEM__KEXEC_IMAGE_LOAD);
+		break;
+	case READING_KEXEC_INITRAMFS:
+		rc = selinux_kernel_load_from_file(contents ? file : NULL,
+				SYSTEM__KEXEC_INITRAMFS_LOAD);
+		break;
+	case READING_POLICY:
+		rc = selinux_kernel_load_from_file(contents ? file : NULL,
+				SYSTEM__POLICY_LOAD);
+		break;
+	case READING_X509_CERTIFICATE:
+		rc = selinux_kernel_load_from_file(contents ? file : NULL,
+				SYSTEM__X509_CERTIFICATE_LOAD);
 		break;
 	default:
 		break;
@@ -4146,10 +4165,27 @@  static int selinux_kernel_read_file(struct file *file,
 static int selinux_kernel_load_data(enum kernel_load_data_id id, bool contents)
 {
 	int rc = 0;
-
+	BUILD_BUG_ON_MSG(LOADING_MAX_ID > 7,
+        "New kernel_load_data_id introduced; update SELinux!");
 	switch (id) {
+	case LOADING_FIRMWARE:
+		rc = selinux_kernel_load_from_file(NULL, SYSTEM__FIRMWARE_LOAD);
+		break;
 	case LOADING_MODULE:
-		rc = selinux_kernel_module_from_file(NULL);
+		rc = selinux_kernel_load_from_file(NULL, SYSTEM__MODULE_LOAD);
+		break;
+	case LOADING_KEXEC_IMAGE:
+		rc = selinux_kernel_load_from_file(NULL, SYSTEM__KEXEC_IMAGE_LOAD);
+		break;
+	case LOADING_KEXEC_INITRAMFS:
+		rc = selinux_kernel_load_from_file(NULL, SYSTEM__KEXEC_INITRAMFS_LOAD);
+		break;
+	case LOADING_POLICY:
+		rc = selinux_kernel_load_from_file(NULL, SYSTEM__POLICY_LOAD);
+		break;
+	case LOADING_X509_CERTIFICATE:
+		rc = selinux_kernel_load_from_file(NULL,
+				SYSTEM__X509_CERTIFICATE_LOAD);
 		break;
 	default:
 		break;
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 03e82477dce9..cfac41d12f7d 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -63,7 +63,9 @@  const struct security_class_mapping secclass_map[] = {
 	{ "process2", { "nnp_transition", "nosuid_transition", NULL } },
 	{ "system",
 	  { "ipc_info", "syslog_read", "syslog_mod", "syslog_console",
-	    "module_request", "module_load", NULL } },
+	    "module_request", "module_load", "firmware_load",
+	    "kexec_image_load", "kexec_initramfs_load", "policy_load",
+	    "x509_certificate_load", NULL } },
 	{ "capability", { COMMON_CAP_PERMS, NULL } },
 	{ "filesystem",
 	  { "mount", "remount", "unmount", "getattr", "relabelfrom",