On 1/17/19 9:00 AM, Stephen Smalley wrote:
> On 1/15/19 1:46 PM, Daniel Walsh wrote:
>> 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.
>
> Hmm...I don't quite understand that since libsemanage automatically
> disables policy reload if SELinux is disabled and since setsebool
> didn't work on SELinux-disabled systems prior to my patch (this
> thread). I don't really understand the point of -N for setsebool.
>
> In any event, my impression is that changing the semantics of -N to
> also suppress setting of active booleans would break the
> container-selinux.spec usage of setsebool and thus we probably
> shouldn't change it.
>
> Any reason to not go ahead and merge my patch enabling use of
> setsebool -P on SELinux-disabled hosts?
>
Nope, I think you can merge it.
>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> 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;
>>>
>>
>>
>
