On Thu, Feb 20, 2025 at 3:24 PM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
>
> On Thu, Feb 20, 2025 at 3:23 PM Stephen Smalley
> <stephen.smalley.work@xxxxxxxxx> wrote:
> >
> > 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?
>
> Sorry, that last one would be SYSTEM__SECURITY_POLICY_LOAD to be
> precise, but the point remains.
Also, it appears that this policy_load is used by non-security code as
well, so that's another reason to not rename it.
