| Message ID | 1465483638-9489-12-git-send-email-dgdegra@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 09, 2016 at 10:47:14AM -0400, Daniel De Graaf wrote: > When an unknown domctl, sysctl, or other operation is encountered in the > FLASK security server, use the allow_unknown bit in the security policy > to decide if the permission should be allowed or denied. This bit is > off by default, but it can be set by using checkpolicy -U allow when > compiling the policy. This allows new operations to be tested without > needing to immediately add security checks; however, it is not flexible > enough to avoid adding the actual permission checks. An error message > is printed to the hypervisor console when this fallback is encountered. .. and the operation is permitted. > > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > --- > xen/xsm/flask/hooks.c | 44 +++++++++++++++++++++++++--------------- > xen/xsm/flask/include/security.h | 2 ++ > xen/xsm/flask/ss/policydb.c | 1 + > xen/xsm/flask/ss/policydb.h | 6 ++++++ > xen/xsm/flask/ss/services.c | 5 +++++ > 5 files changed, 42 insertions(+), 16 deletions(-) > > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index a8d45e7..3ab3fbf 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -136,6 +136,23 @@ static int get_irq_sid(int irq, u32 *sid, struct avc_audit_data *ad) > return 0; > } > > +static int avc_unknown_permission(const char *name, int id) > +{ > + int rc; I would add a new line here. > + if ( !flask_enforcing || security_get_allow_unknown() ) > + { > + printk(XENLOG_G_WARNING "FLASK: Allowing unknown %s: %d.\n", name, id); > + rc = 0; > + } > + else > + { > + printk(XENLOG_G_ERR "FLASK: Denying unknown %s: %d.\n", name, id); > + rc = -EPERM; > + } > + > + return rc; > +} > + The rest looks OK, but I have a question: Is this how Linux operates?
On 06/17/2016 11:45 AM, Konrad Rzeszutek Wilk wrote: > On Thu, Jun 09, 2016 at 10:47:14AM -0400, Daniel De Graaf wrote: >> When an unknown domctl, sysctl, or other operation is encountered in the >> FLASK security server, use the allow_unknown bit in the security policy >> to decide if the permission should be allowed or denied. This bit is >> off by default, but it can be set by using checkpolicy -U allow when >> compiling the policy. This allows new operations to be tested without >> needing to immediately add security checks; however, it is not flexible >> enough to avoid adding the actual permission checks. An error message >> is printed to the hypervisor console when this fallback is encountered. > > .. and the operation is permitted. The error message is printed either way (with a different priority). Were you suggesting I expand this explanation to include both the error and warning messages separately? >> >> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> >> --- >> xen/xsm/flask/hooks.c | 44 +++++++++++++++++++++++++--------------- >> xen/xsm/flask/include/security.h | 2 ++ >> xen/xsm/flask/ss/policydb.c | 1 + >> xen/xsm/flask/ss/policydb.h | 6 ++++++ >> xen/xsm/flask/ss/services.c | 5 +++++ >> 5 files changed, 42 insertions(+), 16 deletions(-) >> >> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c >> index a8d45e7..3ab3fbf 100644 >> --- a/xen/xsm/flask/hooks.c >> +++ b/xen/xsm/flask/hooks.c >> @@ -136,6 +136,23 @@ static int get_irq_sid(int irq, u32 *sid, struct avc_audit_data *ad) >> return 0; >> } >> >> +static int avc_unknown_permission(const char *name, int id) >> +{ >> + int rc; > > I would add a new line here. OK >> + if ( !flask_enforcing || security_get_allow_unknown() ) >> + { >> + printk(XENLOG_G_WARNING "FLASK: Allowing unknown %s: %d.\n", name, id); >> + rc = 0; >> + } >> + else >> + { >> + printk(XENLOG_G_ERR "FLASK: Denying unknown %s: %d.\n", name, id); >> + rc = -EPERM; >> + } >> + >> + return rc; >> +} >> + > > The rest looks OK, but I have a question: Is this how Linux operates? Yes; selinux_nlmsg_perm for an unknown netlink message seems to be an example there.
On Fri, Jun 17, 2016 at 01:02:58PM -0400, Daniel De Graaf wrote: > On 06/17/2016 11:45 AM, Konrad Rzeszutek Wilk wrote: > >On Thu, Jun 09, 2016 at 10:47:14AM -0400, Daniel De Graaf wrote: > >>When an unknown domctl, sysctl, or other operation is encountered in the > >>FLASK security server, use the allow_unknown bit in the security policy > >>to decide if the permission should be allowed or denied. This bit is > >>off by default, but it can be set by using checkpolicy -U allow when > >>compiling the policy. This allows new operations to be tested without > >>needing to immediately add security checks; however, it is not flexible > >>enough to avoid adding the actual permission checks. An error message > >>is printed to the hypervisor console when this fallback is encountered. > > > >.. and the operation is permitted. > > The error message is printed either way (with a different priority). Were correct. > you suggesting I expand this explanation to include both the error and > warning messages separately? It just that the patch changes the behavior. That is in the past if you had created an policy using checkpolicy -U allow it would print an error and return -EPERM. But now it will print an error and return 0 and pass the XSM check (aka operation ends being permitted). > > >> > >>Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > >>--- > >> xen/xsm/flask/hooks.c | 44 +++++++++++++++++++++++++--------------- > >> xen/xsm/flask/include/security.h | 2 ++ > >> xen/xsm/flask/ss/policydb.c | 1 + > >> xen/xsm/flask/ss/policydb.h | 6 ++++++ > >> xen/xsm/flask/ss/services.c | 5 +++++ > >> 5 files changed, 42 insertions(+), 16 deletions(-) > >> > >>diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > >>index a8d45e7..3ab3fbf 100644 > >>--- a/xen/xsm/flask/hooks.c > >>+++ b/xen/xsm/flask/hooks.c > >>@@ -136,6 +136,23 @@ static int get_irq_sid(int irq, u32 *sid, struct avc_audit_data *ad) > >> return 0; > >> } > >> > >>+static int avc_unknown_permission(const char *name, int id) > >>+{ > >>+ int rc; > > > >I would add a new line here. > > OK > > >>+ if ( !flask_enforcing || security_get_allow_unknown() ) > >>+ { > >>+ printk(XENLOG_G_WARNING "FLASK: Allowing unknown %s: %d.\n", name, id); > >>+ rc = 0; > >>+ } > >>+ else > >>+ { > >>+ printk(XENLOG_G_ERR "FLASK: Denying unknown %s: %d.\n", name, id); > >>+ rc = -EPERM; > >>+ } > >>+ > >>+ return rc; > >>+} > >>+ > > > >The rest looks OK, but I have a question: Is this how Linux operates? > > Yes; selinux_nlmsg_perm for an unknown netlink message seems to be an > example there. > > -- > Daniel De Graaf > National Security Agency
On 06/17/2016 01:13 PM, Konrad Rzeszutek Wilk wrote: > On Fri, Jun 17, 2016 at 01:02:58PM -0400, Daniel De Graaf wrote: >> On 06/17/2016 11:45 AM, Konrad Rzeszutek Wilk wrote: >>> On Thu, Jun 09, 2016 at 10:47:14AM -0400, Daniel De Graaf wrote: >>>> When an unknown domctl, sysctl, or other operation is encountered in the >>>> FLASK security server, use the allow_unknown bit in the security policy >>>> to decide if the permission should be allowed or denied. This bit is >>>> off by default, but it can be set by using checkpolicy -U allow when >>>> compiling the policy. This allows new operations to be tested without >>>> needing to immediately add security checks; however, it is not flexible >>>> enough to avoid adding the actual permission checks. An error message >>>> is printed to the hypervisor console when this fallback is encountered. >>> >>> .. and the operation is permitted. >> >> The error message is printed either way (with a different priority). Were > > correct. >> you suggesting I expand this explanation to include both the error and >> warning messages separately? > > It just that the patch changes the behavior. That is in the past if > you had created an policy using checkpolicy -U allow it would print an > error and return -EPERM. > > But now it will print an error and return 0 and pass the XSM check > (aka operation ends being permitted). I would be surprised if someone actually used allow_unknown before now, since it did nothing and required manually enabling. But if they did, this is a functionality change. I'll add a note of that.
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index a8d45e7..3ab3fbf 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -136,6 +136,23 @@ static int get_irq_sid(int irq, u32 *sid, struct avc_audit_data *ad) return 0; } +static int avc_unknown_permission(const char *name, int id) +{ + int rc; + if ( !flask_enforcing || security_get_allow_unknown() ) + { + printk(XENLOG_G_WARNING "FLASK: Allowing unknown %s: %d.\n", name, id); + rc = 0; + } + else + { + printk(XENLOG_G_ERR "FLASK: Denying unknown %s: %d.\n", name, id); + rc = -EPERM; + } + + return rc; +} + static int flask_domain_alloc_security(struct domain *d) { struct domain_security_struct *dsec; @@ -271,7 +288,7 @@ static int flask_evtchn_send(struct domain *d, struct evtchn *chn) rc = 0; break; default: - rc = -EPERM; + rc = avc_unknown_permission("event channel state", chn->state); } return rc; @@ -423,7 +440,7 @@ static int flask_console_io(struct domain *d, int cmd) perm = XEN__WRITECONSOLE; break; default: - return -EPERM; + return avc_unknown_permission("console_io", cmd); } return domain_has_xen(d, perm); @@ -455,7 +472,7 @@ static int flask_profile(struct domain *d, int op) perm = XEN__PRIVPROFILE; break; default: - return -EPERM; + return avc_unknown_permission("xenoprof op", op); } return domain_has_xen(d, perm); @@ -521,8 +538,7 @@ static int flask_domctl_scheduler_op(struct domain *d, int op) return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__GETSCHEDULER); default: - printk("flask_domctl_scheduler_op: Unknown op %d\n", op); - return -EPERM; + return avc_unknown_permission("domctl_scheduler_op", op); } } @@ -537,8 +553,7 @@ static int flask_sysctl_scheduler_op(int op) return domain_has_xen(current->domain, XEN__GETSCHEDULER); default: - printk("flask_sysctl_scheduler_op: Unknown op %d\n", op); - return -EPERM; + return avc_unknown_permission("sysctl_scheduler_op", op); } } @@ -735,8 +750,7 @@ static int flask_domctl(struct domain *d, int cmd) return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SOFT_RESET); default: - printk("flask_domctl: Unknown op %d\n", cmd); - return -EPERM; + return avc_unknown_permission("domctl", cmd); } } @@ -811,8 +825,7 @@ static int flask_sysctl(int cmd) XEN2__LIVEPATCH_OP, NULL); default: - printk("flask_sysctl: Unknown op %d\n", cmd); - return -EPERM; + return avc_unknown_permission("sysctl", cmd); } } @@ -1129,7 +1142,7 @@ static inline int flask_page_offline(uint32_t cmd) case sysctl_query_page_offline: return flask_resource_use_core(); default: - return -EPERM; + return avc_unknown_permission("page_offline", cmd); } } @@ -1402,8 +1415,7 @@ static int flask_platform_op(uint32_t op) SECCLASS_XEN2, XEN2__GET_SYMBOL, NULL); default: - printk("flask_platform_op: Unknown op %d\n", op); - return -EPERM; + return avc_unknown_permission("platform_op", op); } } @@ -1434,7 +1446,7 @@ static int flask_shadow_control(struct domain *d, uint32_t op) perm = SHADOW__LOGDIRTY; break; default: - return -EPERM; + return avc_unknown_permission("shadow_control", op); } return current_has_perm(d, SECCLASS_SHADOW, perm); @@ -1538,7 +1550,7 @@ static int flask_apic(struct domain *d, int cmd) perm = XEN__WRITEAPIC; break; default: - return -EPERM; + return avc_unknown_permission("apic", cmd); } return domain_has_xen(d, perm); diff --git a/xen/xsm/flask/include/security.h b/xen/xsm/flask/include/security.h index 2b00177..1da020d 100644 --- a/xen/xsm/flask/include/security.h +++ b/xen/xsm/flask/include/security.h @@ -78,6 +78,8 @@ int security_sid_to_context(u32 sid, char **scontext, u32 *scontext_len); int security_context_to_sid(char *scontext, u32 scontext_len, u32 *out_sid); +int security_get_allow_unknown(void); + int security_irq_sid(int pirq, u32 *out_sid); int security_iomem_sid(unsigned long, u32 *out_sid); diff --git a/xen/xsm/flask/ss/policydb.c b/xen/xsm/flask/ss/policydb.c index 8aa88c1..46574c3 100644 --- a/xen/xsm/flask/ss/policydb.c +++ b/xen/xsm/flask/ss/policydb.c @@ -1843,6 +1843,7 @@ int policydb_read(struct policydb *p, void *fp) goto bad; } } + p->allow_unknown = !!(le32_to_cpu(buf[1]) & ALLOW_UNKNOWN); if ( p->policyvers >= POLICYDB_VERSION_POLCAP && ebitmap_read(&p->policycaps, fp) != 0 ) diff --git a/xen/xsm/flask/ss/policydb.h b/xen/xsm/flask/ss/policydb.h index 50e22f3..eb1e44d 100644 --- a/xen/xsm/flask/ss/policydb.h +++ b/xen/xsm/flask/ss/policydb.h @@ -246,6 +246,8 @@ struct policydb { unsigned int policyvers; + unsigned int allow_unknown : 1; + u16 target_type; }; @@ -261,6 +263,10 @@ extern int policydb_read(struct policydb *p, void *fp); #define POLICYDB_CONFIG_MLS 1 +/* the config flags related to unknown classes/perms are bits 2 and 3 */ +#define REJECT_UNKNOWN 0x00000002 +#define ALLOW_UNKNOWN 0x00000004 + #define OBJECT_R "object_r" #define OBJECT_R_VAL 1 diff --git a/xen/xsm/flask/ss/services.c b/xen/xsm/flask/ss/services.c index c9b27a0..ce880e9 100644 --- a/xen/xsm/flask/ss/services.c +++ b/xen/xsm/flask/ss/services.c @@ -1465,6 +1465,11 @@ err: } +int security_get_allow_unknown(void) +{ + return policydb.allow_unknown; +} + /** * security_irq_sid - Obtain the SID for a physical irq. * @pirq: physical irq
When an unknown domctl, sysctl, or other operation is encountered in the FLASK security server, use the allow_unknown bit in the security policy to decide if the permission should be allowed or denied. This bit is off by default, but it can be set by using checkpolicy -U allow when compiling the policy. This allows new operations to be tested without needing to immediately add security checks; however, it is not flexible enough to avoid adding the actual permission checks. An error message is printed to the hypervisor console when this fallback is encountered. Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> --- xen/xsm/flask/hooks.c | 44 +++++++++++++++++++++++++--------------- xen/xsm/flask/include/security.h | 2 ++ xen/xsm/flask/ss/policydb.c | 1 + xen/xsm/flask/ss/policydb.h | 6 ++++++ xen/xsm/flask/ss/services.c | 5 +++++ 5 files changed, 42 insertions(+), 16 deletions(-)