Re: [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts

[Daniel Walsh <dwalsh@redhat.com>]

On 1/15/19 1:03 PM, Stephen Smalley wrote:
> On 1/15/19 11:22 AM, Petr Lautrbach wrote:
>> Stephen Smalley <sds@tycho.nsa.gov> writes:
>>
>>> On 1/14/19 6:31 AM, Petr Lautrbach wrote:
>>>> Stephen Smalley <sds@tycho.nsa.gov> writes:
>>>>
>>>>> As reported in #123, setsebool immediately exits with an error if
>>>>> SELinux is disabled, preventing its use for setting boolean
>>>>> persistent
>>>>> values.  In contrast, semanage boolean -m works on SELinux-disabled
>>>>> hosts.  Change setsebool so that it can be used with the -P option
>>>>> (persistent changes) even if SELinux is disabled.  In the
>>>>> SELinux-disabled
>>>>> case, skip setting of active boolean values, but set the
>>>>> persistent value
>>>>> in the policy store.  Policy reload is automatically disabled by
>>>>> libsemanage
>>>>> when SELinux is disabled, so we only need to call
>>>>> semanage_set_reload()
>>>>> if -N was used.
>>>>>
>>>>
>>>> So right now, `setsebool -N` and `semanage boolean -N` have the
>>>> same effect that
>>>> `load_policy` is not run, but the value of the boolean is changed when
>>>> SELinux is enabled so it affects the system. Would it make sense to
>>>> use
>>>> -N to just change values in the store and do not change the value
>>>> in the
>>>> running kernel? E.g.
>>>>
>>>> --- a/policycoreutils/setsebool/setsebool.c
>>>> +++ b/policycoreutils/setsebool/setsebool.c
>>>> @@ -187,11 +187,14 @@ static int semanage_set_boolean_list(size_t
>>>> boolcnt,
>>>>                                                     boolean) < 0)
>>>>                           goto err;
>>>>    -               if (enabled && semanage_bool_set_active(handle,
>>>> bool_key,
>>>> boolean) < 0) {
>>>> -                       fprintf(stderr, "Failed to change boolean
>>>> %s: %m\n",
>>>> -                               boollist[j].name);
>>>> -                       goto err;
>>>> -               }
>>>> +               if (no_reload)
>>>> +                       semanage_set_reload(handle, 0);
>>>> +               else
>>>> +                       if (enabled &&
>>>> semanage_bool_set_active(handle, bool_key, boolean) < 0) {
>>>> +                               fprintf(stderr, "Failed to change
>>>> boolean %s: %m\n",
>>>> +                                                              
>>>> boollist[j].name);
>>>> +                               goto err;
>>>> +                       }
>>>>
>>>>
>>>> A similar patch would need to be applied to seobject.py as well in
>>>> this case.
>>>
>>> That makes sense to me logically (in fact, I don't really understand
>>> why
>>> setsebool w/o -P would ever trigger a reload), but I guess the
>>> concern is
>>> whether any existing users rely on the current behavior, e.g. the %post
>>> scriptlet in container-selinux that led to this issue.
>>
>> container-selinux.spec:
>> ==========================================================================
>>
>> # Install all modules in a single transaction
>> if [ $1 -eq 1 ]; then
>>      %{_sbindir}/setsebool -P -N virt_use_nfs=1
>> virt_sandbox_use_all_caps=1
>> fi
>> ...
>> if %{_sbindir}/selinuxenabled ; then
>>      %{_sbindir}/load_policy
>>      %relabel_files
>>      if [ $1 -eq 1 ]; then
>>     restorecon -R %{_sharedstatedir}/docker &> /dev/null || :
>>     restorecon -R %{_sharedstatedir}/containers &> /dev/null || :
>>      fi
>> fi
>> ==========================================================================
>>
>>
>> It would definitely break this scriptlet on SELinux enabled systems as
>> load_policy preserves booleans.
>>
>> So the question is if it's preferred current behavior with it's side
>> effects or if it's worth to try to fix it and properly announce the
>> change in release notes.
>>
>> I take that it's not nice to change/break things but to me it
>> looks like -N generally considered as option which is used to avoid
>> changes in the running kernel.
>
> As documented in the man page, -N just means that "the policy on disk
> is not reloaded into the kernel.".  So from a compatibility and a
> documentation POV, I doubt we can just change this behavior now.
> Looking back at the original commit that added -N to setsebool
> (413b4933ee7203286050c2daf6f9714673cd3a5a) , it says " Fix setsebool
> to use -N to not reload policy into the kernel optional on permanant
> changes." which suggests that the intent was to only affect persistent
> boolean changes IIUC.  But cc'ing Dan for clarification.
Yes that was the intention. The goal was to work on disabled systems.
>
>>
>>
>>>
>>>>
>>>>
>>>>
>>>>> Fixes: https://github.com/SELinuxProject/selinux/issues/123
>>>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>>>>> ---
>>>>> v2 changes setsebool to only call semanage_set_reload() if -N was
>>>>> specified;
>>>>> otherwise we can use the libsemanage defaults just as we do in
>>>>> semodule
>>>>> and semanage.
>>>>>    policycoreutils/setsebool/setsebool.c | 15 ++++++---------
>>>>>    1 file changed, 6 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/policycoreutils/setsebool/setsebool.c
>>>>> b/policycoreutils/setsebool/setsebool.c
>>>>> index 53d3566c..a5157efc 100644
>>>>> --- a/policycoreutils/setsebool/setsebool.c
>>>>> +++ b/policycoreutils/setsebool/setsebool.c
>>>>> @@ -18,7 +18,7 @@
>>>>>    #include <errno.h>
>>>>>      int permanent = 0;
>>>>> -int reload = 1;
>>>>> +int no_reload = 0;
>>>>>    int verbose = 0;
>>>>>      int setbool(char **list, size_t start, size_t end);
>>>>> @@ -38,11 +38,6 @@ int main(int argc, char **argv)
>>>>>        if (argc < 2)
>>>>>            usage();
>>>>>    -    if (is_selinux_enabled() <= 0) {
>>>>> -        fputs("setsebool:  SELinux is disabled.\n", stderr);
>>>>> -        return 1;
>>>>> -    }
>>>>> -
>>>>>        while (1) {
>>>>>            clflag = getopt(argc, argv, "PNV");
>>>>>            if (clflag == -1)
>>>>> @@ -53,7 +48,7 @@ int main(int argc, char **argv)
>>>>>                permanent = 1;
>>>>>                break;
>>>>>            case 'N':
>>>>> -            reload = 0;
>>>>> +            no_reload = 1;
>>>>>                break;
>>>>>            case 'V':
>>>>>                verbose = 1;
>>>>> @@ -130,6 +125,7 @@ static int semanage_set_boolean_list(size_t
>>>>> boolcnt,
>>>>>        semanage_bool_key_t *bool_key = NULL;
>>>>>        int managed;
>>>>>        int result;
>>>>> +    int enabled = is_selinux_enabled();
>>>>>          handle = semanage_handle_create();
>>>>>        if (handle == NULL) {
>>>>> @@ -191,7 +187,7 @@ static int semanage_set_boolean_list(size_t
>>>>> boolcnt,
>>>>>                              boolean) < 0)
>>>>>                goto err;
>>>>>    -        if (semanage_bool_set_active(handle, bool_key,
>>>>> boolean) < 0) {
>>>>> +        if (enabled && semanage_bool_set_active(handle, bool_key,
>>>>> boolean) < 0) {
>>>>>                fprintf(stderr, "Failed to change boolean %s: %m\n",
>>>>>                    boollist[j].name);
>>>>>                goto err;
>>>>> @@ -202,7 +198,8 @@ static int semanage_set_boolean_list(size_t
>>>>> boolcnt,
>>>>>            boolean = NULL;
>>>>>        }
>>>>>    -    semanage_set_reload(handle, reload);
>>>>> +    if (no_reload)
>>>>> +        semanage_set_reload(handle, 0);
>>>>>        if (semanage_commit(handle) < 0)
>>>>>            goto err;
>