On Tue, 2017-05-16 at 17:19 -0400, Paul Moore wrote:
> On Tue, May 16, 2017 at 5:11 PM, Stephen Smalley <sds@xxxxxxxxxxxxx>
> wrote:
> > On Tue, 2017-05-16 at 16:56 -0400, Paul Moore wrote:
> > > On Fri, May 12, 2017 at 12:44 PM, Stephen Smalley <sds@xxxxxxxxx.
> > > gov>
> > > wrote:
> > > > Log the state of SELinux policy capabilities when a policy is
> > > > loaded.
> > > > For each policy capability known to the kernel, log an
> > > > informational
> > > > message with the policy capability name and the value set in
> > > > the
> > > > policy.
> > > > For policy capabilities that are set in the loaded policy but
> > > > unknown
> > > > to the kernel, log a warning with the policy capability index,
> > > > since
> > > > this is the only information presently available in the policy.
> > > >
> > > > Sample output with a policy created with a new capability
> > > > defined
> > > > that is not known to the kernel:
> > > > [ 43.812265] SELinux: policy capability
> > > > network_peer_controls=1
> > > > [ 43.812266] SELinux: policy capability open_perms=1
> > > > [ 43.812266] SELinux: policy capability
> > > > extended_socket_class=1
> > > > [ 43.812267] SELinux: policy capability
> > > > always_check_network=0
> > > > [ 43.812267] SELinux: policy capability cgroup_seclabel=0
> > > > [ 43.812268] SELinux: unknown policy capability 5
> > > >
> > > > Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> > > > ---
> > > > v2 drops the Resolves line since I think we are not supposed to
> > > > include
> > > > bug tracking info in upstream kernel commit messages (correct
> > > > me if
> > > > wrong).
> > > >
> > > > security/selinux/include/security.h | 2 ++
> > > > security/selinux/selinuxfs.c | 13 ++-----------
> > > > security/selinux/ss/services.c | 23
> > > > +++++++++++++++++++++++
> > > > 3 files changed, 27 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/security/selinux/include/security.h
> > > > b/security/selinux/include/security.h
> > > > index f979c35..c4224bb 100644
> > > > --- a/security/selinux/include/security.h
> > > > +++ b/security/selinux/include/security.h
> > > > @@ -76,6 +76,8 @@ enum {
> > > > };
> > > > #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX -
> > > > 1)
> > > >
> > > > +extern char
> > > > *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX];
> > > > +
> > > > extern int selinux_policycap_netpeer;
> > > > extern int selinux_policycap_openperm;
> > > > extern int selinux_policycap_extsockclass;
> > > > diff --git a/security/selinux/selinuxfs.c
> > > > b/security/selinux/selinuxfs.c
> > > > index ce71718..ea2da91 100644
> > > > --- a/security/selinux/selinuxfs.c
> > > > +++ b/security/selinux/selinuxfs.c
> > > > @@ -41,15 +41,6 @@
> > > > #include "objsec.h"
> > > > #include "conditional.h"
> > > >
> > > > -/* Policy capability filenames */
> > > > -static char *policycap_names[] = {
> > > > - "network_peer_controls",
> > > > - "open_perms",
> > > > - "extended_socket_class",
> > > > - "always_check_network",
> > > > - "cgroup_seclabel"
> > > > -};
> > > > -
> > > > unsigned int selinux_checkreqprot =
> > > > CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;
> > > >
> > > > static int __init checkreqprot_setup(char *str)
> > > > @@ -1750,9 +1741,9 @@ static int sel_make_policycap(void)
> > > > sel_remove_entries(policycap_dir);
> > > >
> > > > for (iter = 0; iter <= POLICYDB_CAPABILITY_MAX; iter++)
> > > > {
> > > > - if (iter < ARRAY_SIZE(policycap_names))
> > > > + if (iter < ARRAY_SIZE(selinux_policycap_names))
> > > > dentry = d_alloc_name(policycap_dir,
> > > > - policycap_names[i
> > > > ter]
> > > > );
> > > > + selinux_policycap
> > > > _nam
> > > > es[iter]);
> > > > else
> > > > dentry = d_alloc_name(policycap_dir,
> > > > "unknown");
> > > >
> > > > diff --git a/security/selinux/ss/services.c
> > > > b/security/selinux/ss/services.c
> > > > index 60d9b02..0abdec2 100644
> > > > --- a/security/selinux/ss/services.c
> > > > +++ b/security/selinux/ss/services.c
> > > > @@ -70,6 +70,15 @@
> > > > #include "ebitmap.h"
> > > > #include "audit.h"
> > > >
> > > > +/* Policy capability names */
> > > > +char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
> > > > + "network_peer_controls",
> > > > + "open_perms",
> > > > + "extended_socket_class",
> > > > + "always_check_network",
> > > > + "cgroup_seclabel"
> > > > +};
> > > > +
> > > > int selinux_policycap_netpeer;
> > > > int selinux_policycap_openperm;
> > > > int selinux_policycap_extsockclass;
> > > > @@ -1986,6 +1995,9 @@ static int convert_context(u32 key,
> > > >
> > > > static void security_load_policycaps(void)
> > > > {
> > > > + unsigned int i;
> > > > + struct ebitmap_node *node;
> > > > +
> > > > selinux_policycap_netpeer =
> > > > ebitmap_get_bit(&policydb.policycaps,
> > > > POLICYDB_CAPA
> > > > BILI
> > > > TY_NETPEER);
> > > > selinux_policycap_openperm =
> > > > ebitmap_get_bit(&policydb.policycaps,
> > > > @@ -1997,6 +2009,17 @@ static void
> > > > security_load_policycaps(void)
> > > > selinux_policycap_cgroupseclabel =
> > > > ebitmap_get_bit(&policydb.policycaps,
> > > > POLICYDB_CAPABILITY_CGROUPSECLA
> > > > BEL)
> > > > ;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(selinux_policycap_names);
> > > > i++)
> > > > + pr_info("SELinux: policy capability %s=%d\n",
> > > > + selinux_policycap_names[i],
> > > > + ebitmap_get_bit(&policydb.policycaps,
> > > > i));
> > > > +
> > > > + ebitmap_for_each_positive_bit(&policydb.policycaps,
> > > > node,
> > > > i) {
> > > > + if (i >= ARRAY_SIZE(selinux_policycap_names))
> > > > + pr_warn("SELinux: unknown policy
> > > > capability %u\n",
> > > > + i);
> > >
> > > Should this really be KERN_WARN? An undefined policy capability
> > > may
> > > be odd, but it doesn't seem like it warrants "warning" status;
> > > while
> > > not quite apples-to-apples, missing classes/perms are displayed
> > > with
> > > KERN_NOTICE.
> >
> > If the kernel does not support a policy capability that was enabled
> > in
> > the policy, then it may not be capable of enforcing the policy as
> > intended, and may therefore be operating insecurely (e.g. consider
> > if
> > your kernel was too old to support network_peer_controls or
> > always_check_network or open_perms but your policy has enabled them
> > and
> > is relying on those checks to be present) ...
>
> Well, I think the key is "may"; depending on the policy capability
> and
> the handle_unknown setting things might not fail in an insecure
> manner, but the system would definitely behave
> strangely. Regardless,
> I see your point and I'm now wondering if we should just fail the
> policy load if we detect any policy capabilities beyond what we know
> about.
I think that would prove problematic in practice since the system would
then refuse to boot with such a policy (the only way to boot in Linux
distributions would then be to boot with enforcing=0 and come up with
no policy - requiring you to then replace the policy, reboot, and
relabel, and in Android you can't boot at all if policy load fails - it
will reboot into recovery mode). It would mean that we could not add a
new policy capability to a policy until we were sure that no one was
still using a kernel that predated the support for it. That would more
or less preclude new policy capabilities from ever being enabled by
default in refpolicy (or at least take years to introduce them), and
would also be a problem for Android, since many different kernel
versions (and often much older ones) are used with both.
A warning seemed like a reasonable middle ground between silently
ignoring unknown policy capabilities (as the kernel currently does
before this patch) and rejecting the policy altogether. WARN vs INFO
was due to the fact that an unknown capability seemed more significant
and potentially an indicator of a kernel/policy mismatch than the state
of the known policy capabilities.
>
> > With undefined classes/perms, the behavior is governed by
> > handle_unknown, so a policy built with handle_unknown=allow has
> > already
> > accepted the potential for unknown classes/permissions being
> > allowed,
> > while a policy built with handle_unknown=deny will fail closed/safe
> > and
> > thus not degrade in security. Arguably though maybe those ought to
> > be
> > warnings too.
>
> Possibly. I'm less worried about that.
