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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

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