Bug 1319652 - selinux-policy build never checks assertions
Summary: selinux-policy build never checks assertions
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: libsemanage
Version: 26
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: ---
Assignee: Petr Lautrbach
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1177994
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-03-21 09:29 UTC by Miroslav Grepl
Modified: 2018-04-01 19:06 UTC (History)
9 users (show)

Fixed In Version: libsemanage-2.5-3.fc25 libsemanage-2.7-12.fc28
Doc Type: Bug Fix
Doc Text:
Clone Of: 1177994
Environment:
Last Closed: 2018-04-01 19:06:07 UTC
Type: Bug


Attachments (Terms of Use)

Description Miroslav Grepl 2016-03-21 09:29:54 UTC
+++ This bug was initially created as a clone of Bug #1177994 +++

Description of problem:

Back around 2008, semanage.conf was changed to not check policy assertions during load, by setting expand-check=0. The reasoning[1] was that it could be done instead during policy build with a 'make validate' step, because the process takes a long time.

In this commit[2] in 2012, that step in the build was silently changed to add:

make validate [...] SEMOD_EXP="/usr/bin/semodule_expand -a"

The problem is that the '-a' option means "don't check assertions"[3]. As far as I can tell, they haven't been checked at build or load time since f18.

It takes an incredibly long time to run (I'm at 40min and still going). Running it over a reduced policy in a different environment reveals there are bugs hidden here:

 libsepol.check_assertion_helper: neverallow violated by allow restorecond_t semanage_store_t:file { relabelto };
 (and more, but it's only showing one error at a time)

[1] http://www.spinics.net/lists/selinux/msg03931.html
[2] https://lists.fedoraproject.org/pipermail/scm-commits/2012-September/866834.html
[3] https://github.com/fedora-selinux/selinux/blob/master/policycoreutils/semodule_expand/semodule_expand.c#L79

--- Additional comment from John Brooks on 2015-01-01 04:35:13 EST ---

I gave up after about an hour of waiting for check_assertion to run on fedora policy, and started looking at performance. Most of the time is in libsepol's avtab hash table lookups.

With some changes to the hash table and -O2, it now takes about 5 minutes. I suspect we can do much better. There is still a peak of about 1.2GB RES.

I'll clean up that patch and submit it soon.

--- Additional comment from Daniel Walsh on 2015-01-02 07:58:48 EST ---

If you fix the performance problem, we can turn this back on.

--- Additional comment from John Brooks on 2015-01-05 20:50:26 EST ---

https://github.com/fedora-selinux/selinux/pull/3

Let me know if there is a better way to submit changes.

--- Additional comment from Miroslav Grepl on 2015-03-05 07:39:54 EST ---

Petr,
there is updated patch in upstream from John.

--- Additional comment from Jan Kurik on 2015-07-15 10:35:42 EDT ---

This bug appears to have been reported against 'rawhide' during the Fedora 23 development cycle.
Changing version to '23'.

(As we did not run this process for some time, it could affect also pre-Fedora 23 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 23 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora23

--- Additional comment from Petr Lautrbach on 2015-09-02 02:16:58 EDT ---

The updated libsepol-2.4 with John's patches is already in Fedora 23 so I guess you can remove '-a' option.

--- Additional comment from Miroslav Grepl on 2015-09-02 03:19:26 EDT ---

We should have all fixes in Fedora userspace and also in Fedora policy in F23/Rawhide. 

We also removed this option from selinux-policy.spec in rawhide and we are testing in for F23. 

So you can test it with the latest F23 policy and libsepol builds to see how it works. Thank you.

--- Additional comment from Petr Lautrbach on 2015-12-09 11:19:53 EST ---

I believe this is already incorporated in Fedora 23.

Comment 1 Miroslav Grepl 2016-03-21 09:31:42 UTC
Could we reflect this change also in semanage.conf?


Some numbers posted by pmoore@redhat.com.

  # grep '^expand-check' /etc/selinux/semanage.conf 
  expand-check=0
  # time semanage dontaudit off
  real    0m5.270s
  user    0m4.447s
  sys     0m0.799s
  # time semanage dontaudit on
  real    0m5.385s
  user    0m4.578s
  sys     0m0.786s

  # grep '^expand-check' /etc/selinux/semanage.conf 
  expand-check=1
  # time semanage dontaudit off
  real    0m5.555s
  user    0m4.745s
  sys     0m0.780s
  # time semanage dontaudit on
  real    0m5.861s
  user    0m5.037s
  sys     0m0.804s

Comment 2 Petr Lautrbach 2016-03-21 10:23:44 UTC
Pushed to rawhide libsemanage-2.5-3.fc25.

Please test it and if there's only 7% penalty then I'll make an update for F24 as well.

Comment 3 Daniel Walsh 2016-03-21 19:49:56 UTC
I have seen this crash when using typebounds commands.

Comment 4 Paul Moore 2016-03-22 14:32:19 UTC
(In reply to Petr Lautrbach from comment #2)
> Pushed to rawhide libsemanage-2.5-3.fc25.

For reasons I don't understand, I'm still not seeing this update in Rawhide, suggestions?

  # dnf clean metadata
  Cleaning repos: pcmoore-kernel-secnext plautrba-selinux rawhide-debuginfo
                : rawhide
  17 metadata files removed
  11 dbcache files removed
  # dnf update libsemanage
  Fedora - Rawhide - Debug                        3.4 MB/s |  18 MB     00:05    
  Fedora - Rawhide - Developmental packages for t 1.8 MB/s |  47 MB     00:25    
  Copr repo for selinux owned by plautrba          75 kB/s |  40 kB     00:00    
  Copr repo for kernel-secnext owned by pcmoore   1.6 MB/s | 5.9 MB     00:03    
  Last metadata expiration check: 0:00:04 ago on Tue Mar 22 10:31:23 2016.
  Dependencies resolved.
  Nothing to do.
  Complete!
  # rpm -q libsemanage
  libsemanage-2.5-2.fc25.x86_64

Comment 5 Petr Lautrbach 2016-03-22 14:44:24 UTC
I must have missed to run 'fedpkg build', sorry. It's being built right now - http://koji.fedoraproject.org/koji/taskinfo?taskID=13425830

Comment 6 Petr Lautrbach 2016-03-22 15:13:42 UTC
Something's wrong in koji:

13425830 build (rawhide, //libsemanage:2041bbce9964058fdbac5eb5ee5da0a1d383dc86): open (buildhw-10.phx2.fedoraproject.org) -> FAILED: BuildError: Error running GIT command "git clone -n git://pkgs.fedoraproject.org//libsemanage /var/lib/mock/f25-build-5239985-592735/root/tmp/scmroot/libsemanage", see checkout.log for details

Comment 7 Paul Moore 2016-03-22 15:17:35 UTC
I've had koji/brew blow up with internal errors like that before, most of the time submitting the build again works.

Comment 8 Petr Lautrbach 2016-03-22 19:27:57 UTC
Built by mgrepl, thanks!

http://koji.fedoraproject.org/koji/buildinfo?buildID=747934

Comment 9 Paul Moore 2016-03-23 15:37:50 UTC
I'm still not seeing libsemanage-2.5-3.fc25 on my Rawhide systems for some reason.

Comment 10 Petr Lautrbach 2016-03-24 08:04:02 UTC
It might be some delay on mirrors. In the mean time, you can try the build from comment 8 or change the value in your semanage.conf  - expand-check=1

Update will warn you about changed config file and you will have semanage.conf.rpmnew file in /etc/selinux with the package default.

Comment 11 Petr Lautrbach 2016-03-24 10:22:47 UTC
btw works for me:

# dnf update libsemanage
...
 libsemanage              x86_64      2.5-3.fc25       rawhide      148 k
 libsemanage-python       x86_64      2.5-3.fc25       rawhide      108 k
 libsemanage-python3      x86_64      2.5-3.fc25       rawhide      111 k

Comment 12 Paul Moore 2016-03-24 12:48:20 UTC
The update finally appeared on my Rawhide system and some quick tests revealed numbers similar to what I reported previously (see comment #1).

Comment 13 Stephen Smalley 2016-03-28 15:07:01 UTC
This will break if the user has any locally installed policy modules that violate a neverallow, which is not unlikely since they have never been enforced up until now and an audit2allow-generated policy could easily run afoul of a particular neverallow.  The end result would be that all semodule and semanage operations (including those triggered by a policy update) would start failing because the neverallow checks would fail, and the only way to fix would be to switch back to expand-check=0 temporarily, remove the offending module, and then re-enable expand-check=1.  The user would then need to fix the offending module's source (which may not be available to them any longer; again, if they generated it via audit2allow at some point in the past, installed the .pp file and then didn't backup or otherwise preserve the .te file) in order to recompile and re-install it.
So while I applaud re-enabling make validate on policy builds and encourage use of expand-check=1 by developers, I'm not sure you can inflict this on end users yet.

Also, as Dan pointed out, there seems to be a bug in the CIL type bounds error reporting logic that causes a seg fault in libsepol 2.5 for his docker example.

Comment 14 Paul Moore 2016-03-28 19:53:56 UTC
That's a good point that I hadn't thought of earlier, although at what point do we want to enable the expanded checks?  I think we need to work towards enabling this, and I fear it will be a flag-day event regardless.

Comment 15 Stephen Smalley 2016-03-28 20:07:08 UTC
Probably need more granular control over it, e.g. ability to separately disable/enable neverallow vs type hierarchy checking (easy), ability to exempt specific modules from specific checks (more difficult).

We are however getting neverallow and type hierarchy checking on the Fedora-shipped policy as a result of re-enabling make validate without overriding SEMOD_EXP in the Fedora selinux-policy spec file.  So we are only lacking the checks on third party and user policy modules.

Comment 16 Daniel Walsh 2016-03-28 21:20:36 UTC
We could turn the check on for only Fresh installs, and leave the check off on updates.  But this could still break third parties.

Comment 17 Paul Moore 2016-03-28 22:34:59 UTC
(In reply to Daniel Walsh from comment #16)
> We could turn the check on for only Fresh installs, and leave the check off
> on updates.  But this could still break third parties.

Is there precedence for doing something like that?  I worry we would get flooded by confused people trying to figure out why they can load policy module on system A (older, updated system), but it fails on system B (newer, fresh install).

Comment 18 Petr Lautrbach 2016-03-29 06:25:33 UTC
These kind of changes usually happen in Rawhide where they are somehow expected. We  need to document it in release notes with the expand-check=0 workaround. As for F24, it's probably late to propose it as a Fedora change, but since we are in pre-Beta phase, it should be still feasible to have this change in Release notes as well. I can create a FESCO ticket for that to have officially approved.

Comment 19 Daniel Walsh 2016-03-29 13:23:53 UTC
I would say this should only be in Rawhide, to find out what kind of damage it will cause.

This could break people doing 

audit2allow -M mypol

But then again maybe it should.

Comment 20 Paul Moore 2016-03-29 14:09:16 UTC
Thanks Petr.  I guess we'll go with this and see what happens, maybe we break the whole world, maybe not; we'll never know how bad it is until we try.

Comment 21 Miroslav Grepl 2016-03-29 14:26:45 UTC
(In reply to Daniel Walsh from comment #19)
> I would say this should only be in Rawhide, to find out what kind of damage
> it will cause.
> 
> This could break people doing 
> 
> audit2allow -M mypol
> 
> But then again maybe it should.

I agree with Rawhide for now. But definitely something what we need to have.

Comment 22 Petr Lautrbach 2016-03-29 14:36:42 UTC
Apparently  it breaks docker - https://github.com/fedora-cloud/docker-selinux/issues/16 so lets postpone it to Rawhide/f25 to let docker time to fix it.

Comment 23 Stephen Smalley 2016-05-02 19:12:25 UTC
IMHO, this should be reverted (i.e. expand-check = 0 again in default semanage.conf). Rationale: Users may have previously installed local policy modules generated by audit2allow that violate neverallows.  In this situation, on upgrade, semodule will fail from %post in selinux-policy-targeted due to neverallow failure.  However, rpm will think the package has been successfully installed since this occurs from %post, even though libsemanage will have rolled back to the previous policy, so rpm -q will lie and the user may not even know that they do not have the correct policy installed.  The user will be unable to make any policy updates or changes until they disable expand-check or disable/remove the offending module(s), and even then, they'll have to manually do a reinstall selinux-policy-targeted to just get the current policy installed.

I think it suffices to perform the validation at build time (via make validate), and policy developers can manually enable expand-check=1 for their own development/testing, but we can't make this the default in Fedora now given that it hasn't been imposed from the beginning and users may have built up quite a history of local policy modules, some of which they may not even still have sources.

Comment 24 Miroslav Grepl 2016-05-03 07:22:27 UTC
I don't agree here. I don't think people will hit this issue so often
and if so they will report a new bug or ask. We don't have a lot
neverallows checks and there is a reason for them and it makes SELinux
Policy more secure.

You can see

neverallow check failed at line 250 of
/var/lib/selinux/targeted/tmp/modules/100/authlogin/cil
  (neverallow authlogin_typeattr_1 shadow_t (file (read)))
    <root>
    allow at line 5 of /var/lib/selinux/targeted/tmp/modules/400/mypol/cil
      (allow httpd_t shadow_t (file (getattr open read)))


I wrote the blog

https://mgrepl.wordpress.com/2015/09/09/selinux-insides-part2-neverallow-assertions/

about that.

Also it is a rawhide system which is a correct place for changes like this. We should probably do a public announcement.

Comment 25 Stephen Smalley 2016-05-03 14:51:20 UTC
So I might agree if we had sane failure semantics, but that isn't the current situation.  Policy module install really needs to happen as part of the rpm transaction, not from %post, which was the purpose of the rpm selinux plugin work, but no one ever took that and used it AFAIK.

Again, a user on upgrade could end up running with the old policy due to neverallow failure while rpm -q tells them they are running the new policy.  And they might not even see the error message unless they happen to run the update interactively or go look at the logs.  And it will never self-correct on subsequent policy updates; it can only be fixed by the user manually removing the offending module or modifying semanage.conf to disable expand-check, then running dnf reinstall selinux-policy-targeted to force a re-install of the policy.

Your call, but consider yourself warned...

Comment 26 Petr Lautrbach 2016-05-03 19:00:26 UTC
Thanks Stephen. Your concerns are definitely valuable. 

I'm reopening this bug as we need to address the upgrade problem and provide the documentation or rather create an official Fedora Change page for Fedora 25 to allow broader discussion. Incompatible changes sometimes happen in Fedora but they need to be advertised and preferably approved by FESCo.

As for the upgrade problem, it could be feasible to solve it by a rpm trigger script which would check if it's possible to rebuild original policy and wouldn't allow to update if not. But it's just an idea which needs to be investigated.

Given that we are talking about Rawhide before F25, I'd let expand-check enabled for now and reverted it before f25 branch if we can't address described issues.

Comment 27 Miroslav Grepl 2016-05-05 20:00:25 UTC
Yes, I did not want to pretend there is no issue. Petr described it correctly.

Thank you guys for this discussion.

Comment 28 Jan Kurik 2016-07-26 04:56:01 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 25 development cycle.
Changing version to '25'.

Comment 29 Petr Lautrbach 2016-08-01 11:40:47 UTC
FYI: since we don't have a solution for the concerns raised above, I've reverted the default value of expand-check to 0 in libsemanage-2.5-7.fc25.

We will continue with the effort to have expand-check=1 in Rawhide/Fedora 26.

Comment 30 Fedora End Of Life 2017-02-28 09:56:12 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 26 development cycle.
Changing version to '26'.

Comment 31 Fedora Update System 2018-03-26 11:00:21 UTC
checkpolicy-2.7-7.fc28 libselinux-2.7-13.fc28 libsemanage-2.7-12.fc28 libsepol-2.7-6.fc28 policycoreutils-2.7-17.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-08531132c6

Comment 32 Fedora Update System 2018-03-26 11:00:37 UTC
checkpolicy-2.7-7.fc28 libselinux-2.7-13.fc28 libsemanage-2.7-12.fc28 libsepol-2.7-6.fc28 policycoreutils-2.7-17.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-08531132c6

Comment 33 Fedora Update System 2018-03-26 15:01:09 UTC
checkpolicy-2.7-7.fc28, libselinux-2.7-13.fc28, libsemanage-2.7-12.fc28, libsepol-2.7-6.fc28, policycoreutils-2.7-17.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-08531132c6

Comment 34 Fedora Update System 2018-04-01 19:06:07 UTC
checkpolicy-2.7-7.fc28, libselinux-2.7-13.fc28, libsemanage-2.7-12.fc28, libsepol-2.7-6.fc28, policycoreutils-2.7-17.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.