sepgsql seems rather thoroughly broken on Fedora 30

I wrote:

I tried to run the contrib/sepgsql tests, following the instructions,
on a recently-set-up Fedora 30 machine. I've done that successfully
on previous Fedora releases, but it's no go with F30.
...
I'm pretty sure the test recipe last worked for me on F28.
Off to try F29.

On Fedora 29, compiling the policy file spews what look like exactly
the same "errors". Everything after that works.

I don't have a functioning F28 installation right now, so I can't
double-check whether the errors appear on that.

regards, tom lane

On 7/17/19 12:54 PM, Tom Lane wrote:

I wrote:

I tried to run the contrib/sepgsql tests, following the instructions,
on a recently-set-up Fedora 30 machine. I've done that successfully
on previous Fedora releases, but it's no go with F30.
...
I'm pretty sure the test recipe last worked for me on F28.
Off to try F29.

On Fedora 29, compiling the policy file spews what look like exactly
the same "errors". Everything after that works.

I don't have a functioning F28 installation right now, so I can't
double-check whether the errors appear on that.

Thanks for the report -- Mike Palmiotto said he would take a look as
soon as he can.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Wed, Jul 17, 2019 at 12:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I tried to run the contrib/sepgsql tests, following the instructions,
on a recently-set-up Fedora 30 machine. I've done that successfully
on previous Fedora releases, but it's no go with F30.

First off, building the sepgsql-regtest.pp policy file spews
a bunch of complaints that I don't recall having seen before:

$ make -f /usr/share/selinux/devel/Makefile
/usr/share/selinux/devel/include/services/container.if:14: Error: duplicate definition of container_runtime_domtrans(). Original definition on 14.
/usr/share/selinux/devel/include/services/container.if:41: Error: duplicate definition of container_runtime_run(). Original definition on 41.
/usr/share/selinux/devel/include/services/container.if:61: Error: duplicate definition of container_runtime_exec(). Original definition on 61.
/usr/share/selinux/devel/include/services/container.if:80: Error: duplicate definition of container_read_state(). Original definition on 80.
... more of the same ...
/usr/share/selinux/devel/include/services/container.if:726: Error: duplicate definition of docker_stream_connect(). Original definition on 726.
/usr/share/selinux/devel/include/services/container.if:730: Error: duplicate definition of docker_spc_stream_connect(). Original definition on 730.
/usr/share/selinux/devel/include/services/container.if:744: Error: duplicate definition of container_spc_read_state(). Original definition on 744.
/usr/share/selinux/devel/include/services/container.if:763: Error: duplicate definition of container_domain_template(). Original definition on 763.
/usr/share/selinux/devel/include/services/container.if:791: Error: duplicate definition of container_spc_rw_pipes(). Original definition on 791.

These errors are due to a conflict between "container-selinux" and
"selinux-policy-devel." With both packages installed, you will see the
container interface file in both
/usr/share/selinux/devel/include/contrib and
/usr/share/selinux/devel/include/services:

% sudo find /usr/share/selinux/devel -type f -name "container.if"
/usr/share/selinux/devel/include/contrib/container.if
/usr/share/selinux/devel/include/services/container.if

This is likely a bug that should be fixed by "container-selinux." I'll
see what I can do about getting that fixed upstream.
As you noted, the build errors are likely a red herring, since the .pp
still installs and the test script recognizes the module as loaded. If
you want to get rid of these for now and you aren't particularly
concerned about your container policy module, you can just uninstall
the "container-selinux" package.

============== checking selinux environment ==============
checking for matchpathcon ... ok
checking for runcon ... ok
checking for sestatus ... ok
checking current user domain ... unconfined_t
checking selinux operating mode ... enforcing
checking for sepgsql-regtest policy ... ok
checking whether policy is enabled ... on
on
checking whether we can run psql ... failed

<snip>
(BTW, what's that extra "on" after "checking whether policy is enabled"?)

The second "on" is from the `getsebool sepgsql_enable_users_ddl`
check, which has no associated "checking policy boolean" message.
We'll minimally want to add more specific messages for the two
`getsebool` checks.

$ runcon -t sepgsql_regtest_user_t psql --help
psql: fatal: could not look up effective user ID 1000: user does not exist

But uid 1000 is me according to /etc/passwd and according to "id":

$ id
uid=1000(tgl) gid=1000(tgl) groups=1000(tgl),10(wheel) context=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023

so there's nothing much wrong with having that as euid.

I speculate that the policy is forbidding sepgsql_regtest_user_t
from reading /etc/passwd. Perhaps this is fallout from the
compile problems reported for the policy module? But I'm way
out of my depth here.

I wonder what your password file is labeled. It ought to be:
% ls -Z /etc/passwd
system_u:object_r:passwd_file_t:s0 /etc/passwd

The sepgsql_regtest_user_t domain should be allowed to read any file
labeled "passwd_file_t". We can check that with the `sesearch` tool,
provided by the "setools-console" package on F30:

% sudo sesearch -A -s sepgsql_regtest_user_t -t passwd_file_t
allow domain file_type:blk_file map; [ domain_can_mmap_files ]:True
allow domain file_type:chr_file map; [ domain_can_mmap_files ]:True
allow domain file_type:file map; [ domain_can_mmap_files ]:True
allow nsswitch_domain passwd_file_t:file { getattr ioctl lock map open read };

If your /etc/passwd label is not correct, you can try just running
`restorecon -RF /` to fix it.

In any case, it looks like this entire test script and policy could
use another layer of varnish, so I'll work on fixing up the
messages/functionality and post a patch which makes this a bit more
robust (hopefully a bit later tonight).
Sorry for the delayed response. Hopefully the band-aid fixes I
provided get you going for now.

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

Mike Palmiotto <mike.palmiotto@crunchydata.com> writes:

On Wed, Jul 17, 2019 at 12:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

$ runcon -t sepgsql_regtest_user_t psql --help
psql: fatal: could not look up effective user ID 1000: user does not exist

I wonder what your password file is labeled. It ought to be:
% ls -Z /etc/passwd
system_u:object_r:passwd_file_t:s0 /etc/passwd

Good thought, but no cigar:

$ ls -Z /etc/passwd
system_u:object_r:passwd_file_t:s0 /etc/passwd

Happy to poke at anything else you can suggest.

regards, tom lane

On Thu, Jul 18, 2019 at 11:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Mike Palmiotto <mike.palmiotto@crunchydata.com> writes:

On Wed, Jul 17, 2019 at 12:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

$ runcon -t sepgsql_regtest_user_t psql --help
psql: fatal: could not look up effective user ID 1000: user does not exist

You can rule out SELinux for this piece by running `sudo setenforce
0`. If the `runcon ... psql` command works in Permissive we should
look at your audit log to determine what is being denied. audit2allow
will provide a summary of the SELinux denials and is generally a good
starting point:

# grep denied /var/log/audit/audit.log | audit2allow

If SELinux is indeed the issue here and you want to avoid doing all of
this detective work, it may be a good idea to just run a system-wide
restorecon (assuming you didn't already do that before) to make sure
your labels are in a decent state.

FWIW, this appears to be working on my recently-installed F30 VM:

% runcon -t sepgsql_regtest_user_t psql --help &> /dev/null
% echo $?
0

Hopefully a system-wide `restorecon` just magically fixes this for
you. Otherwise, we can start digging into denials.

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

Mike Palmiotto <mike.palmiotto@crunchydata.com> writes:

On Thu, Jul 18, 2019 at 11:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

$ runcon -t sepgsql_regtest_user_t psql --help
psql: fatal: could not look up effective user ID 1000: user does not exist

You can rule out SELinux for this piece by running `sudo setenforce
0`. If the `runcon ... psql` command works in Permissive we should
look at your audit log to determine what is being denied. audit2allow
will provide a summary of the SELinux denials and is generally a good
starting point:

# grep denied /var/log/audit/audit.log | audit2allow

It's definitely SELinux. The grep finds entries like

type=AVC msg=audit(1563547268.044:465): avc: denied { read } for pid=10940 comm="psql" name="passwd" dev="sda6" ino=4721184 scontext=unconfined_u:unconfined_r:sepgsql_regtest_user_t:s0-s0:c0.c1023 tcontext=system_u:object_r:passwd_file_t:s0 tclass=file permissive=0

which audit2allow turns into

#============= sepgsql_regtest_user_t ==============
allow sepgsql_regtest_user_t passwd_file_t:file read;

So somehow, my system's interpretation of the test policy file does
not include that permission.

I tried:

* restorecon / ... no effect, which is unsurprising given that /etc/passwd
was OK already.

* removing container-selinux ... this made the compile warnings go away,
as you predicted, but no change in the test results.

FWIW, this appears to be working on my recently-installed F30 VM:

% runcon -t sepgsql_regtest_user_t psql --help &> /dev/null
% echo $?
0

Well, that's just weird. I've not done anything to the SELinux state
on this installation either, so what's different?

I am wondering whether maybe the different behavior is a result of some
RPM that's present on my system but not yours, or vice versa. As
a first stab at that, I see:

$ rpm -qa | grep selinux | sort
cockpit-selinux-198-1.fc30.noarch
container-selinux-2.107-1.git453b816.fc30.noarch
flatpak-selinux-1.4.2-2.fc30.x86_64
libselinux-2.9-1.fc30.x86_64
libselinux-devel-2.9-1.fc30.x86_64
libselinux-utils-2.9-1.fc30.x86_64
python3-libselinux-2.9-1.fc30.x86_64
rpm-plugin-selinux-4.14.2.1-4.fc30.1.x86_64
selinux-policy-3.14.3-40.fc30.noarch
selinux-policy-devel-3.14.3-40.fc30.noarch
selinux-policy-targeted-3.14.3-40.fc30.noarch
tpm2-abrmd-selinux-2.0.0-4.fc30.noarch

regards, tom lane

Mike Palmiotto <mike.palmiotto@crunchydata.com> writes:

The sepgsql_regtest_user_t domain should be allowed to read any file
labeled "passwd_file_t". We can check that with the `sesearch` tool,
provided by the "setools-console" package on F30:

% sudo sesearch -A -s sepgsql_regtest_user_t -t passwd_file_t
allow domain file_type:blk_file map; [ domain_can_mmap_files ]:True
allow domain file_type:chr_file map; [ domain_can_mmap_files ]:True
allow domain file_type:file map; [ domain_can_mmap_files ]:True
allow nsswitch_domain passwd_file_t:file { getattr ioctl lock map open read };

I got around to trying this, and lookee here:

$ sudo sesearch -A -s sepgsql_regtest_user_t -t passwd_file_t
allow domain file_type:blk_file map; [ domain_can_mmap_files ]:True
allow domain file_type:chr_file map; [ domain_can_mmap_files ]:True
allow domain file_type:file map; [ domain_can_mmap_files ]:True
allow domain file_type:lnk_file map; [ domain_can_mmap_files ]:True

Nothing about passwd_file_t. So *something* is different about the
way the policy is being expanded.

regards, tom lane

On Fri, Jul 19, 2019 at 11:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I got around to trying this, and lookee here:

$ sudo sesearch -A -s sepgsql_regtest_user_t -t passwd_file_t
allow domain file_type:blk_file map; [ domain_can_mmap_files ]:True
allow domain file_type:chr_file map; [ domain_can_mmap_files ]:True
allow domain file_type:file map; [ domain_can_mmap_files ]:True
allow domain file_type:lnk_file map; [ domain_can_mmap_files ]:True

Nothing about passwd_file_t. So *something* is different about the
way the policy is being expanded.

Okay, I was finally able to replicate the issue (and fix it). It looks
like perhaps the userdom_base_user_template changed and no longer
allows reading of passwd_file_t? At any rate, I added some policy to
ensure that we have the proper permissions.

I also beefed up the test script a bit so it now:
- installs the SELinux policy module
- spins up a temporary cluster to muddy postgresql.conf and run the
setup sql in an isolated environment

We probably need to polish this a bit more, but what do you think
about something similar to the attached patches? They should hopefully
reduce some of the complexity of running these regression tests.

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

Mike Palmiotto <mike.palmiotto@crunchydata.com> writes:

We probably need to polish this a bit more, but what do you think
about something similar to the attached patches? They should hopefully
reduce some of the complexity of running these regression tests.

I can confirm that the 0001 patch fixes things on my Fedora 30 box.
So that's good, though I don't know enough to evaluate it for style
or anything like that.

I don't think I like the 0002 patch very much, because of its putting
all the sudo actions into the script. I'd rather not give a script
root permissions, thanks. Maybe I'm in the minority on that.
Also, since the documentation explicitly says that the
/usr/share/selinux/devel/Makefile path is not to be relied on,
why would we hard-wire it into the script?

A bigger-picture issue is that right now, configuring a cluster for
sepgsql is a very manual process (cf. section F.35.2). I think there's
some advantage in forcing the user to run through that before running
the regression test, namely that they'll get the bugs out of any
misunderstandings or needed local changes. If we had that a bit more
automated then maybe having the test script do-it-for-you would be
sensible. (IOW, the fact that the test process is more like "make
installcheck" than "make check" seems like a feature not a bug.)

regards, tom lane

On Fri, Jul 19, 2019 at 4:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Mike Palmiotto <mike.palmiotto@crunchydata.com> writes:

We probably need to polish this a bit more, but what do you think
about something similar to the attached patches? They should hopefully
reduce some of the complexity of running these regression tests.

I can confirm that the 0001 patch fixes things on my Fedora 30 box.
So that's good, though I don't know enough to evaluate it for style
or anything like that.

I think the policy is in need of review/rewriting anyway. The proper
thing to do would be to create a common template for all of the
SELinux regtest user domains and create more of a hierarchical policy
to reduce redundancy. If you want to wait for more formal policy
updates, I can do that in my spare time. Otherwise, the patch I posted
should work with the general style of this policy module.

I don't think I like the 0002 patch very much, because of its putting
all the sudo actions into the script. I'd rather not give a script
root permissions, thanks. Maybe I'm in the minority on that.

Definitely not. I cringed a little bit as I was making those
additions, but figured it was fine since it's just a test script (and
we have to run `sudo` for various other installation items as well).

Also, since the documentation explicitly says that the
/usr/share/selinux/devel/Makefile path is not to be relied on,
why would we hard-wire it into the script?

A bigger-picture issue is that right now, configuring a cluster for
sepgsql is a very manual process (cf. section F.35.2). I think there's
some advantage in forcing the user to run through that before running
the regression test, namely that they'll get the bugs out of any
misunderstandings or needed local changes. If we had that a bit more
automated then maybe having the test script do-it-for-you would be
sensible. (IOW, the fact that the test process is more like "make
installcheck" than "make check" seems like a feature not a bug.)

Makes sense to me. Thanks for the feedback!

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

Mike Palmiotto <mike.palmiotto@crunchydata.com> writes:

On Fri, Jul 19, 2019 at 4:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I can confirm that the 0001 patch fixes things on my Fedora 30 box.
So that's good, though I don't know enough to evaluate it for style
or anything like that.

I think the policy is in need of review/rewriting anyway. The proper
thing to do would be to create a common template for all of the
SELinux regtest user domains and create more of a hierarchical policy
to reduce redundancy. If you want to wait for more formal policy
updates, I can do that in my spare time. Otherwise, the patch I posted
should work with the general style of this policy module.

Hearing no further comments, I went ahead and pushed 0001 (after
checking that it works on F28, which is the oldest Fedora version
I have at hand right now). Stylistic improvements to the script
are fine, but let's get the bug fixed for now.

BTW, I noticed that the documentation about how to run the tests
is a bit stale as well --- for instance, it says to use

$ sudo semodule -u sepgsql-regtest.pp

but that slaps your wrist:

The --upgrade option is deprecated. Use --install instead.

So if anyone does feel like polishing things in this area, some doc
review seems indicated.

regards, tom lane