Re: [PATCH 2/3] selinux: add checksum to policydb

[Stephen Smalley]

On Fri, 2017-04-28 at 17:16 +0200, Sebastien Buisson wrote:
> 2017-04-27 20:47 GMT+02:00 Stephen Smalley <sds@xxxxxxxxxxxxx>:
> > > I just checked, with the method of computing the checksum on a
> > > (data,
> > > len) pair on entry to security_load_policy() the checksum does
> > > not
> > > change after using setsebool. So it seems I would need to call
> > > security_read_policy() to retrieve the binary representation of
> > > the
> > > policy as currently enforced by the kernel. Unless you can see
> > > another
> > > way?
> > 
> > I don't think that's a viable option, since security_read_policy()
> > is
> > going to be expensive in order to generate a full policy image,
> > while
> > security_set_bools() is supposed to be substantially cheaper than a
> > full policy load.
> > 
> > Also, the advantage of taking the hash of the original input file
> > is
> > that you can independently compute a reference hash offline or on
> > the
> > server from the same policy file and compare them and you can
> > identify
> > which policy file was loaded based on the hash.
> > 
> > If you care about the active boolean state, then I'd suggest
> > hashing
> > the active boolean state separately and storing that after the
> > policy
> > hash.ÂÂYou can do that in both security_load_policy() and
> > security_set_bools().ÂÂJust iterate through the bools like
> > security_set_bools() does, write the ->state of each boolean into a
> > buffer, and then hash that buffer.
> 
> I just noticed another issue: with the method of computing the
> checksum on a (data, len) pair on entry to security_load_policy(),
> the
> checksum does not change after inserting a new module with semodule.
> It is a problem as a module can allow actions by certain users on
> some
> file contexts. So not detecting that kind of policy tampering defeats
> the purpose of the checksum as I imagine it.

You seem to be conflating kernel policy with userspace policy. 
security_load_policy() is provided with the kernel policy image, which
is the result of linking the kernel-relevant portions of all policy
modules together. A hash of that image will change if you insert a
policy module that affects the kernel policy in any way.  But a change
that only affects userspace policy isn't ever going to be reflected in
the kernel.  It doesn't matter where or when you compute your checksum
within the kernel; it isn't ever going to reflect those userspace
policy changes.

> To address this I propose to come back to the idea of the notifier.
> The checksum would not be stored inside the struct policydb. The
> checksum would be computed on a (data, len) pair got from
> security_read_policy() every time someone is asking for it through
> the
> security_policy_cksum() hook. The ones that would potentially call
> security_policy_cksum() are those that would register a callback on
> lsm_notifier, and the userspace processes reading
> /sys/fs/selinux/policycksum. So no matter if computing the checksum
> gets expensive, that would be the caller's responsibility to use it
> with care. Just like with /sys/fs/selinux/policy today in fact.

This won't detect changes to userspace policy configurations either,
and it is less efficient than just computing/updating the checksum in
security_load_policy() and security_set_bools().  Also, if all you want
is a hash of /sys/fs/selinux/policy, then userspace can already read
and hash that itself at any time.  You aren't really providing any
additional information that way.  In contrast, saving and providing a
hash of the policy image that was loaded is not something that is
currently available, and could be useful in checking against a
reference hash of the policy file or in identifying which policy file
was loaded.

> > > > You needed to get (global) enforcing mode too, didn't
> > > > you?ÂÂThat's
> > > > separate from the policy.
> > > 
> > > Exactly, I also need to rework the patch I proposed about this,
> > > in
> > > light of the comments I received.
> > 
> > So perhaps what you really want is a hook interface and a selinuxfs
> > interface that returns a single string that encodes all of the
> > policy
> > properties that you care about?ÂÂRather than separate hooks and
> > interfaces?ÂÂYou could embed the enforcing status in the string
> > too.
> > Should probably include checkreqprot as well since that affects
> > enforcement of mmap/mprotect checks.
> 
> True, I should build a string of the form:
> <0 or 1 for enforce>:<0 or 1 for checkreqprot>:<hashalg>=<global
> checksum>
> I should probably rename it 'policybrief' instead of 'policycksum'.
>
> I realize that the 'SELinux user to UNIX user' assignments are
> important as well. If for instance a regular user on a given cluster
> node is mapped to unconfined_u instead of user_u, this user would
> erroneously have major privileges. I do not know where I should look
> for this information, and possibly compute another checksum.

As above, that's userspace policy configuration, and not something that
kernel can or should deal with.