On Mon, Feb 17, 2025 at 3:58 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> On Tue, 11 Feb 2025 at 19:22, <kippndavis.work@xxxxxxx> wrote:
> >
> > From: "Kipp N. Davis" <kippndavis.work@xxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Kipp N. Davis <kippndavis.work@xxxxxxx>
> >
> > ---
> > 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?
IMHO, no. READING_UNKNOWN/LOADING_UNKNOWN aren't intended to be used
and would be a bug elsewhere in the kernel.
>
> > + 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)
I don't really see how that is less ambiguous since SELinux policy is
a security policy too, but don't have any strong feelings either way.
We have the load_policy permission in the security class for loading
SELinux policy, so if we use security_policy_load here, then we'll end
up with SECURITY__LOAD_POLICY and SYSTEM__SECURITY_LOAD_POLICY.
What could go wrong?
>
> > + "x509_certificate_load", NULL } },
> > { "capability", { COMMON_CAP_PERMS, NULL } },
> > { "filesystem",
> > { "mount", "remount", "unmount", "getattr", "relabelfrom",
> > --
> > 2.48.1
> >
> >
