| Message ID | 20170512164423.17190-1-sds@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, May 12, 2017 at 12:44 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > 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). For future reference, I would encourage people to provide links to upstream issue trackers so long as the full URL is given and that URL is publicly accessible.
On Fri, May 12, 2017 at 12:44 PM, Stephen Smalley <sds@tycho.nsa.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@tycho.nsa.gov> > --- > 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[iter]); > + selinux_policycap_names[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_CAPABILITY_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_CGROUPSECLABEL); > + > + 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. > + } > } > > static int security_preserve_bools(struct policydb *p); > -- > 2.9.3 >
On Tue, 2017-05-16 at 16:56 -0400, Paul Moore wrote: > On Fri, May 12, 2017 at 12:44 PM, Stephen Smalley <sds@tycho.nsa.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@tycho.nsa.gov> > > --- > > 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[iter] > > ); > > + 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_CAPABILI > > 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_CGROUPSECLABEL) > > ; > > + > > + 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). This warning would only ever manifest if running an old kernel with a new policy (or with a bug in the policy compiler). 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. > > > + } > > } > > > > static int security_preserve_bools(struct policydb *p); > > -- > > 2.9.3 > > > >
On Tue, May 16, 2017 at 5:11 PM, Stephen Smalley <sds@tycho.nsa.gov> 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@tycho.nsa.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@tycho.nsa.gov> >> > --- >> > 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[iter] >> > ); >> > + 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_CAPABILI >> > 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_CGROUPSECLABEL) >> > ; >> > + >> > + 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. > 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.
On Tue, 2017-05-16 at 17:19 -0400, Paul Moore wrote: > On Tue, May 16, 2017 at 5:11 PM, Stephen Smalley <sds@tycho.nsa.gov> > 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@tycho.nsa. > > > 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@tycho.nsa.gov> > > > > --- > > > > 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.
On Wed, May 17, 2017 at 9:17 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On Tue, 2017-05-16 at 17:19 -0400, Paul Moore wrote: >> On Tue, May 16, 2017 at 5:11 PM, Stephen Smalley <sds@tycho.nsa.gov> >> wrote: ... >> > 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. Isn't that the right thing to do anyway? You shouldn't use a policy with a policy capability on a kernel that has no knowledge of the policy capability for the reasons we've already discussed. I'm also going to guess that this isn't as large a problem as we may be making it out, distributions should catch this problem fairly quickly in their development process and those that are savvy enough to load their own policy should know how to handle this as well. > 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) ... Once again, is it safe for refpolicy to enable these now if we are worrying about older kernels? > ... and > would also be a problem for Android, since many different kernel > versions (and often much older ones) are used with both. I'm not saying I have completely made my mind up about this, but right now I'm thinking the issue of new-policy/old-kernel is a distro problem (I view Android as just another distribution) and not something we can safely resolve upstream. > 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.
On Wed, 2017-05-17 at 15:39 -0400, Paul Moore wrote: > On Wed, May 17, 2017 at 9:17 AM, Stephen Smalley <sds@tycho.nsa.gov> > wrote: > > On Tue, 2017-05-16 at 17:19 -0400, Paul Moore wrote: > > > On Tue, May 16, 2017 at 5:11 PM, Stephen Smalley <sds@tycho.nsa.g > > > ov> > > > wrote: > > ... > > > > > 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. > > Isn't that the right thing to do anyway? You shouldn't use a policy > with a policy capability on a kernel that has no knowledge of the > policy capability for the reasons we've already discussed. I'm also > going to guess that this isn't as large a problem as we may be making > it out, distributions should catch this problem fairly quickly in > their development process and those that are savvy enough to load > their own policy should know how to handle this as well. Possibly I overstated the seriousness of using a policy with an unknown policy capability ;) Historically we have preserved compatibility in policies so that old kernels continued to work correctly even if they do not support the capability, and you would only lose out on the ability to leverage that capability for improved access control. I don't think we can/should rely on the distributions to ensure that users don't end up with unbootable systems. The people maintaining policy are not the same as the people maintaining the kernel, we've gone to great lengths to allow them to be independently updated, and the point at which their updates take effect differs (policy updates are immediately loaded; kernel updates won't until they choose to reboot into the new kernel). Even better, a policy load failure at update time might not even be visible to the user, particularly as it isn't even visible to rpm (hurray for doing things from %post), so they might not know they are in trouble until they next reboot and find their system dead. Further, some distributions support multiple kernels, and all distributions support multiple kernels over time (i.e. updates). So the potential for users to end up with newer policy that enables a capability unknown to their currently running kernel is significant. Even if the policy packager carefully tests the policy on the kernel he is running, that's no guarantee it will work for all users. And the failure mode would be quite severe; the system would refuse to boot, they would have to boot with enforcing=0, no policy would load (effectively selinux=0), download a fixed policy, reboot and relabel. That would not be good PR for selinux. > > > 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) ... > > Once again, is it safe for refpolicy to enable these now if we are > worrying about older kernels? Well, as I said, it has generally been done in a backward-compatible manner, so the system degrades gracefully. And we have to be able to add them in a timely manner if we don't want SELinux advancement to come to a halt. > > ... and > > would also be a problem for Android, since many different kernel > > versions (and often much older ones) are used with both. > > I'm not saying I have completely made my mind up about this, but > right > now I'm thinking the issue of new-policy/old-kernel is a distro > problem (I view Android as just another distribution) and not > something we can safely resolve upstream. Android is in some ways easier than distributions (policy and kernel are not updated independently), but in other ways it is harder (every device forks its own kernel and maintains it, so kernel maintenance is even more distributed, and kernels are often not updated or at least not rebased). I don't think we can count on that going well either. > > > 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. Still think warning is best. Willing to degrade to info. Not willing to fail on policy load.
On Wed, May 17, 2017 at 4:45 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On Wed, 2017-05-17 at 15:39 -0400, Paul Moore wrote: ... >> Isn't that the right thing to do anyway? You shouldn't use a policy >> with a policy capability on a kernel that has no knowledge of the >> policy capability for the reasons we've already discussed. I'm also >> going to guess that this isn't as large a problem as we may be making >> it out, distributions should catch this problem fairly quickly in >> their development process and those that are savvy enough to load >> their own policy should know how to handle this as well. > > Possibly I overstated the seriousness of using a policy with an unknown > policy capability ;) There ya go, you just made me threaten to fail the policy load to get here ;) > Historically we have preserved compatibility in > policies so that old kernels continued to work correctly even if they > do not support the capability, and you would only lose out on the > ability to leverage that capability for improved access control. Yes, I agree that I don't think this is a serious issue with any of the existing policy capabilities. > I don't think we can/should rely on the distributions to ensure that > users don't end up with unbootable systems ... I obviously agree, how could one not? However, I also think we need to take a realistic view of distributions that take use rather extreme default configurations and don't ensure that their updates are properly managed. I don't see the point in arguing every possible combination in this thread (I think we all have other more important things to do, at least I hope we do), but I don't want to be held responsible if a distro shoots *itself* in the foot. >> > 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. > > Still think warning is best. Willing to degrade to info. Not willing > to fail on policy load. INFO please.
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[iter]); + selinux_policycap_names[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_CAPABILITY_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_CGROUPSECLABEL); + + 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); + } } static int security_preserve_bools(struct policydb *p);
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@tycho.nsa.gov> --- 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(-)