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; >>> >> >> >