On 1/15/19 1:03 PM, Stephen Smalley wrote:
> On 1/15/19 11:22 AM, Petr Lautrbach wrote:
>> Stephen Smalley <sds@xxxxxxxxxxxxx> writes:
>>
>>> On 1/14/19 6:31 AM, Petr Lautrbach wrote:
>>>> Stephen Smalley <sds@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>>>>> ---
>>>>> 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;
>
