[PATCH] libsemanage: do not change file mode of seusers and users

Discussion:

[PATCH] libsemanage: do not change file mode of seusers and users_extra

Vit Mojzis

2018-04-12 10:26:01 UTC

Commit 8702a865e08b5660561e194a83e4a363061edc03 causes file mode of
seusers and users_extra to change based on the value defined in config
file whenever direct_commit is called and policy is not rebuilt.
(e.g. when setting a boolean).

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1512639

$ ll /var/lib/selinux/targeted/active/users_extra
-rw-------. 1 root root 101 11. dub 17.31 /var/lib/selinux/targeted/active/users_extra
$ ll /var/lib/selinux/targeted/active/seusers
-rw-------. 1 root root 73 11. dub 17.31 /var/lib/selinux/targeted/active/seusers
$ semanage boolean -m --on httpd_can_network_connect
$ ll /var/lib/selinux/targeted/active/seusers
-rw-r--r--. 1 root root 73 23. bře 16.59 /var/lib/selinux/targeted/active/seusers
$ ll /var/lib/selinux/targeted/active/users_extra
-rw-r--r--. 1 root root 101 23. bře 16.59 /var/lib/selinux/targeted/active/users_extra
$ rpm -Vq selinux-policy-targeted
.M.....T. /var/lib/selinux/targeted/active/seusers
.M.....T. /var/lib/selinux/targeted/active/users_extra

Signed-off-by: Vit Mojzis <***@redhat.com>
---
libsemanage/src/direct_api.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index e7ec952f..c58961be 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -1481,7 +1481,7 @@ rebuild:
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
- sh->conf->file_mode);
+ 0);
if (retval < 0)
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
@@ -1499,7 +1499,7 @@ rebuild:
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
- sh->conf->file_mode);
+ 0);
if (retval < 0)
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);

--
2.14.3

Stephen Smalley

2018-04-12 15:07:04 UTC

Permalink

Post by Vit Mojzis
Commit 8702a865e08b5660561e194a83e4a363061edc03 causes file mode of
seusers and users_extra to change based on the value defined in config
file whenever direct_commit is called and policy is not rebuilt.
(e.g. when setting a boolean).
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1512639

I think this patch is correct and expect to apply it, but am left wondering about the permissions
on /var/lib/selinux/targeted in general. It appears that we are inconsistent in our file modes
on files under /var/lib/selinux/targeted/active, e.g. file_contexts.homedirs, *.local, and modules/*/* are 0644,
whereas other files are 0600. Of course, given that the directories are 0600, only root can even lookup files under
these directories regardless of their individual file modes so it isn't as though those files are truly accessible.
Looks like there are other uses of sh->conf->file_mode that are suspect in semanage_direct_commit() for files
in the store, whereas I think it should only be used for installed files (i.e. /etc/selinux/targeted/*).

Post by Vit Mojzis
$ ll /var/lib/selinux/targeted/active/users_extra
-rw-------. 1 root root 101 11. dub 17.31 /var/lib/selinux/targeted/active/users_extra
$ ll /var/lib/selinux/targeted/active/seusers
-rw-------. 1 root root 73 11. dub 17.31 /var/lib/selinux/targeted/active/seusers
$ semanage boolean -m --on httpd_can_network_connect
$ ll /var/lib/selinux/targeted/active/seusers
-rw-r--r--. 1 root root 73 23. bře 16.59 /var/lib/selinux/targeted/active/seusers
$ ll /var/lib/selinux/targeted/active/users_extra
-rw-r--r--. 1 root root 101 23. bře 16.59 /var/lib/selinux/targeted/active/users_extra
$ rpm -Vq selinux-policy-targeted
.M.....T. /var/lib/selinux/targeted/active/seusers
.M.....T. /var/lib/selinux/targeted/active/users_extra
---
libsemanage/src/direct_api.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index e7ec952f..c58961be 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
- sh->conf->file_mode);
+ 0);
if (retval < 0)
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
- sh->conf->file_mode);
+ 0);
if (retval < 0)
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);

Stephen Smalley

2018-04-12 17:22:40 UTC

Permalink

Post by Stephen Smalley

Actually, we seem to be inconsistent even among different modules; some seem to be 0600 and others 0644, likely due
to some being prebuilt/prepackaged that way and others installed via semodule -i. Also, policy.kern and policy.linked are presently 0644.

On a separate but related note, rpm -V selinux-policy-targeted output seems somewhat surprising, e.g. wouldn't expect file_contexts.local, commit_num, etc to be managed by rpm itself. Not sure it should be managing /var/lib/selinux at all.

Post by Stephen Smalley

Petr Lautrbach

2018-04-12 20:03:30 UTC

Permalink

Post by Stephen Smalley

Actually, we seem to be inconsistent even among different modules; some seem to be 0600 and others 0644, likely due
to some being prebuilt/prepackaged that way and others installed via semodule -i. Also, policy.kern and policy.linked are presently 0644.
On a separate but related note, rpm -V selinux-policy-targeted output seems somewhat surprising, e.g. wouldn't expect file_contexts.local, commit_num, etc to be managed by rpm itself. Not sure it should be managing /var/lib/selinux at all.

Note that /etc/selinux/targeted/modules/active was part of selinux-policy-targeted since 2011.

file_contexts.local is in /etc/selinux and is shipped with %config(noreplace). It means it's preserved during updates and
`rpm -qf /etc/selinux/targeted/contexts/files/file_contexts.local` shows the relevant package.

The other files showed by `rpm -V` are probably not necessary to be included in the package.

As far as I know we need to ship the SELinux store in /var/lib/selinux as whole for systems using OSTree where packages
are not installed, i.e. post installation scripts are not run, but they are just extracted to a filesystem.

Post by Stephen Smalley

Post by Vit Mojzis
$ ll /var/lib/selinux/targeted/active/users_extra
-rw-------. 1 root root 101 11.Â dub 17.31 /var/lib/selinux/targeted/active/users_extra
$ ll /var/lib/selinux/targeted/active/seusers
-rw-------. 1 root root 73 11.Â dub 17.31 /var/lib/selinux/targeted/active/seusers
$ semanage boolean -m --on httpd_can_network_connect
$ ll /var/lib/selinux/targeted/active/seusers
-rw-r--r--. 1 root root 73 23.Â bÅe 16.59 /var/lib/selinux/targeted/active/seusers
$ ll /var/lib/selinux/targeted/active/users_extra
-rw-r--r--. 1 root root 101 23.Â bÅe 16.59 /var/lib/selinux/targeted/active/users_extra
$ rpm -Vq selinux-policy-targeted
.M.....T. /var/lib/selinux/targeted/active/seusers
.M.....T. /var/lib/selinux/targeted/active/users_extra
---
libsemanage/src/direct_api.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index e7ec952f..c58961be 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
- sh->conf->file_mode);
+ 0);
if (retval < 0)
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
- sh->conf->file_mode);
+ 0);
if (retval < 0)
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);

Stephen Smalley

2018-04-12 20:31:32 UTC

Permalink

Post by Petr Lautrbach

Post by Stephen Smalley

Actually, we seem to be inconsistent even among different modules; some seem to be 0600 and others 0644, likely due
to some being prebuilt/prepackaged that way and others installed via semodule -i. Also, policy.kern and policy.linked are presently 0644.
On a separate but related note, rpm -V selinux-policy-targeted output seems somewhat surprising, e.g. wouldn't expect file_contexts.local, commit_num, etc to be managed by rpm itself. Not sure it should be managing /var/lib/selinux at all.

Note that /etc/selinux/targeted/modules/active was part of selinux-policy-targeted since 2011.
file_contexts.local is in /etc/selinux and is shipped with %config(noreplace). It means it's preserved during updates and
`rpm -qf /etc/selinux/targeted/contexts/files/file_contexts.local` shows the relevant package.
The other files showed by `rpm -V` are probably not necessary to be included in the package.
As far as I know we need to ship the SELinux store in /var/lib/selinux as whole for systems using OSTree where packages
are not installed, i.e. post installation scripts are not run, but they are just extracted to a filesystem.

Don't quite understand why you'd ship a file_contexts.local at all; any file contexts you want to ship in the base policy can just go in a policy module. Also not sure why setrans.conf is part of selinux-policy-targeted instead of mcstrans.

Understand why you are packaging /var/lib/selinux, just wondering if they should be verified or not as part of rpm -V.