8:15 a.m.
On 03/28/2009 07:51 AM, Daniel P. Berrange wrote:
On Thu, Mar 26, 2009 at 11:51:21AM -0400, Daniel J Walsh wrote:
> This patch fixes the seclabel handling in domain_conf.c to allow
> virt-manager to set the seclabel model, type and label.
>
> Also adds missing error messages when the xml is incorrect.
I'm not sure why this change to the XML parser is needed ? The calling
app is already able to supply a seclabel, provided it sets type='static'
to indicate a statically defined label. If it doesn't set this, then
libvirt will ignore anything i nthe XML, and generate a dynamic label.
This change is appears to just be making it parse dynamic label from
the XML, which shouldn't be needed.
Currently you get no error messages if you specify anything wrong the
code fails silently.
> How much verification should we be doing on this? I have another
patch
> that verifies the model as being a known model and a patch to verify the
> label is a correct label. (IE SELinux verifies the label is understood
> by the kernel.)
During the parsing stage, the only semantic validation we do is for
stuff listed in the hypervisor capabilities object (virCapabilitiesPtr).
There is a record of the security model in the capabilities object,
so you could validate that. Validating the actual user supplied
label would be done later at the time it is used.
But if a user specifies that a label that the libvirt system does not
understand, he needs to know that right away.
If I am in virt-manager and I specify
system_u:system_r:svirt1_t:s0
I really want to know then that I made a typo, not in a log file later
when the object fails to start.
More importantly from an MLS point of view, it would be good to know if
I spacify
system_u:system_r:svirt_t:TopSecret
I get an error back telling me the machine is only cleared to Secret.
> The problem with this second patch is it sucks in security.[ch],
> security_selinux.[ch] into the libvirt_lxc. Should I be doing this?
The second patch here seems to be empty ... ?
> --- a/src/domain_conf.c
> +++ b/src/domain_conf.c
> @@ -1859,12 +1859,28 @@ virSecurityLabelDefParseXML(virConnectPtr conn,
> if (virXPathNode(conn, "./seclabel", ctxt) == NULL)
> return 0;
>
> + p = virXPathStringLimit(conn, "string(./seclabel/@model)",
> + VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
> + if (p == NULL) {
> + virDomainReportError(conn, VIR_ERR_XML_ERROR,
> + "%s", _("missing seclabel model"));
> + goto error;
> + }
> + def->seclabel.model = p;
> +
> p = virXPathStringLimit(conn, "string(./seclabel/@type)",
> VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> - if (p == NULL)
> + if (p == NULL) {
> + virDomainReportError(conn, VIR_ERR_XML_ERROR,
> + "%s", _("missing seclabel type"));
> goto error;
> - if ((def->seclabel.type = virDomainSeclabelTypeFromString(p))< 0)
> + }
> +
> + if ((def->seclabel.type = virDomainSeclabelTypeFromString(p))< 0) {
> + virDomainReportError(conn, VIR_ERR_XML_ERROR,
> + _("unknown seclabel type %s"), p);
> goto error;
> + }
> VIR_FREE(p);
>
> /* Only parse details, if using static labels, or
> @@ -1872,16 +1888,14 @@ virSecurityLabelDefParseXML(virConnectPtr conn,
> */
> if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC ||
> !(flags& VIR_DOMAIN_XML_INACTIVE)) {
> - p = virXPathStringLimit(conn, "string(./seclabel/@model)",
> - VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
> - if (p == NULL)
> - goto error;
> - def->seclabel.model = p;
>
> p = virXPathStringLimit(conn, "string(./seclabel/label[1])",
> VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> - if (p == NULL)
> - goto error;
> + if (p == NULL) {
> + virDomainReportError(conn, VIR_ERR_XML_ERROR,
> + _("seclabel label is too long"));
> + goto error;
> + }
> def->seclabel.label = p;
> }
>
> @@ -1890,8 +1904,11 @@ virSecurityLabelDefParseXML(virConnectPtr conn,
> !(flags& VIR_DOMAIN_XML_INACTIVE)) {
> p = virXPathStringLimit(conn,
"string(./seclabel/imagelabel[1])",
> VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> - if (p == NULL)
> + if (p == NULL) {
> + virDomainReportError(conn, VIR_ERR_XML_ERROR,
> + _("seclabel image label is too long"));
> goto error;
> + }
> def->seclabel.imagelabel = p;
> }
>
> diff --git a/src/security_selinux.c b/src/security_selinux.c
> index 1708d55..5937f48 100644
Regards,
Daniel