On Wed, Mar 4, 2020 at 4:32 PM James Carter <jwcart2@xxxxxxxxx> wrote: > On Wed, Mar 4, 2020 at 9:25 AM Stephen Smalley > <stephen.smalley.work@xxxxxxxxx> wrote: > > > > On Wed, Mar 4, 2020 at 4:07 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > > I played with this a bit by recompiling the local binary policy with > > > secilc and then comparing the CIL of both binary policies (I used this > > > script [1]) and the results are a bit confusing... There is no > > > difference in result between -X 0 and -X 1 [2] and in both cases it > > > removes some unused attributes (those are only referenced from > > > neverallow rules) that were in the original policy > > > (/etc/selinux/targeted/policy/policy.31 from my Fedora 31 machine), > > > but not in the one recompiled via checkpolicy -C + secilc... At least > > > I was able to confirm that secilc -X 2 really removes the attributes > > > that have only one type and reduces the policy size by a few > > > kilobytes. > > > > > > I suspect that the reason for the unremoved attributes in the policy > > > built by semodule are due to a bug in libsepol: It seems that when it > > > starts with a cildb that has the neverallow rules in the input policy > > > + has disable_neverallow set, it removes the rules but not the > > > attributes that are used only in them. Only when it reads the policy > > > again, it identifies these unused attributes (since there are no > > > longer any neverallow rules in the input) and removes them > > > unconditionally. It could be something else, but if I'm right then I > > > think libsepol should be fixed to remove the unused attributes right > > > away. I don't dare digging into the CIL code to investigate it, though > > > ;) > > > > James will have to confirm the details but IIRC we had to keep > > attributes in the policy > > when they are referenced by a neverallow in order to avoid breaking > > Android because > > it uses the attribute definition from the policy as part of CTS > > validation of OEM policies > > (it extracts the neverallows from the AOSP policy, extracts the binary > > policy from the device, > > and checks the neverallows against the device policy). > > > > Steve is correct, we keep attributes that appear in neverallow rules > to avoid breaking Android. We also keep attributes that appear in > typeattributesets for attributes that appear in neverallow rules. > > See commits 67b410e80f0917bf1b378997cac0dddf1e6406f7 (libsepol/cil: > Keep attributes used by generated attributes in neverallow rules) and > 0be23c3f15fdbef35a57d8586aeeae9b1f7606cc (libsepol/cil: Add ability to > expand some attributes in binary policy) for more details. OK, so it is actually expected behavior. Fortunately bumping the attrs_expand_size to 2 doesn't interfere with this logic (I compared the binary policies produced by semodule -B with and without the patch [1] and the neverallow-only attributes were not removed). [1] https://patchwork.kernel.org/patch/11419703/ -- Ondrej Mosnacek <omosnace at redhat dot com> Software Engineer, Security Technologies Red Hat, Inc.
