On Fri, Feb 21, 2025 at 9:12 AM Daniel Burgener
<dburgener@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On 2/21/2025 8:52 AM, Stephen Smalley wrote:
> > 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.
>
> What about "kernel_policy_load"? It's a little weird, since 1. SELinux
> policy is of course also a policy loaded into the kernel and 2. All of
> these permission names could be prefixed with kernel (eg
> "kernel_module_load"). But still, I sympathize with Christian's concern
> that "policy" in SELinux world refers to the SELinux policy, and
> system:policy_load vs security:load_policy is wildly confusing.
I'll defer to Paul to make the final call on naming so the patch
author doesn't need to re-spin the patches endlessly.
