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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel