* [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts
@ 2019-01-10 16:26 Stephen Smalley
2019-01-12 18:56 ` Nicolas Iooss
2019-01-14 11:31 ` Petr Lautrbach
0 siblings, 2 replies; 10+ messages in thread
From: Stephen Smalley @ 2019-01-10 16:26 UTC (permalink / raw)
To: selinux; +Cc: jwcart2, Stephen Smalley
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.
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;
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts
2019-01-10 16:26 [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts Stephen Smalley
@ 2019-01-12 18:56 ` Nicolas Iooss
2019-01-14 11:31 ` Petr Lautrbach
1 sibling, 0 replies; 10+ messages in thread
From: Nicolas Iooss @ 2019-01-12 18:56 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, James Carter
On Thu, Jan 10, 2019 at 5:24 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> 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.
>
> 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.
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> 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;
>
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts
2019-01-10 16:26 [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts Stephen Smalley
2019-01-12 18:56 ` Nicolas Iooss
@ 2019-01-14 11:31 ` Petr Lautrbach
2019-01-15 13:28 ` Stephen Smalley
1 sibling, 1 reply; 10+ messages in thread
From: Petr Lautrbach @ 2019-01-14 11:31 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, jwcart2
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.
> 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;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts
2019-01-14 11:31 ` Petr Lautrbach
@ 2019-01-15 13:28 ` Stephen Smalley
2019-01-15 16:22 ` Petr Lautrbach
0 siblings, 1 reply; 10+ messages in thread
From: Stephen Smalley @ 2019-01-15 13:28 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: selinux, jwcart2
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.
>
>
>
>> 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;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts
2019-01-15 13:28 ` Stephen Smalley
@ 2019-01-15 16:22 ` Petr Lautrbach
2019-01-15 18:03 ` Stephen Smalley
0 siblings, 1 reply; 10+ messages in thread
From: Petr Lautrbach @ 2019-01-15 16:22 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, jwcart2
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.
>
>>
>>
>>
>>> 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;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts
2019-01-15 16:22 ` Petr Lautrbach
@ 2019-01-15 18:03 ` Stephen Smalley
2019-01-15 18:46 ` Daniel Walsh
0 siblings, 1 reply; 10+ messages in thread
From: Stephen Smalley @ 2019-01-15 18:03 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: selinux, jwcart2, Daniel J Walsh
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.
>
>
>>
>>>
>>>
>>>
>>>> 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;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts
2019-01-15 18:03 ` Stephen Smalley
@ 2019-01-15 18:46 ` Daniel Walsh
2019-01-17 14:00 ` Stephen Smalley
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Walsh @ 2019-01-15 18:46 UTC (permalink / raw)
To: Stephen Smalley, Petr Lautrbach; +Cc: selinux, jwcart2
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;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts
2019-01-15 18:46 ` Daniel Walsh
@ 2019-01-17 14:00 ` Stephen Smalley
2019-01-17 15:10 ` Daniel Walsh
0 siblings, 1 reply; 10+ messages in thread
From: Stephen Smalley @ 2019-01-17 14:00 UTC (permalink / raw)
To: dwalsh, Petr Lautrbach; +Cc: selinux, jwcart2
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@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.
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?
>>
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>> 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;
>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts
2019-01-17 14:00 ` Stephen Smalley
@ 2019-01-17 15:10 ` Daniel Walsh
2019-01-18 16:13 ` Stephen Smalley
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Walsh @ 2019-01-17 15:10 UTC (permalink / raw)
To: Stephen Smalley, Petr Lautrbach; +Cc: selinux, jwcart2
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@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.
>
> 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@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;
>>>
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts
2019-01-17 15:10 ` Daniel Walsh
@ 2019-01-18 16:13 ` Stephen Smalley
0 siblings, 0 replies; 10+ messages in thread
From: Stephen Smalley @ 2019-01-18 16:13 UTC (permalink / raw)
To: dwalsh, Petr Lautrbach; +Cc: selinux, jwcart2
On 1/17/19 10:10 AM, Daniel Walsh wrote:
> 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@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.
>>
>> 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.
FYI, I merged this via
https://github.com/SELinuxProject/selinux/pull/129
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> 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;
>>>>
>>>
>>>
>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-01-18 16:11 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-10 16:26 [PATCH v2] setsebool: support use of -P on SELinux-disabled hosts Stephen Smalley
2019-01-12 18:56 ` Nicolas Iooss
2019-01-14 11:31 ` Petr Lautrbach
2019-01-15 13:28 ` Stephen Smalley
2019-01-15 16:22 ` Petr Lautrbach
2019-01-15 18:03 ` Stephen Smalley
2019-01-15 18:46 ` Daniel Walsh
2019-01-17 14:00 ` Stephen Smalley
2019-01-17 15:10 ` Daniel Walsh
2019-01-18 16:13 ` Stephen Smalley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).