Use semodule -lfull --checksum instead of the datastore

[bachradsusi]

SELinux userspace release 3.4 introduced a new command line option [-m|--checksum] to semodule which adds sha256 checksum of modules to its output. It can be used to check whether the same module is already installed or not. Given that selinuxd installed modules use priority 350 we can use semodule checksum and priority 350 as an indicator whether a module was already installed by selinuxd or not and therefore there's no need to track the state of modules in a separate datastore.

semodule --checksum is supported since Red Hat Enterprise Linux 8.6

Signed-off-by: Petr Lautrbach lautrbach@redhat.com

[jhrozek]

thanks I'll review this week (unless someone else is quicker)

I think the 8.6 requirement is fine, I'm not aware of anyone using selinuxd on other distros and OCP is using 8.6 these days.

[JAORMX]

@bachradsusi are there any plans on having the semodule utilities be actual standalone library components? Currently they all depend on the binaries being there and that's not ideal. I'd much rather have selinuxd make function calls instead of calling binaries that have to exist in the container/OS.

That being said, I think this is a good approach. Let's make sure this PR passes the linting and tests.

[bachradsusi]

Currently, there's no such plan. Back in January I was experiencing with a patch which would allow to configure libsemanage so that it would not exec load_policy, setfiles and sefcontext_compile - SELinuxProject/selinux@3320e44 As it's described in the commit message it's even possible right now but it's kind of a hack and requires a system configuration change.

The reason for using exec() on these tools is to allow policy admin to specify transitions to domains which are allowed to manage selinux, e.g. useradd doesn't need to be allowed to use 'load_policy' permission on 'security' class because useradd_t domain is allowed to transition to load_policy_t domain via exec() on /sbin/load_policy

[bachradsusi]

The patch is obviously incomplete. Please wait with the review.

[jhrozek]

btw even version bumpgs trigger CI failures these days, we gotta look into that first I guess: https://github.com/containers/selinuxd/actions/runs/3589427595/jobs/6041847578

[jhrozek]

(in case that wasn't clear, that link is from another PR)

[bachradsusi]

I believe this is a false positive.

[bachradsusi]

Could you please take a look at failing test and explain me what is wrong?

[JAORMX]

The reason for using exec() on these tools is to allow policy admin to specify transitions to domains which are allowed to manage selinux, e.g. useradd doesn't need to be allowed to use 'load_policy' permission on 'security' class because useradd_t domain is allowed to transition to load_policy_t domain via exec() on /sbin/load_policy

The reason makes a lot of sense. I wonder though if it would be possible to specify a macro to make the functionality optional. I understand why it would be a lot easier to maintain and use making binaries like useradd exec and thus not require load_policy permissions. But for utilities like selinuxd, it would be relevant for us to explicitly give it all the permissions needed. So... having a macro we can set for libsemanage to compile and not exec would be pretty neat! just an idea.

[bachradsusi]

The reason for using exec() on these tools is to allow policy admin to specify transitions to domains which are allowed to manage selinux, e.g. useradd doesn't need to be allowed to use 'load_policy' permission on 'security' class because useradd_t domain is allowed to transition to load_policy_t domain via exec() on /sbin/load_policy

The reason makes a lot of sense. I wonder though if it would be possible to specify a macro to make the functionality optional. I understand why it would be a lot easier to maintain and use making binaries like useradd exec and thus not require load_policy permissions. But for utilities like selinuxd, it would be relevant for us to explicitly give it all the permissions needed. So... having a macro we can set for libsemanage to compile and not exec would be pretty neat! just an idea.

I believe this should be already feasible without changing libsemanage. Given that it usually runs in its own image, the image could be built with modified semanage.conf.

E.g. to avoid exec() on load_policy:

1. add the following to /etc/selinux/semanage.conf

    [load_policy]
    path = /bin/true
    [end]

2. update selinuxd to call selinux_mkload_policy(0) after semodule -i ..., see SELinuxProject/selinux@3320e44#diff-3c368e8a1e1c8f6d6fd4b7072dbb8617601fccf448e8f0bd2350957bdd4f62d2R1484

[JAORMX]

I believe this should be already feasible without changing libsemanage. Given that it usually runs in its own image, the image could be built with modified semanage.conf.

this is already a little tricky since we mount the host's /etc/selinux directory given that we need access to several things from that directory. Mounting things individually would complicate the deployment even further 😢

[bachradsusi]

I believe this should be already feasible without changing libsemanage. Given that it usually runs in its own image, the image could be built with modified semanage.conf.

this is already a little tricky since we mount the host's /etc/selinux directory given that we need access to several things from that directory. Mounting things individually would complicate the deployment even further cry

I knew I forgot about some important detail.

[JAORMX]

@bachradsusi seems to me that the tests themselves is broken which is an issue unrelated to this PR 😕 this is happening also for simple dependency upgrades

[bachradsusi]

The reason makes a lot of sense. I wonder though if it would be possible to specify a macro to make the functionality optional. I understand why it would be a lot easier to maintain and use making binaries like useradd exec and thus not require load_policy permissions. But for utilities like selinuxd, it would be relevant for us to explicitly give it all the permissions needed. So... having a macro we can set for libsemanage to compile and not exec would be pretty neat! just an idea.

So it's possible to avoid execve() on load_policy and setfiles - both from policycoreutils. There are semanage_set_reload(sh, 0) and semanage_set_check_contexts(sh, 0) which suppress execing on these binaries. See bellow for POC.

# cat > /etc/selinux.d/xyz.cil <<EOF
(allow sshd_t init_t (file (execute)))
EOF

# cat > install_module.py <<EOF
import os
import semanage
import selinux

sh = semanage.semanage_handle_create()
semanage.semanage_connect(sh)
semanage.semanage_set_reload(sh, 0)
semanage.semanage_set_check_contexts(sh, 0)
semanage.semanage_set_default_priority(sh, 500)
semanage.semanage_module_install_file(sh, "/etc/selinux.d/xyz.cil")
semanage.semanage_commit(sh)

os.system("cat /etc/selinux.d/xyz.cil")
print("after semanage.semanage_set_reload(sh, 0)")
os.system("sesearch -A -s sshd_t -t init_t -c file -p execute")
print()
selinux.selinux_mkload_policy(1)

print("after selinux.selinux_mkload_policy(1)")
os.system("sesearch -A -s sshd_t -t init_t -c file -p execute")
EOF

# strace -o log -ff python3 install_module.py 
(allow sshd_t init_t (file (execute)))

after semanage.semanage_set_reload(sh, 0)

after selinux.selinux_mkload_policy(1)
allow sshd_t init_t:file execute;

# grep execve log
403785 execve("/usr/bin/python3", ["python3", "install_module.py"], 0x7ffec29a5e90 /* 37 vars */) = 0
403789 execve("/usr/sbin/sefcontext_compile", ["/usr/sbin/sefcontext_compile", "-r", "/var/lib/selinux/final/targeted/"...], NULL <unfinished ...>
403789 <... execve resumed>)            = 0
403790 execve("/usr/sbin/sefcontext_compile", ["/usr/sbin/sefcontext_compile", "-r", "/var/lib/selinux/final/targeted/"...], NULL <unfinished ...>
403790 <... execve resumed>)            = 0
403791 execve("/usr/sbin/sefcontext_compile", ["/usr/sbin/sefcontext_compile", "-r", "/var/lib/selinux/final/targeted/"...], NULL <unfinished ...>
...

[jhrozek]

@bachradsusi can you review #83 so I can merge it and then rebase this PR on top of that? I think #83 will make the tests pass again.

[bachradsusi]

Rebased. But if I read #83 correctly, it changes just github actions while tests fail also running make test. It seems to be related to this PR but I wasn't able to find the reason and I still don't completely understand how these test works.

[jhrozek]

Rebased. But if I read #83 correctly, it changes just github actions while tests fail also running make test. It seems to be related to this PR but I wasn't able to find the reason and I still don't completely understand how these test works.

Hey, sorry for the delay in review. I think the tests are wrong and relied on the DS being there and in particular the first install returning an error from the DS put. Now that doesn't happen, we always get back a mock sha from testhandler.go which always returns the sha so we always think the module was installed already.

I'll adjust the tests (hopefully) tomorrow.

[bachradsusi]

Thanks for the hint.

I've fixed func (smt *SEModuleTestHandler) GetPolicyModule(moduleName string) so that it doesn't return a module when it's not in the module list.

The current version is without "Should skip policy installation if it's already installed" as it uses datastore. All other tests passed.

[jhrozek]

Thanks for the hint.

I've fixed func (smt *SEModuleTestHandler) GetPolicyModule(moduleName string) so that it doesn't return a module when it's not in the module list.

The current version is without "Should skip policy installation if it's already installed" as it uses datastore. All other tests passed.

Great, thank you. I'll try to get to the PR when I'm back from the Christmas break. Sorry about the constant delays - we had several SPO blockers that took way longer to resolve than I expected.