[PATCH] xsm/flask: improve unknown permission handling

[PATCH] xsm/flask: improve unknown permission handling

[Daniel De Graaf]

When an unknown domctl, sysctl, or other operation is encountered in the
FLASK security server, use the allow_unknown bit in the security policy
(set by running checkpolicy -U allow) to decide if the permission should
be allowed or denied.  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            | 40 ++++++++++++++++++++++++----------------
 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, 38 insertions(+), 16 deletions(-)

diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 0ba2ce9..4c8a1d2 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -135,6 +135,19 @@ 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)
+{
+    /* A guest making an invalid hypercall can trigger this message, so it can't
+     * be an ASSERT or BUG_ON, but normally it is caused by a missing case in
+     * one of the switch statements below.
+     */
+    printk(XENLOG_G_ERR "FLASK: Unknown %s: %d.\n", name, id);
+    if ( !flask_enforcing || security_get_allow_unknown() )
+        return 0;
+    else
+        return -EPERM;
+}
+
 static int flask_domain_alloc_security(struct domain *d)
 {
     struct domain_security_struct *dsec;
@@ -270,7 +283,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;
@@ -422,7 +435,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);
@@ -454,7 +467,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);
@@ -520,8 +533,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);
     }
 }
 
@@ -536,8 +548,7 @@ static int flask_sysctl_scheduler_op(int op)
         return domain_has_xen(current->domain, XEN__GETSCHEDULER);
 
     default:
-        printk("flask_domctl_scheduler_op: Unknown op %d\n", op);
-        return -EPERM;
+        return avc_unknown_permission("sysctl_scheduler_op", op);
     }
 }
 
@@ -731,8 +742,7 @@ static int flask_domctl(struct domain *d, int cmd)
         return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CONFIGURE_DOMAIN);
 
     default:
-        printk("flask_domctl: Unknown op %d\n", cmd);
-        return -EPERM;
+        return avc_unknown_permission("domctl", cmd);
     }
 }
 
@@ -790,8 +800,7 @@ static int flask_sysctl(int cmd)
                                     XEN2__PSR_CMT_OP, NULL);
 
     default:
-        printk("flask_sysctl: Unknown op %d\n", cmd);
-        return -EPERM;
+        return avc_unknown_permission("sysctl", cmd);
     }
 }
 
@@ -1078,7 +1087,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);
     }
 }
 
@@ -1240,7 +1249,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);
@@ -1344,7 +1353,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);
@@ -1409,8 +1418,7 @@ static int flask_platform_op(uint32_t op)
                                     XEN2__RESOURCE_OP, NULL);
 
     default:
-        printk("flask_platform_op: Unknown op %d\n", op);
-        return -EPERM;
+        return avc_unknown_permission("platform_op", op);
     }
 }
 
diff --git a/xen/xsm/flask/include/security.h b/xen/xsm/flask/include/security.h
index 348f018..a93f14a 100644
--- a/xen/xsm/flask/include/security.h
+++ b/xen/xsm/flask/include/security.h
@@ -71,6 +71,8 @@ int security_context_to_sid(char *scontext, u32 scontext_len, u32 *out_sid);
 
 int security_get_user_sids(u32 callsid, char *username, u32 **sids, u32 *nel);
 
+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 50b2c78..b648f05 100644
--- a/xen/xsm/flask/ss/policydb.c
+++ b/xen/xsm/flask/ss/policydb.c
@@ -1802,6 +1802,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 b176300..aeaf69b 100644
--- a/xen/xsm/flask/ss/policydb.h
+++ b/xen/xsm/flask/ss/policydb.h
@@ -245,6 +245,8 @@ struct policydb {
 
     unsigned int policyvers;
 
+    unsigned int allow_unknown : 1;
+
     u16 target_type;
 };
 
@@ -260,6 +262,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 f0e459a..9b18509 100644
--- a/xen/xsm/flask/ss/services.c
+++ b/xen/xsm/flask/ss/services.c
@@ -1464,6 +1464,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
-- 
1.9.3

Re: [PATCH] xsm/flask: improve unknown permission handling

[Jan Beulich]

>>> On 25.11.14 at 19:05, <dgdegra@tycho.nsa.gov> wrote:
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -135,6 +135,19 @@ 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)

const char *name

> +{
> +    /* A guest making an invalid hypercall can trigger this message, so it can't
> +     * be an ASSERT or BUG_ON, but normally it is caused by a missing case in
> +     * one of the switch statements below.
> +     */
> +    printk(XENLOG_G_ERR "FLASK: Unknown %s: %d.\n", name, id);

I think this ought to be XENLOG_G_WARNING when not returning
an error. E.g. switch printing and return code determination, use
the return code to select the correct log level, and return after
logging the message.

Jan

> +    if ( !flask_enforcing || security_get_allow_unknown() )
> +        return 0;
> +    else
> +        return -EPERM;
> +}
> +
>  static int flask_domain_alloc_security(struct domain *d)
>  {
>      struct domain_security_struct *dsec;

Re: [PATCH] xsm/flask: improve unknown permission handling

[George Dunlap]

On Tue, Nov 25, 2014 at 6:05 PM, Daniel De Graaf <dgdegra@tycho.nsa.gov> 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
> (set by running checkpolicy -U allow) to decide if the permission should
> be allowed or denied.  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.

Thanks -- I do think as Konrad said however, that when building with
debug=y, we want the failure to be more obvious.  A crash is probably
the best thing.

I guess we want something like the following after the printk in
avc_unknown_permission()?

#ifndef NDEBUG
    BUG();
#endif

 -George

Re: [PATCH] xsm/flask: improve unknown permission handling

[Andrew Cooper]

On 27/11/14 15:23, George Dunlap wrote:
> On Tue, Nov 25, 2014 at 6:05 PM, Daniel De Graaf <dgdegra@tycho.nsa.gov> 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
>> (set by running checkpolicy -U allow) to decide if the permission should
>> be allowed or denied.  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.
> Thanks -- I do think as Konrad said however, that when building with
> debug=y, we want the failure to be more obvious.  A crash is probably
> the best thing.
>
> I guess we want something like the following after the printk in
> avc_unknown_permission()?
>
> #ifndef NDEBUG
>     BUG();
> #endif

ASSERT(!"Flask default policy error");

provides rather more information in the panic message, and avoids the
#ifdefs.

~Andrew

Re: [PATCH] xsm/flask: improve unknown permission handling

[Daniel De Graaf]

On 11/27/2014 10:33 AM, Andrew Cooper wrote:
> On 27/11/14 15:23, George Dunlap wrote:
>> On Tue, Nov 25, 2014 at 6:05 PM, Daniel De Graaf <dgdegra@tycho.nsa.gov> 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
>>> (set by running checkpolicy -U allow) to decide if the permission should
>>> be allowed or denied.  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.
>> Thanks -- I do think as Konrad said however, that when building with
>> debug=y, we want the failure to be more obvious.  A crash is probably
>> the best thing.
>>
>> I guess we want something like the following after the printk in
>> avc_unknown_permission()?
>>
>> #ifndef NDEBUG
>>      BUG();
>> #endif
>
> ASSERT(!"Flask default policy error");
>
> provides rather more information in the panic message, and avoids the
> #ifdefs.
>
> ~Andrew

This allows any (privileged or unprivileged) guest to trigger the ASSERT
and cause a hypervisor crash on a debug build.  Given that XSA-37 was
considered a security vulnerability due to this type of behavior, I am
hesitant to deliberately add a path to trigger a hypervisor crash, even
if it makes testing easier.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] xsm/flask: improve unknown permission handling

[Andrew Cooper]

On 03/12/14 18:37, Daniel De Graaf wrote:
> On 11/27/2014 10:33 AM, Andrew Cooper wrote:
>> On 27/11/14 15:23, George Dunlap wrote:
>>> On Tue, Nov 25, 2014 at 6:05 PM, Daniel De Graaf
>>> <dgdegra@tycho.nsa.gov> 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
>>>> (set by running checkpolicy -U allow) to decide if the permission
>>>> should
>>>> be allowed or denied.  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.
>>> Thanks -- I do think as Konrad said however, that when building with
>>> debug=y, we want the failure to be more obvious.  A crash is probably
>>> the best thing.
>>>
>>> I guess we want something like the following after the printk in
>>> avc_unknown_permission()?
>>>
>>> #ifndef NDEBUG
>>>      BUG();
>>> #endif
>>
>> ASSERT(!"Flask default policy error");
>>
>> provides rather more information in the panic message, and avoids the
>> #ifdefs.
>>
>> ~Andrew
>
> This allows any (privileged or unprivileged) guest to trigger the ASSERT
> and cause a hypervisor crash on a debug build.  Given that XSA-37 was
> considered a security vulnerability due to this type of behavior, I am
> hesitant to deliberately add a path to trigger a hypervisor crash, even
> if it makes testing easier.
>

XSA-37 was only an XSA because the rules at the time were unclear as
whether it was an issue or not.  At the same time, the rules were
clarified to state that issues in a debug build only are not security
issues.

~Andrew

Re: [PATCH] xsm/flask: improve unknown permission handling

[David Vrabel]

On 03/12/14 18:42, Andrew Cooper wrote:
> 
> XSA-37 was only an XSA because the rules at the time were unclear as
> whether it was an issue or not.  At the same time, the rules were
> clarified to state that issues in a debug build only are not security
> issues.

Given that we occasionally ask our customers to run debug versions of
Xen to diagnose particular problems I think this policy should change
(if not by the Xen project security team, then at least internally).

David

Re: [PATCH] xsm/flask: improve unknown permission handling

[George Dunlap]

On 12/04/2014 10:37 AM, David Vrabel wrote:
> On 03/12/14 18:42, Andrew Cooper wrote:
>>
>> XSA-37 was only an XSA because the rules at the time were unclear as
>> whether it was an issue or not.  At the same time, the rules were
>> clarified to state that issues in a debug build only are not security
>> issues.
> 
> Given that we occasionally ask our customers to run debug versions of
> Xen to diagnose particular problems I think this policy should change
> (if not by the Xen project security team, then at least internally).

Well given that debug builds *already*, by design, crash on a lot of
things that don't crash in production, then you are already increasing
their risk of a host crash just by giving them that build.  If
increasing the risk of a host crash isn't acceptable, then you should
stop giving them debug builds.

Alternately, maybe we can add an option either at compile time or at
boot time for ASSERTs not to crash for your situation.

But the fact that we have ASSERTs at all mean that we *expect* debug
builds to crash.  If that's not what we want we need to get rid of the
ASSERTs entirely.

 -George

Re: [PATCH] xsm/flask: improve unknown permission handling

[David Vrabel]

On 04/12/14 11:12, George Dunlap wrote:
> On 12/04/2014 10:37 AM, David Vrabel wrote:
>> On 03/12/14 18:42, Andrew Cooper wrote:
>>>
>>> XSA-37 was only an XSA because the rules at the time were unclear as
>>> whether it was an issue or not.  At the same time, the rules were
>>> clarified to state that issues in a debug build only are not security
>>> issues.
>>
>> Given that we occasionally ask our customers to run debug versions of
>> Xen to diagnose particular problems I think this policy should change
>> (if not by the Xen project security team, then at least internally).
> 
> Well given that debug builds *already*, by design, crash on a lot of
> things that don't crash in production, then you are already increasing
> their risk of a host crash just by giving them that build.  If
> increasing the risk of a host crash isn't acceptable, then you should
> stop giving them debug builds.

I disagree.  ASSERTs will cause Xen to fail more /predictably/.  A bug
that would trigger an ASSERT will most likely cause a less predictable
failure later on in a non-debug Xen.

> Alternately, maybe we can add an option either at compile time or at
> boot time for ASSERTs not to crash for your situation.

Making ASSERT not crash doesn't help (see above).

> But the fact that we have ASSERTs at all mean that we *expect* debug
> builds to crash.  If that's not what we want we need to get rid of the
> ASSERTs entirely.

????

David