[RFC PATCH] selinux: deprecate setting checkreqprot to 1

[RFC PATCH] selinux: deprecate setting checkreqprot to 1

[Stephen Smalley]

Deprecate setting the SELinux checkreqprot tunable to 1 via kernel
parameter or /sys/fs/selinux/checkreqprot.  Setting it to 0 is left
intact for compatibility since Android and some Linux distributions
do so for security and treat an inability to set it as a fatal error.
Eventually setting it to 0 will become a no-op and the kernel will
stop using checkreqprot's value internally altogether.

checkreqprot was originally introduced as a compatibility mechanism
for legacy userspace and the READ_IMPLIES_EXEC personality flag.
However, if set to 1, it weakens security by allowing mappings to be
made executable without authorization by policy.  The default value
for the SECURITY_SELINUX_CHECKREQPROT_VALUE config option was changed
from 1 to 0 in commit 2a35d196c160e3 ("selinux: change
CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE default") and both Android
and Linux distributions began explicitly setting
/sys/fs/selinux/checkreqprot to 0 some time ago.

Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
RFC-only, not yet tested.

 Documentation/admin-guide/kernel-parameters.txt | 1 +
 security/selinux/Kconfig                        | 3 +++
 security/selinux/hooks.c                        | 4 ++++
 security/selinux/selinuxfs.c                    | 6 ++++++
 4 files changed, 14 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index eed51293d6cf..c894ddfa1393 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -512,6 +512,7 @@
 			Default value is set via a kernel config option.
 			Value can be changed at runtime via
 				/sys/fs/selinux/checkreqprot.
+			Setting checkreqprot to 1 is deprecated.
 
 	cio_ignore=	[S390]
 			See Documentation/s390/common_io.rst for details.
diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
index 1014cb0ee956..9e921fc72538 100644
--- a/security/selinux/Kconfig
+++ b/security/selinux/Kconfig
@@ -88,6 +88,9 @@ config SECURITY_SELINUX_CHECKREQPROT_VALUE
 	  'checkreqprot=' boot parameter.  It may also be changed at runtime
 	  via /sys/fs/selinux/checkreqprot if authorized by policy.
 
+	  WARNING: this option is deprecated and will be removed in a future
+	  kernel release.
+
 	  If you are unsure how to answer this question, answer 0.
 
 config SECURITY_SELINUX_SIDTAB_HASH_BITS
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 63a6e36abe9f..11d47bb7d40a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7149,7 +7149,11 @@ static __init int selinux_init(void)
 
 	memset(&selinux_state, 0, sizeof(selinux_state));
 	enforcing_set(&selinux_state, selinux_enforcing_boot);
+
 	selinux_state.checkreqprot = selinux_checkreqprot_boot;
+	if (selinux_state.checkreqprot)
+		pr_warn("SELinux: checkreqprot set to 1 via kernel parameter.  This is deprecated.\n");
+
 	selinux_ss_init(&selinux_state.ss);
 	selinux_avc_init(&selinux_state.avc);
 
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 79c710911a3c..c6c136eee371 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -668,6 +668,12 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
 	if (sscanf(page, "%u", &new_value) != 1)
 		goto out;
 
+	if (new_value) {
+		char comm[sizeof(current->comm)];
+		memcpy(comm, current->comm, sizeof(comm));
+		pr_warn_once("SELinux: %s (%d) set checkreqprot to 1.  This is deprecated.\n", comm, current->pid);
+	}
+
 	fsi->state->checkreqprot = new_value ? 1 : 0;
 	length = count;
 out:
-- 
2.24.1

Re: [RFC PATCH] selinux: deprecate setting checkreqprot to 1

[Paul Moore]

On Tue, Jan 7, 2020 at 3:22 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> Deprecate setting the SELinux checkreqprot tunable to 1 via kernel
> parameter or /sys/fs/selinux/checkreqprot.  Setting it to 0 is left
> intact for compatibility since Android and some Linux distributions
> do so for security and treat an inability to set it as a fatal error.
> Eventually setting it to 0 will become a no-op and the kernel will
> stop using checkreqprot's value internally altogether.
>
> checkreqprot was originally introduced as a compatibility mechanism
> for legacy userspace and the READ_IMPLIES_EXEC personality flag.
> However, if set to 1, it weakens security by allowing mappings to be
> made executable without authorization by policy.  The default value
> for the SECURITY_SELINUX_CHECKREQPROT_VALUE config option was changed
> from 1 to 0 in commit 2a35d196c160e3 ("selinux: change
> CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE default") and both Android
> and Linux distributions began explicitly setting
> /sys/fs/selinux/checkreqprot to 0 some time ago.
>
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> RFC-only, not yet tested.
>
>  Documentation/admin-guide/kernel-parameters.txt | 1 +
>  security/selinux/Kconfig                        | 3 +++
>  security/selinux/hooks.c                        | 4 ++++
>  security/selinux/selinuxfs.c                    | 6 ++++++
>  4 files changed, 14 insertions(+)

No objection so long as the testing goes okay, although I don't think
we will see any surprises.

However, as you pointed out earlier, we should probably add an entry
to Documentation/ABI/obsolete; while the "checkreqprot" selinuxfs node
isn't going away, we are restricting it in a way which was previously
allowed.

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index eed51293d6cf..c894ddfa1393 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -512,6 +512,7 @@
>                         Default value is set via a kernel config option.
>                         Value can be changed at runtime via
>                                 /sys/fs/selinux/checkreqprot.
> +                       Setting checkreqprot to 1 is deprecated.
>
>         cio_ignore=     [S390]
>                         See Documentation/s390/common_io.rst for details.
> diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
> index 1014cb0ee956..9e921fc72538 100644
> --- a/security/selinux/Kconfig
> +++ b/security/selinux/Kconfig
> @@ -88,6 +88,9 @@ config SECURITY_SELINUX_CHECKREQPROT_VALUE
>           'checkreqprot=' boot parameter.  It may also be changed at runtime
>           via /sys/fs/selinux/checkreqprot if authorized by policy.
>
> +         WARNING: this option is deprecated and will be removed in a future
> +         kernel release.
> +
>           If you are unsure how to answer this question, answer 0.
>
>  config SECURITY_SELINUX_SIDTAB_HASH_BITS
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 63a6e36abe9f..11d47bb7d40a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -7149,7 +7149,11 @@ static __init int selinux_init(void)
>
>         memset(&selinux_state, 0, sizeof(selinux_state));
>         enforcing_set(&selinux_state, selinux_enforcing_boot);
> +
>         selinux_state.checkreqprot = selinux_checkreqprot_boot;
> +       if (selinux_state.checkreqprot)
> +               pr_warn("SELinux: checkreqprot set to 1 via kernel parameter.  This is deprecated.\n");
> +
>         selinux_ss_init(&selinux_state.ss);
>         selinux_avc_init(&selinux_state.avc);
>
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 79c710911a3c..c6c136eee371 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -668,6 +668,12 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
>         if (sscanf(page, "%u", &new_value) != 1)
>                 goto out;
>
> +       if (new_value) {
> +               char comm[sizeof(current->comm)];
> +               memcpy(comm, current->comm, sizeof(comm));
> +               pr_warn_once("SELinux: %s (%d) set checkreqprot to 1.  This is deprecated.\n", comm, current->pid);
> +       }
> +
>         fsi->state->checkreqprot = new_value ? 1 : 0;
>         length = count;
>  out:
> --
> 2.24.1

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH] selinux: deprecate setting checkreqprot to 1

[Stephen Smalley]

On 1/8/20 12:57 AM, Paul Moore wrote:
> On Tue, Jan 7, 2020 at 3:22 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>
>> Deprecate setting the SELinux checkreqprot tunable to 1 via kernel
>> parameter or /sys/fs/selinux/checkreqprot.  Setting it to 0 is left
>> intact for compatibility since Android and some Linux distributions
>> do so for security and treat an inability to set it as a fatal error.
>> Eventually setting it to 0 will become a no-op and the kernel will
>> stop using checkreqprot's value internally altogether.
>>
>> checkreqprot was originally introduced as a compatibility mechanism
>> for legacy userspace and the READ_IMPLIES_EXEC personality flag.
>> However, if set to 1, it weakens security by allowing mappings to be
>> made executable without authorization by policy.  The default value
>> for the SECURITY_SELINUX_CHECKREQPROT_VALUE config option was changed
>> from 1 to 0 in commit 2a35d196c160e3 ("selinux: change
>> CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE default") and both Android
>> and Linux distributions began explicitly setting
>> /sys/fs/selinux/checkreqprot to 0 some time ago.
>>
>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>> ---
>> RFC-only, not yet tested.
>>
>>   Documentation/admin-guide/kernel-parameters.txt | 1 +
>>   security/selinux/Kconfig                        | 3 +++
>>   security/selinux/hooks.c                        | 4 ++++
>>   security/selinux/selinuxfs.c                    | 6 ++++++
>>   4 files changed, 14 insertions(+)
> 
> No objection so long as the testing goes okay, although I don't think
> we will see any surprises.

Booting with this patch did produce one surprise; it logged a warning 
claiming that checkreqprot was set to 1 via kernel parameter on Fedora. 
This was incorrect; it is actually defaulted to 1 via kernel config on 
Fedora (Fedora kernel config still has 
CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE=1 at least in F31), so I 
moved the check to checkreqprot_setup() so that it will only log if 
explicitly set via kernel parameter.  Fedora is switching checkreqprot 
to 0 from systemd-tmpfiles via an entry in 
/usr/lib/tmpfiles.d/selinux-policy.conf, so at least it isn't left as 1 
after startup.

> However, as you pointed out earlier, we should probably add an entry
> to Documentation/ABI/obsolete; while the "checkreqprot" selinuxfs node
> isn't going away, we are restricting it in a way which was previously
> allowed.

Ok, I'll do that and add the documentation maintainers to the next 
version of the patch.  I'll follow your example on using sysfs-selinux 
as the prefix but we'll see if they prefer selinuxfs- instead.

> 
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index eed51293d6cf..c894ddfa1393 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -512,6 +512,7 @@
>>                          Default value is set via a kernel config option.
>>                          Value can be changed at runtime via
>>                                  /sys/fs/selinux/checkreqprot.
>> +                       Setting checkreqprot to 1 is deprecated.
>>
>>          cio_ignore=     [S390]
>>                          See Documentation/s390/common_io.rst for details.
>> diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
>> index 1014cb0ee956..9e921fc72538 100644
>> --- a/security/selinux/Kconfig
>> +++ b/security/selinux/Kconfig
>> @@ -88,6 +88,9 @@ config SECURITY_SELINUX_CHECKREQPROT_VALUE
>>            'checkreqprot=' boot parameter.  It may also be changed at runtime
>>            via /sys/fs/selinux/checkreqprot if authorized by policy.
>>
>> +         WARNING: this option is deprecated and will be removed in a future
>> +         kernel release.
>> +
>>            If you are unsure how to answer this question, answer 0.
>>
>>   config SECURITY_SELINUX_SIDTAB_HASH_BITS
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 63a6e36abe9f..11d47bb7d40a 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -7149,7 +7149,11 @@ static __init int selinux_init(void)
>>
>>          memset(&selinux_state, 0, sizeof(selinux_state));
>>          enforcing_set(&selinux_state, selinux_enforcing_boot);
>> +
>>          selinux_state.checkreqprot = selinux_checkreqprot_boot;
>> +       if (selinux_state.checkreqprot)
>> +               pr_warn("SELinux: checkreqprot set to 1 via kernel parameter.  This is deprecated.\n");
>> +
>>          selinux_ss_init(&selinux_state.ss);
>>          selinux_avc_init(&selinux_state.avc);
>>
>> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
>> index 79c710911a3c..c6c136eee371 100644
>> --- a/security/selinux/selinuxfs.c
>> +++ b/security/selinux/selinuxfs.c
>> @@ -668,6 +668,12 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
>>          if (sscanf(page, "%u", &new_value) != 1)
>>                  goto out;
>>
>> +       if (new_value) {
>> +               char comm[sizeof(current->comm)];
>> +               memcpy(comm, current->comm, sizeof(comm));
>> +               pr_warn_once("SELinux: %s (%d) set checkreqprot to 1.  This is deprecated.\n", comm, current->pid);
>> +       }
>> +
>>          fsi->state->checkreqprot = new_value ? 1 : 0;
>>          length = count;
>>   out:
>> --
>> 2.24.1
> 

Re: [RFC PATCH] selinux: deprecate setting checkreqprot to 1

[Ondrej Mosnacek]

On Wed, Jan 8, 2020 at 5:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 1/8/20 12:57 AM, Paul Moore wrote:
> > On Tue, Jan 7, 2020 at 3:22 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>
> >> Deprecate setting the SELinux checkreqprot tunable to 1 via kernel
> >> parameter or /sys/fs/selinux/checkreqprot.  Setting it to 0 is left
> >> intact for compatibility since Android and some Linux distributions
> >> do so for security and treat an inability to set it as a fatal error.
> >> Eventually setting it to 0 will become a no-op and the kernel will
> >> stop using checkreqprot's value internally altogether.
> >>
> >> checkreqprot was originally introduced as a compatibility mechanism
> >> for legacy userspace and the READ_IMPLIES_EXEC personality flag.
> >> However, if set to 1, it weakens security by allowing mappings to be
> >> made executable without authorization by policy.  The default value
> >> for the SECURITY_SELINUX_CHECKREQPROT_VALUE config option was changed
> >> from 1 to 0 in commit 2a35d196c160e3 ("selinux: change
> >> CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE default") and both Android
> >> and Linux distributions began explicitly setting
> >> /sys/fs/selinux/checkreqprot to 0 some time ago.
> >>
> >> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> >> ---
> >> RFC-only, not yet tested.
> >>
> >>   Documentation/admin-guide/kernel-parameters.txt | 1 +
> >>   security/selinux/Kconfig                        | 3 +++
> >>   security/selinux/hooks.c                        | 4 ++++
> >>   security/selinux/selinuxfs.c                    | 6 ++++++
> >>   4 files changed, 14 insertions(+)
> >
> > No objection so long as the testing goes okay, although I don't think
> > we will see any surprises.
>
> Booting with this patch did produce one surprise; it logged a warning
> claiming that checkreqprot was set to 1 via kernel parameter on Fedora.
> This was incorrect; it is actually defaulted to 1 via kernel config on
> Fedora (Fedora kernel config still has
> CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE=1 at least in F31), so I
> moved the check to checkreqprot_setup() so that it will only log if
> explicitly set via kernel parameter.  Fedora is switching checkreqprot
> to 0 from systemd-tmpfiles via an entry in
> /usr/lib/tmpfiles.d/selinux-policy.conf, so at least it isn't left as 1
> after startup.

Interesting... should I send a PR to change the kernel config on
Fedora? Does anyone know why the
/usr/lib/tmpfiles.d/selinux-policy.conf approach was used instead of
just setting the kernel config option to 0? I assume the config option
just didn't exist yet back then?

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH] selinux: deprecate setting checkreqprot to 1

[Stephen Smalley]

On 1/8/20 11:26 AM, Ondrej Mosnacek wrote:
> On Wed, Jan 8, 2020 at 5:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 1/8/20 12:57 AM, Paul Moore wrote:
>>> On Tue, Jan 7, 2020 at 3:22 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>>
>>>> Deprecate setting the SELinux checkreqprot tunable to 1 via kernel
>>>> parameter or /sys/fs/selinux/checkreqprot.  Setting it to 0 is left
>>>> intact for compatibility since Android and some Linux distributions
>>>> do so for security and treat an inability to set it as a fatal error.
>>>> Eventually setting it to 0 will become a no-op and the kernel will
>>>> stop using checkreqprot's value internally altogether.
>>>>
>>>> checkreqprot was originally introduced as a compatibility mechanism
>>>> for legacy userspace and the READ_IMPLIES_EXEC personality flag.
>>>> However, if set to 1, it weakens security by allowing mappings to be
>>>> made executable without authorization by policy.  The default value
>>>> for the SECURITY_SELINUX_CHECKREQPROT_VALUE config option was changed
>>>> from 1 to 0 in commit 2a35d196c160e3 ("selinux: change
>>>> CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE default") and both Android
>>>> and Linux distributions began explicitly setting
>>>> /sys/fs/selinux/checkreqprot to 0 some time ago.
>>>>
>>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>>>> ---
>>>> RFC-only, not yet tested.
>>>>
>>>>    Documentation/admin-guide/kernel-parameters.txt | 1 +
>>>>    security/selinux/Kconfig                        | 3 +++
>>>>    security/selinux/hooks.c                        | 4 ++++
>>>>    security/selinux/selinuxfs.c                    | 6 ++++++
>>>>    4 files changed, 14 insertions(+)
>>>
>>> No objection so long as the testing goes okay, although I don't think
>>> we will see any surprises.
>>
>> Booting with this patch did produce one surprise; it logged a warning
>> claiming that checkreqprot was set to 1 via kernel parameter on Fedora.
>> This was incorrect; it is actually defaulted to 1 via kernel config on
>> Fedora (Fedora kernel config still has
>> CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE=1 at least in F31), so I
>> moved the check to checkreqprot_setup() so that it will only log if
>> explicitly set via kernel parameter.  Fedora is switching checkreqprot
>> to 0 from systemd-tmpfiles via an entry in
>> /usr/lib/tmpfiles.d/selinux-policy.conf, so at least it isn't left as 1
>> after startup.
> 
> Interesting... should I send a PR to change the kernel config on
> Fedora? Does anyone know why the
> /usr/lib/tmpfiles.d/selinux-policy.conf approach was used instead of
> just setting the kernel config option to 0? I assume the config option
> just didn't exist yet back then?

Yes, I would recommend doing so since the config option is being 
deprecated too.  The config option predates git so I don't think that 
was the issue; it was probably just a case of the SELinux userspace 
maintainers being able to package a tmpfiles.d config file easily and no 
kernel developer pushing the corresponding change.

I could see this potentially creating problems for architectures that 
still default to VM_EXEC being included in their VM_DATA_DEFAULT_FLAGS. 
For those, we might need to disable the FILE__EXECUTE check on 
mmap/mprotect just as we do for PROCESS__EXECMEM.