[PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Ian Campbell]

system_u:system_r:domU_t is defined in the default policy and makes as
much sense as anything for a default.

This change required moving the call to domain_create_info_setdefault
to be before the ssid_label is translated into ssidref, which also
moves it before some other stuff which consumes things from c_info,
which is correct since setdefault should always be called first. Apart
from the SSID handling there should be no functional change (since
setdefault doesn't actually act on anything which that other stuff
uses).

There is no need to set exec_ssid_label since the default is to leave
the domain using the ssid_label after build.

I haven't done anything with the device model ssid.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Wei.Liu2@citrix.com
---
 docs/man/xl.cfg.pod.5      |    4 +++-
 tools/libxl/libxl_create.c |   11 ++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 8e4154f..fcca1cc 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -437,7 +437,9 @@ UUID will be generated.
 
 =item B<seclabel="LABEL">
 
-Assign an XSM security label to this domain.
+Assign an XSM security label to this domain. By default a domain is
+assigned the label B<system_u:system_r:domU_t>, which is defined in
+the default policy.
 
 =item B<init_seclabel="LABEL">
 
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index f0da7dc..4dd2ec2 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -42,6 +42,11 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
     libxl_defbool_setdefault(&c_info->run_hotplug_scripts, true);
     libxl_defbool_setdefault(&c_info->driver_domain, false);
 
+    if (!c_info->ssid_label) {
+        c_info->ssid_label = libxl__strdup(NOGC, "system_u:system_r:domU_t");
+        LOG(INFO, "Using default ssid_label: %s", c_info->ssid_label);
+    }
+
     return 0;
 }
 
@@ -802,6 +807,9 @@ static void initiate_domain_create(libxl__egc *egc,
 
     domid = 0;
 
+    ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
+    if (ret) goto error_out;
+
     if (d_config->c_info.ssid_label) {
         char *s = d_config->c_info.ssid_label;
         ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
@@ -887,9 +895,6 @@ static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
-    ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
-    if (ret) goto error_out;
-
     ret = libxl__domain_make(gc, d_config, &domid, &state->config);
     if (ret) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot make domain: %d", ret);
-- 
1.7.10.4

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Julien Grall]

Hi Ian,

On 14/05/15 11:33, Ian Campbell wrote:
> system_u:system_r:domU_t is defined in the default policy and makes as
> much sense as anything for a default.

So you rule out the possibility to run an unlabelled domain? This is
possible if the policy explicitly authorized it. That's a significant
change in the libxl behavior.

IHMO, having a default policy doesn't mean libxl should set a default
ssid to make XSM transparent to the user.

The explicit ssid makes clear that the guest is using a ssid foo and if
it's not provided then it will fail to boot.

Setting a default value may hide a bigger issue and take the wrong
policy the user forgot to set up an ssid.

> This change required moving the call to domain_create_info_setdefault
> to be before the ssid_label is translated into ssidref, which also
> moves it before some other stuff which consumes things from c_info,
> which is correct since setdefault should always be called first. Apart
> from the SSID handling there should be no functional change (since
> setdefault doesn't actually act on anything which that other stuff
> uses).
> 
> There is no need to set exec_ssid_label since the default is to leave
> the domain using the ssid_label after build.

By setting a ssid label, libxl will print a new warning on Xen not built
with XSM which will confuse the user:

libxl: warning: libxl_create.c:813:initiate_domain_create: XSM Disabled:
init_seclabel not supported

> 
> I haven't done anything with the device model ssid.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Cc: Wei.Liu2@citrix.com
> ---
>  docs/man/xl.cfg.pod.5      |    4 +++-
>  tools/libxl/libxl_create.c |   11 ++++++++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 8e4154f..fcca1cc 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -437,7 +437,9 @@ UUID will be generated.
>  
>  =item B<seclabel="LABEL">
>  
> -Assign an XSM security label to this domain.
> +Assign an XSM security label to this domain. By default a domain is
> +assigned the label B<system_u:system_r:domU_t>, which is defined in
> +the default policy.

It's not easy to know that seclabel will be stored in ssid_label.

It would be good to have this explanation into the toolstack API.

Regards,

-- 
Julien Grall

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Ian Campbell]

On Thu, 2015-05-14 at 12:21 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 14/05/15 11:33, Ian Campbell wrote:
> > system_u:system_r:domU_t is defined in the default policy and makes as
> > much sense as anything for a default.
> 
> So you rule out the possibility to run an unlabelled domain? This is
> possible if the policy explicitly authorized it. That's a significant
> change in the libxl behavior.

I didn't realise this was a possibility, wouldn't such a domain be
system_u:system_r:unlabeled_t> or something?

Note that this won't override a label which is just '' (i.e. an empty
string rather than NULL). I don't know if that results in the behaviour
you want.

When this was discussed before (in a conversation Wei started, but which
I can't find, maybe it was IRC rather than email) it seemed that
consensus was that by default things should Just Work as if XSM weren't
disabled, which is what I've implemented here.


> IHMO, having a default policy doesn't mean libxl should set a default
> ssid to make XSM transparent to the user.
> 
> The explicit ssid makes clear that the guest is using a ssid foo and if
> it's not provided then it will fail to boot.
> 
> Setting a default value may hide a bigger issue and take the wrong
> policy the user forgot to set up an ssid.

Does domU_t really have so many privileges that this is an issue? I'd
expect it to be almost totally privilegeless apart from things which any
domU needs.

The benefits of XSM seem to mainly apply to the various service domains.

Daniel, do you have an opinion here?

> 
> > This change required moving the call to domain_create_info_setdefault
> > to be before the ssid_label is translated into ssidref, which also
> > moves it before some other stuff which consumes things from c_info,
> > which is correct since setdefault should always be called first. Apart
> > from the SSID handling there should be no functional change (since
> > setdefault doesn't actually act on anything which that other stuff
> > uses).
> > 
> > There is no need to set exec_ssid_label since the default is to leave
> > the domain using the ssid_label after build.
> 
> By setting a ssid label, libxl will print a new warning on Xen not built
> with XSM which will confuse the user:
> 
> libxl: warning: libxl_create.c:813:initiate_domain_create: XSM Disabled:
> init_seclabel not supported

Ah, I didn't try that case. I'll see if I can work out a way to suppress
that warning.

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Julien Grall]

On 14/05/15 12:54, Ian Campbell wrote:
> On Thu, 2015-05-14 at 12:21 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 14/05/15 11:33, Ian Campbell wrote:
>>> system_u:system_r:domU_t is defined in the default policy and makes as
>>> much sense as anything for a default.
>>
>> So you rule out the possibility to run an unlabelled domain? This is
>> possible if the policy explicitly authorized it. That's a significant
>> change in the libxl behavior.
> 
> I didn't realise this was a possibility, wouldn't such a domain be
> system_u:system_r:unlabeled_t> or something?

I'm not sure how unlabeled works. I will let Daniel answer to this.

> Note that this won't override a label which is just '' (i.e. an empty
> string rather than NULL). I don't know if that results in the behaviour
> you want.

IIRC, NULL means unlabeled. '' would be translated as an invalid ssid
and throw an error.

Regards,

-- 
Julien Grall

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Daniel De Graaf]

On 05/14/2015 07:54 AM, Ian Campbell wrote:
> On Thu, 2015-05-14 at 12:21 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 14/05/15 11:33, Ian Campbell wrote:
>>> system_u:system_r:domU_t is defined in the default policy and makes as
>>> much sense as anything for a default.
>>
>> So you rule out the possibility to run an unlabelled domain? This is
>> possible if the policy explicitly authorized it. That's a significant
>> change in the libxl behavior.
>
> I didn't realise this was a possibility, wouldn't such a domain be
> system_u:system_r:unlabeled_t> or something?

Yes.  FLASK resolves any numeric SID value that is unused (including zero)
to the unlabeled sid (defined in tools/flask/policy/policy/initial_sids
to be system_u:system_r:unlabeled_t).  Because this could be the result of
an error (in the hypervisor, toolstack, etc), the use of unlabled_t for
real objects is discouraged in SELinux and XSM/FLASK.

> Note that this won't override a label which is just '' (i.e. an empty
> string rather than NULL). I don't know if that results in the behaviour
> you want.
>
> When this was discussed before (in a conversation Wei started, but which
> I can't find, maybe it was IRC rather than email) it seemed that
> consensus was that by default things should Just Work as if XSM weren't
> disabled, which is what I've implemented here.

I agree that this is a useful feature.  It is possible to extend the
initial_sids list with new entries that are used by the toolstack instead
of by the hypervisor, which could be used to define SECINITSID_DOMU as the
default label for a domU created by a toolstack without a label.  This is
better than hard-coding a string that may not be valid in a given security
policy, and it can be associated with a label that better reflects how the
policy wishes to treat domains with an "incomplete" configuration file.

The header file defining these SIDs is buried in the hypervisor source
tree (xen/xsm/flask/include/flask.h) and is only generated during a build
with XSM enabled.  It may be simpler to define the value in a shared header
and add a BUILD_BUG_ON somewhere in the flask code to check for mismatches.

>> IHMO, having a default policy doesn't mean libxl should set a default
>> ssid to make XSM transparent to the user.
>>
>> The explicit ssid makes clear that the guest is using a ssid foo and if
>> it's not provided then it will fail to boot.
>>
>> Setting a default value may hide a bigger issue and take the wrong
>> policy the user forgot to set up an ssid.
>
> Does domU_t really have so many privileges that this is an issue? I'd
> expect it to be almost totally privilegeless apart from things which any
> domU needs.
>
> The benefits of XSM seem to mainly apply to the various service domains.
>
> Daniel, do you have an opinion here?

In the example policy, domU_t should have the same level of access as a
normal domain (i.e. not device model stubdom) has with XSM disabled.

The only real difference is that the example policy does not allow any
domain to act as a device model to domU_t; it uses domHVM_t and dm_dom_t
for this.  If you want to use configurations with device model stubdoms
that also do not assign labels in the configuration, this distinction
will need to be removed.

>>> This change required moving the call to domain_create_info_setdefault
>>> to be before the ssid_label is translated into ssidref, which also
>>> moves it before some other stuff which consumes things from c_info,
>>> which is correct since setdefault should always be called first. Apart
>>> from the SSID handling there should be no functional change (since
>>> setdefault doesn't actually act on anything which that other stuff
>>> uses).
>>>
>>> There is no need to set exec_ssid_label since the default is to leave
>>> the domain using the ssid_label after build.
>>
>> By setting a ssid label, libxl will print a new warning on Xen not built
>> with XSM which will confuse the user:
>>
>> libxl: warning: libxl_create.c:813:initiate_domain_create: XSM Disabled:
>> init_seclabel not supported
>
> Ah, I didn't try that case. I'll see if I can work out a way to suppress
> that warning.

I would be fine with removing that warning completely; someone trying to
use XSM without it enabled will likely be able to figure out the problem
without this error, likely by noticing the "-" labels in xl list -v/-Z.

------------->8-----------------
Example patch adding SECINITSID_DOMU, for testing/reference.

---
diff --git a/tools/flask/policy/policy/initial_sids b/tools/flask/policy/policy/initial_sids
index 5de0bbf..48aad17 100644
--- a/tools/flask/policy/policy/initial_sids
+++ b/tools/flask/policy/policy/initial_sids
@@ -12,3 +12,4 @@ sid irq gen_context(system_u:object_r:irq_t,s0)
  sid iomem gen_context(system_u:object_r:iomem_t,s0)
  sid ioport gen_context(system_u:object_r:ioport_t,s0)
  sid device gen_context(system_u:object_r:device_t,s0)
+sid domU gen_context(system_u:system_r:domU_t,s0)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index f0da7dc..0c3d4ed 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -815,6 +815,8 @@ static void initiate_domain_create(libxl__egc *egc,
                  goto error_out;
              }
          }
+    } else {
+        d_config->c_info.ssidref = 11; /* SECINITSID_DOMU */
      }
  
      if (d_config->b_info.exec_ssid_label) {
diff --git a/xen/xsm/flask/policy/initial_sids b/xen/xsm/flask/policy/initial_sids
index e508bde..a442a38 100644
--- a/xen/xsm/flask/policy/initial_sids
+++ b/xen/xsm/flask/policy/initial_sids
@@ -13,4 +13,5 @@ sid ioport
  sid iomem
  sid irq
  sid device
+sid domU
  # FLASK

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Ian Campbell]

On Thu, 2015-05-14 at 19:09 -0400, Daniel De Graaf wrote:
> On 05/14/2015 07:54 AM, Ian Campbell wrote:
> > On Thu, 2015-05-14 at 12:21 +0100, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 14/05/15 11:33, Ian Campbell wrote:
> >>> system_u:system_r:domU_t is defined in the default policy and makes as
> >>> much sense as anything for a default.
> >>
> >> So you rule out the possibility to run an unlabelled domain? This is
> >> possible if the policy explicitly authorized it. That's a significant
> >> change in the libxl behavior.
> >
> > I didn't realise this was a possibility, wouldn't such a domain be
> > system_u:system_r:unlabeled_t> or something?
> 
> Yes.  FLASK resolves any numeric SID value that is unused (including zero)
> to the unlabeled sid (defined in tools/flask/policy/policy/initial_sids
> to be system_u:system_r:unlabeled_t).  Because this could be the result of
> an error (in the hypervisor, toolstack, etc), the use of unlabled_t for
> real objects is discouraged in SELinux and XSM/FLASK.

OK, so I think this rules out using unlabelled_t as a default.

> > Note that this won't override a label which is just '' (i.e. an empty
> > string rather than NULL). I don't know if that results in the behaviour
> > you want.
> >
> > When this was discussed before (in a conversation Wei started, but which
> > I can't find, maybe it was IRC rather than email) it seemed that
> > consensus was that by default things should Just Work as if XSM weren't
> > disabled, which is what I've implemented here.
> 
> I agree that this is a useful feature.  It is possible to extend the
> initial_sids list with new entries that are used by the toolstack instead
> of by the hypervisor, which could be used to define SECINITSID_DOMU as the
> default label for a domU created by a toolstack without a label.  This is
> better than hard-coding a string that may not be valid in a given security
> policy, and it can be associated with a label that better reflects how the
> policy wishes to treat domains with an "incomplete" configuration file.

That sounds good.

>From the PoV of the code in libxl this would be done by writing ssidref
as a literal number rather than translating a string (judging from your
example patch). That works for me.

While looking into this I noticed the existing code is
   if (ssid_label)
       ssid_ref = translate(ssid_label)

So my first patch has another issue which is that it will override a
users attempt to use ssid_ref themselves (which they are entitled to
do).

Is ssidref==0 "unused"? i.e. could we use it to differentiate whether
the field had been filled in by the user or not? If not is there some
other number? Or should we reserve one using the same technique you
suggest here for domU?

> The header file defining these SIDs is buried in the hypervisor source
> tree (xen/xsm/flask/include/flask.h) and is only generated during a build
> with XSM enabled.  It may be simpler to define the value in a shared header
> and add a BUILD_BUG_ON somewhere in the flask code to check for mismatches.

I was about to ask about this. Short of a pretty serious change to the
build a BUILD_BUG_ON seems like a reasonable approach.

> >> IHMO, having a default policy doesn't mean libxl should set a default
> >> ssid to make XSM transparent to the user.
> >>
> >> The explicit ssid makes clear that the guest is using a ssid foo and if
> >> it's not provided then it will fail to boot.
> >>
> >> Setting a default value may hide a bigger issue and take the wrong
> >> policy the user forgot to set up an ssid.
> >
> > Does domU_t really have so many privileges that this is an issue? I'd
> > expect it to be almost totally privilegeless apart from things which any
> > domU needs.
> >
> > The benefits of XSM seem to mainly apply to the various service domains.
> >
> > Daniel, do you have an opinion here?
> 
> In the example policy, domU_t should have the same level of access as a
> normal domain (i.e. not device model stubdom) has with XSM disabled.
> 
> The only real difference is that the example policy does not allow any
> domain to act as a device model to domU_t; it uses domHVM_t and dm_dom_t
> for this.  If you want to use configurations with device model stubdoms
> that also do not assign labels in the configuration, this distinction
> will need to be removed.

I'd be inclined to go the other way and either have a default ssid for
the DM or to fail if one isn't given (the latter would probably happen
anyway due to enforcement?).

Sounds like the default ssidref should be either ~= domU_t of domHVM_t
depending on the type of domain? (domU_t is really domPV_t?)

> 
> >>> This change required moving the call to domain_create_info_setdefault
> >>> to be before the ssid_label is translated into ssidref, which also
> >>> moves it before some other stuff which consumes things from c_info,
> >>> which is correct since setdefault should always be called first. Apart
> >>> from the SSID handling there should be no functional change (since
> >>> setdefault doesn't actually act on anything which that other stuff
> >>> uses).
> >>>
> >>> There is no need to set exec_ssid_label since the default is to leave
> >>> the domain using the ssid_label after build.
> >>
> >> By setting a ssid label, libxl will print a new warning on Xen not built
> >> with XSM which will confuse the user:
> >>
> >> libxl: warning: libxl_create.c:813:initiate_domain_create: XSM Disabled:
> >> init_seclabel not supported
> >
> > Ah, I didn't try that case. I'll see if I can work out a way to suppress
> > that warning.
> 
> I would be fine with removing that warning completely; someone trying to
> use XSM without it enabled will likely be able to figure out the problem
> without this error, likely by noticing the "-" labels in xl list -v/-Z.

Actually, I think using an SSID as the defaulted thing will remove this
message anyway (not sure if it causes another later, I'll check).

> ------------->8-----------------
> Example patch adding SECINITSID_DOMU, for testing/reference.
> 
> ---
> diff --git a/tools/flask/policy/policy/initial_sids b/tools/flask/policy/policy/initial_sids
> index 5de0bbf..48aad17 100644
> --- a/tools/flask/policy/policy/initial_sids
> +++ b/tools/flask/policy/policy/initial_sids
> @@ -12,3 +12,4 @@ sid irq gen_context(system_u:object_r:irq_t,s0)
>   sid iomem gen_context(system_u:object_r:iomem_t,s0)
>   sid ioport gen_context(system_u:object_r:ioport_t,s0)
>   sid device gen_context(system_u:object_r:device_t,s0)
> +sid domU gen_context(system_u:system_r:domU_t,s0)
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index f0da7dc..0c3d4ed 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -815,6 +815,8 @@ static void initiate_domain_create(libxl__egc *egc,
>                   goto error_out;
>               }
>           }
> +    } else {
> +        d_config->c_info.ssidref = 11; /* SECINITSID_DOMU */
>       }
>   
>       if (d_config->b_info.exec_ssid_label) {
> diff --git a/xen/xsm/flask/policy/initial_sids b/xen/xsm/flask/policy/initial_sids
> index e508bde..a442a38 100644
> --- a/xen/xsm/flask/policy/initial_sids
> +++ b/xen/xsm/flask/policy/initial_sids
> @@ -13,4 +13,5 @@ sid ioport
>   sid iomem
>   sid irq
>   sid device
> +sid domU
>   # FLASK
> 

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Daniel De Graaf]

On 05/15/2015 05:39 AM, Ian Campbell wrote:
> On Thu, 2015-05-14 at 19:09 -0400, Daniel De Graaf wrote:
>> On 05/14/2015 07:54 AM, Ian Campbell wrote:
>>> On Thu, 2015-05-14 at 12:21 +0100, Julien Grall wrote:
>>>> Hi Ian,
>>>>
>>>> On 14/05/15 11:33, Ian Campbell wrote:
>>>>> system_u:system_r:domU_t is defined in the default policy and makes as
>>>>> much sense as anything for a default.
>>>>
>>>> So you rule out the possibility to run an unlabelled domain? This is
>>>> possible if the policy explicitly authorized it. That's a significant
>>>> change in the libxl behavior.
>>>
>>> I didn't realise this was a possibility, wouldn't such a domain be
>>> system_u:system_r:unlabeled_t> or something?
>>
>> Yes.  FLASK resolves any numeric SID value that is unused (including zero)
>> to the unlabeled sid (defined in tools/flask/policy/policy/initial_sids
>> to be system_u:system_r:unlabeled_t).  Because this could be the result of
>> an error (in the hypervisor, toolstack, etc), the use of unlabled_t for
>> real objects is discouraged in SELinux and XSM/FLASK.
>
> OK, so I think this rules out using unlabelled_t as a default.
>
>>> Note that this won't override a label which is just '' (i.e. an empty
>>> string rather than NULL). I don't know if that results in the behaviour
>>> you want.
>>>
>>> When this was discussed before (in a conversation Wei started, but which
>>> I can't find, maybe it was IRC rather than email) it seemed that
>>> consensus was that by default things should Just Work as if XSM weren't
>>> disabled, which is what I've implemented here.
>>
>> I agree that this is a useful feature.  It is possible to extend the
>> initial_sids list with new entries that are used by the toolstack instead
>> of by the hypervisor, which could be used to define SECINITSID_DOMU as the
>> default label for a domU created by a toolstack without a label.  This is
>> better than hard-coding a string that may not be valid in a given security
>> policy, and it can be associated with a label that better reflects how the
>> policy wishes to treat domains with an "incomplete" configuration file.
>
> That sounds good.
>
>>From the PoV of the code in libxl this would be done by writing ssidref
> as a literal number rather than translating a string (judging from your
> example patch). That works for me.
>
> While looking into this I noticed the existing code is
>     if (ssid_label)
>         ssid_ref = translate(ssid_label)
>
> So my first patch has another issue which is that it will override a
> users attempt to use ssid_ref themselves (which they are entitled to
> do).
>
> Is ssidref==0 "unused"? i.e. could we use it to differentiate whether
> the field had been filled in by the user or not? If not is there some
> other number? Or should we reserve one using the same technique you
> suggest here for domU?

Yes, a value of zero is "not populated"; no valid context resolves to a
SID of zero (including unlabeled_t, which is properly 5 or so).

>> The header file defining these SIDs is buried in the hypervisor source
>> tree (xen/xsm/flask/include/flask.h) and is only generated during a build
>> with XSM enabled.  It may be simpler to define the value in a shared header
>> and add a BUILD_BUG_ON somewhere in the flask code to check for mismatches.
>
> I was about to ask about this. Short of a pretty serious change to the
> build a BUILD_BUG_ON seems like a reasonable approach.
>
>>>> IHMO, having a default policy doesn't mean libxl should set a default
>>>> ssid to make XSM transparent to the user.
>>>>
>>>> The explicit ssid makes clear that the guest is using a ssid foo and if
>>>> it's not provided then it will fail to boot.
>>>>
>>>> Setting a default value may hide a bigger issue and take the wrong
>>>> policy the user forgot to set up an ssid.
>>>
>>> Does domU_t really have so many privileges that this is an issue? I'd
>>> expect it to be almost totally privilegeless apart from things which any
>>> domU needs.
>>>
>>> The benefits of XSM seem to mainly apply to the various service domains.
>>>
>>> Daniel, do you have an opinion here?
>>
>> In the example policy, domU_t should have the same level of access as a
>> normal domain (i.e. not device model stubdom) has with XSM disabled.
>>
>> The only real difference is that the example policy does not allow any
>> domain to act as a device model to domU_t; it uses domHVM_t and dm_dom_t
>> for this.  If you want to use configurations with device model stubdoms
>> that also do not assign labels in the configuration, this distinction
>> will need to be removed.
>
> I'd be inclined to go the other way and either have a default ssid for
> the DM or to fail if one isn't given (the latter would probably happen
> anyway due to enforcement?).

Yes, it would probably fail at xc_domain_set_target in enforcing mode.

> Sounds like the default ssidref should be either ~= domU_t of domHVM_t
> depending on the type of domain? (domU_t is really domPV_t?)

The domU_t type also works for HVM domains with the device model in dom0.

Looking at the problem again, I think a second initial SID for the device
model would be preferable, removing domHVM_t completely.  There are already
other example types in the policy for domains that do not use a device model
(isolated_domU_t is probably the best example), and the result more closely
matches the permissions used in the hypervisor without XSM enabled.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Ian Campbell]

On Fri, 2015-05-15 at 13:09 -0400, Daniel De Graaf wrote:
> > I'd be inclined to go the other way and either have a default ssid for
> > the DM or to fail if one isn't given (the latter would probably happen
> > anyway due to enforcement?).
> 
> Yes, it would probably fail at xc_domain_set_target in enforcing mode.
> 
> > Sounds like the default ssidref should be either ~= domU_t of domHVM_t
> > depending on the type of domain? (domU_t is really domPV_t?)
> 
> The domU_t type also works for HVM domains with the device model in dom0.
> 
> Looking at the problem again, I think a second initial SID for the device
> model would be preferable, removing domHVM_t completely.  There are already
> other example types in the policy for domains that do not use a device model
> (isolated_domU_t is probably the best example), and the result more closely
> matches the permissions used in the hypervisor without XSM enabled.

I'm aroundabout half sure what you are proposing here, but I trust it
makes sense ;-).

I think for now I will investigate using a default ssid for all domains,
which AIUI from above will work out of the box with PV guests and HVM
ones which have qemu in dom0.

For the stubdom case I think I'll leave it to you to change the default
policy, at which point I'll be happy to extend things to a default ssid
for stubdom too.

Ian.

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Ian Campbell]

On Fri, 2015-05-15 at 10:39 +0100, Ian Campbell wrote:
> > The header file defining these SIDs is buried in the hypervisor source
> > tree (xen/xsm/flask/include/flask.h) and is only generated during a build
> > with XSM enabled.  It may be simpler to define the value in a shared header
> > and add a BUILD_BUG_ON somewhere in the flask code to check for mismatches.
> 
> I was about to ask about this. Short of a pretty serious change to the
> build a BUILD_BUG_ON seems like a reasonable approach.

To what extent is a user's customized (e.g. potentially clean room
implemented) policy required to match what goes on here? I suspect the
answer is "fully" and that any custom policy must therefore use exactly
the policy/security_classes and policy/initial_sids as was used when Xen
was built.

If this coupling already exists then I see no particular harm in
extending it to the tools as well, although I think I might see about
making the header available for checking even in non-xsm builds (since I
don't really want to add an xsm compile time option to the tools, nor to
rely on the xsm builds catching errors for non-xsm usage).

BTW, I seem to have two of each of security_classes and initial_sids in
my tree, one in tools/flask/policy/policy/ and the other in
xen/xsm/flask/policy/ and they appear to differ.

The xen initial_sids appears to be derived from the tools one, whereas
security_classes looks different.

Ian.

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Daniel De Graaf]

On 05/18/2015 08:38 AM, Ian Campbell wrote:
> On Fri, 2015-05-15 at 10:39 +0100, Ian Campbell wrote:
>>> The header file defining these SIDs is buried in the hypervisor source
>>> tree (xen/xsm/flask/include/flask.h) and is only generated during a build
>>> with XSM enabled.  It may be simpler to define the value in a shared header
>>> and add a BUILD_BUG_ON somewhere in the flask code to check for mismatches.
>>
>> I was about to ask about this. Short of a pretty serious change to the
>> build a BUILD_BUG_ON seems like a reasonable approach.
>
> To what extent is a user's customized (e.g. potentially clean room
> implemented) policy required to match what goes on here? I suspect the
> answer is "fully" and that any custom policy must therefore use exactly
> the policy/security_classes and policy/initial_sids as was used when Xen
> was built.

When rewriting the security policy, xen/xsm/flask/policy/initial_sids is
expected to remain unchanged, while tools/flask/policy/policy/initial_sids
can be modified to suit the types defined in the rewritten policy.  This
applies to all the files split between the two directories.

> If this coupling already exists then I see no particular harm in
> extending it to the tools as well, although I think I might see about
> making the header available for checking even in non-xsm builds (since I
> don't really want to add an xsm compile time option to the tools, nor to
> rely on the xsm builds catching errors for non-xsm usage).
>
> BTW, I seem to have two of each of security_classes and initial_sids in
> my tree, one in tools/flask/policy/policy/ and the other in
> xen/xsm/flask/policy/ and they appear to differ.
>
> The xen initial_sids appears to be derived from the tools one, whereas
> security_classes looks different.

The security_classes and access_vectors in the local policy (tools/...)
are intended to be used by components outside the hypervisor that do not
implement their own security policy.  The current example policy defines
a class for xenstore permissions, but since xenstore does not actually
use this, it is just an example.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Ian Campbell]

On Mon, 2015-05-18 at 18:37 -0400, Daniel De Graaf wrote:
> On 05/18/2015 08:38 AM, Ian Campbell wrote:
> > On Fri, 2015-05-15 at 10:39 +0100, Ian Campbell wrote:
> >>> The header file defining these SIDs is buried in the hypervisor source
> >>> tree (xen/xsm/flask/include/flask.h) and is only generated during a build
> >>> with XSM enabled.  It may be simpler to define the value in a shared header
> >>> and add a BUILD_BUG_ON somewhere in the flask code to check for mismatches.
> >>
> >> I was about to ask about this. Short of a pretty serious change to the
> >> build a BUILD_BUG_ON seems like a reasonable approach.
> >
> > To what extent is a user's customized (e.g. potentially clean room
> > implemented) policy required to match what goes on here? I suspect the
> > answer is "fully" and that any custom policy must therefore use exactly
> > the policy/security_classes and policy/initial_sids as was used when Xen
> > was built.
> 
> When rewriting the security policy, xen/xsm/flask/policy/initial_sids is
> expected to remain unchanged, while tools/flask/policy/policy/initial_sids
> can be modified to suit the types defined in the rewritten policy.  This
> applies to all the files split between the two directories.

Makes sense.

>From the PoV of this series I think I just need to expose
xen/xsm/flask/policy/initial_sids (via mkflask.sh to generate a header)
to the tools.

I'll arrange for that to happen in v2.

Thanks,

Ian.

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Wei Liu]

On Thu, May 14, 2015 at 11:33:45AM +0100, Ian Campbell wrote:
> system_u:system_r:domU_t is defined in the default policy and makes as
> much sense as anything for a default.
> 
> This change required moving the call to domain_create_info_setdefault
> to be before the ssid_label is translated into ssidref, which also
> moves it before some other stuff which consumes things from c_info,
> which is correct since setdefault should always be called first. Apart
> from the SSID handling there should be no functional change (since
> setdefault doesn't actually act on anything which that other stuff
> uses).
> 
> There is no need to set exec_ssid_label since the default is to leave
> the domain using the ssid_label after build.
> 
> I haven't done anything with the device model ssid.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Cc: Wei.Liu2@citrix.com
> ---
>  docs/man/xl.cfg.pod.5      |    4 +++-
>  tools/libxl/libxl_create.c |   11 ++++++++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 8e4154f..fcca1cc 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -437,7 +437,9 @@ UUID will be generated.
>  
>  =item B<seclabel="LABEL">
>  
> -Assign an XSM security label to this domain.
> +Assign an XSM security label to this domain. By default a domain is
> +assigned the label B<system_u:system_r:domU_t>, which is defined in
> +the default policy.
>  
>  =item B<init_seclabel="LABEL">
>  
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index f0da7dc..4dd2ec2 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -42,6 +42,11 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
>      libxl_defbool_setdefault(&c_info->run_hotplug_scripts, true);
>      libxl_defbool_setdefault(&c_info->driver_domain, false);
>  
> +    if (!c_info->ssid_label) {
> +        c_info->ssid_label = libxl__strdup(NOGC, "system_u:system_r:domU_t");
> +        LOG(INFO, "Using default ssid_label: %s", c_info->ssid_label);

I don't think this is right. For one, the label you hardcoded here 
is defined in the policy we ship. It doesn't necessarily exist in the
policy that is loaded by system admin.

Another thing, as Julien said, is that this generates a warning in Xen
that is not compiled with XSM support.

By definition if you don't label a domain, it should be labeled as
"unlabeled". We already do the right thing.

Wei.
> +    }
> +
>      return 0;
>  }
>  
> @@ -802,6 +807,9 @@ static void initiate_domain_create(libxl__egc *egc,
>  
>      domid = 0;
>  
> +    ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
> +    if (ret) goto error_out;
> +
>      if (d_config->c_info.ssid_label) {
>          char *s = d_config->c_info.ssid_label;
>          ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
> @@ -887,9 +895,6 @@ static void initiate_domain_create(libxl__egc *egc,
>          goto error_out;
>      }
>  
> -    ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
> -    if (ret) goto error_out;
> -
>      ret = libxl__domain_make(gc, d_config, &domid, &state->config);
>      if (ret) {
>          LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot make domain: %d", ret);
> -- 
> 1.7.10.4

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Ian Campbell]

On Thu, 2015-05-14 at 12:58 +0100, Wei Liu wrote:
> On Thu, May 14, 2015 at 11:33:45AM +0100, Ian Campbell wrote:
> > system_u:system_r:domU_t is defined in the default policy and makes as
> > much sense as anything for a default.
> > 
> > This change required moving the call to domain_create_info_setdefault
> > to be before the ssid_label is translated into ssidref, which also
> > moves it before some other stuff which consumes things from c_info,
> > which is correct since setdefault should always be called first. Apart
> > from the SSID handling there should be no functional change (since
> > setdefault doesn't actually act on anything which that other stuff
> > uses).
> > 
> > There is no need to set exec_ssid_label since the default is to leave
> > the domain using the ssid_label after build.
> > 
> > I haven't done anything with the device model ssid.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> > Cc: Wei.Liu2@citrix.com
> > ---
> >  docs/man/xl.cfg.pod.5      |    4 +++-
> >  tools/libxl/libxl_create.c |   11 ++++++++---
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index 8e4154f..fcca1cc 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -437,7 +437,9 @@ UUID will be generated.
> >  
> >  =item B<seclabel="LABEL">
> >  
> > -Assign an XSM security label to this domain.
> > +Assign an XSM security label to this domain. By default a domain is
> > +assigned the label B<system_u:system_r:domU_t>, which is defined in
> > +the default policy.
> >  
> >  =item B<init_seclabel="LABEL">
> >  
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index f0da7dc..4dd2ec2 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -42,6 +42,11 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
> >      libxl_defbool_setdefault(&c_info->run_hotplug_scripts, true);
> >      libxl_defbool_setdefault(&c_info->driver_domain, false);
> >  
> > +    if (!c_info->ssid_label) {
> > +        c_info->ssid_label = libxl__strdup(NOGC, "system_u:system_r:domU_t");
> > +        LOG(INFO, "Using default ssid_label: %s", c_info->ssid_label);
> 
> I don't think this is right. For one, the label you hardcoded here 
> is defined in the policy we ship. It doesn't necessarily exist in the
> policy that is loaded by system admin.

Personally I think that's fine, you either use the default, or you make
sure your custom policy has a domU_t role (a very reasonable thing to
have) or you specify something custom for every domain.

> Another thing, as Julien said, is that this generates a warning in Xen
> that is not compiled with XSM support.
> 
> By definition if you don't label a domain, it should be labeled as
> "unlabeled". We already do the right thing.

So how come osstest is failing? What should we do instead?

Ian.

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Wei Liu]

On Thu, May 14, 2015 at 01:32:04PM +0100, Ian Campbell wrote:
> On Thu, 2015-05-14 at 12:58 +0100, Wei Liu wrote:
> > On Thu, May 14, 2015 at 11:33:45AM +0100, Ian Campbell wrote:
> > > system_u:system_r:domU_t is defined in the default policy and makes as
> > > much sense as anything for a default.
> > > 
> > > This change required moving the call to domain_create_info_setdefault
> > > to be before the ssid_label is translated into ssidref, which also
> > > moves it before some other stuff which consumes things from c_info,
> > > which is correct since setdefault should always be called first. Apart
> > > from the SSID handling there should be no functional change (since
> > > setdefault doesn't actually act on anything which that other stuff
> > > uses).
> > > 
> > > There is no need to set exec_ssid_label since the default is to leave
> > > the domain using the ssid_label after build.
> > > 
> > > I haven't done anything with the device model ssid.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> > > Cc: Wei.Liu2@citrix.com
> > > ---
> > >  docs/man/xl.cfg.pod.5      |    4 +++-
> > >  tools/libxl/libxl_create.c |   11 ++++++++---
> > >  2 files changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > > index 8e4154f..fcca1cc 100644
> > > --- a/docs/man/xl.cfg.pod.5
> > > +++ b/docs/man/xl.cfg.pod.5
> > > @@ -437,7 +437,9 @@ UUID will be generated.
> > >  
> > >  =item B<seclabel="LABEL">
> > >  
> > > -Assign an XSM security label to this domain.
> > > +Assign an XSM security label to this domain. By default a domain is
> > > +assigned the label B<system_u:system_r:domU_t>, which is defined in
> > > +the default policy.
> > >  
> > >  =item B<init_seclabel="LABEL">
> > >  
> > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > > index f0da7dc..4dd2ec2 100644
> > > --- a/tools/libxl/libxl_create.c
> > > +++ b/tools/libxl/libxl_create.c
> > > @@ -42,6 +42,11 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
> > >      libxl_defbool_setdefault(&c_info->run_hotplug_scripts, true);
> > >      libxl_defbool_setdefault(&c_info->driver_domain, false);
> > >  
> > > +    if (!c_info->ssid_label) {
> > > +        c_info->ssid_label = libxl__strdup(NOGC, "system_u:system_r:domU_t");
> > > +        LOG(INFO, "Using default ssid_label: %s", c_info->ssid_label);
> > 
> > I don't think this is right. For one, the label you hardcoded here 
> > is defined in the policy we ship. It doesn't necessarily exist in the
> > policy that is loaded by system admin.
> 
> Personally I think that's fine, you either use the default, or you make
> sure your custom policy has a domU_t role (a very reasonable thing to
> have) or you specify something custom for every domain.
> 
> > Another thing, as Julien said, is that this generates a warning in Xen
> > that is not compiled with XSM support.
> > 
> > By definition if you don't label a domain, it should be labeled as
> > "unlabeled". We already do the right thing.
> 
> So how come osstest is failing? What should we do instead?
> 

AIUI current policy doesn't allow unlabeled domain to do certain things.
Maybe we can figure out a way to tune the policy to give certain
permissions to unlabeled domain.

This would need input from Daniel to know if it is achievable.

Wei.

> Ian.

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Julien Grall]

On 14/05/15 13:39, Wei Liu wrote:
>> So how come osstest is failing? What should we do instead?
>>
> 
> AIUI current policy doesn't allow unlabeled domain to do certain things.
> Maybe we can figure out a way to tune the policy to give certain
> permissions to unlabeled domain.

What about adding a seclabel entry in the domain configuration file?

Regards,

-- 
Julien Grall

Re: [PATCH] libxl: assigned a default ssid_label (XSM label) to guests

[Ian Campbell]

On Thu, 2015-05-14 at 15:05 +0100, Julien Grall wrote:
> On 14/05/15 13:39, Wei Liu wrote:
> >> So how come osstest is failing? What should we do instead?
> >>
> > 
> > AIUI current policy doesn't allow unlabeled domain to do certain things.
> > Maybe we can figure out a way to tune the policy to give certain
> > permissions to unlabeled domain.
> 
> What about adding a seclabel entry in the domain configuration file?

I think Ian J has argued against that, but I will let him speak for
himself in case I'm misremembering.

Ian.