-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jim Meyering wrote:

Daniel J Walsh <dwalsh@redhat.com> wrote:

...

Is this acceptable to upstream?

Hi Daniel and James,

I tried to apply that using "git apply FILE" and that failed due to fuzz and white-space problems. I resorted to using "patch -p1 < FILE", which accommodated. First step was to run "make". that passed. good ;-) Then "make check". So did that. doubly-good. Finally "make syntax-check". That highlighted several problems, which I've corrected below, so now it passes. Also, while glancing through, I stumbled across a potential leak and fixed it. Haven't gone through it thoroughly.

Below I've included the incremental changes to address those problems, along with the rediff'd original. You're welcome to fold those into your patch, in case you're up for another iteration.

One thing I didn't address: In the new virXPathStringLimit function, note that virXPathString returning NULL can also mean out-of-memory (which is diagnosed by virXPathString itself), in which case, the diagnostic from virXPathStringLimit would be wrong.

Also, please consider adding (or at least outlining) a few simple tests to exercise some of the new code. At the very least, the parsing of the new XML elements has to be exercised. Also, please update docs/schemas/capability.rng.

Of course, such a large change deserves more of a ChangeLog entry and at least a little documentation.

From ea230077f6e8bccf085b1ec2febebc10144d44da Mon Sep 17 00:00:00 2001 From: James Morris <jmorris@namei.org> Date: Tue, 17 Feb 2009 18:50:24 +0100 Subject: [PATCH 1/5] selinux support

--- include/libvirt/libvirt.h | 67 +++++++++++++++ include/libvirt/libvirt.h.in | 67 +++++++++++++++ include/libvirt/virterror.h | 2 + po/POTFILES.in | 2 + python/generator.py | 2 + qemud/Makefile.am | 1 + qemud/remote.c | 70 ++++++++++++++++ qemud/remote_dispatch_args.h | 1 + qemud/remote_dispatch_prototypes.h | 14 +++ qemud/remote_dispatch_ret.h | 2 + qemud/remote_dispatch_table.h | 10 +++ qemud/remote_protocol.c | 37 +++++++++ qemud/remote_protocol.h | 37 +++++++++ qemud/remote_protocol.x | 34 ++++++++- src/Makefile.am | 25 +++++- src/capabilities.c | 10 +++ src/capabilities.h | 7 ++ src/domain_conf.c | 51 ++++++++++++ src/domain_conf.h | 9 ++ src/driver.h | 8 ++ src/libvirt.c | 64 +++++++++++++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 3 +- src/lxc_driver.c | 2 + src/openvz_driver.c | 2 + src/qemu_conf.h | 3 + src/qemu_driver.c | 158 ++++++++++++++++++++++++++++++++++++ src/remote_internal.c | 63 ++++++++++++++ src/security.c | 133 ++++++++++++++++++++++++++++++ src/security.h | 72 ++++++++++++++++ src/security_selinux.c | 108 ++++++++++++++++++++++++ src/security_selinux.h | 18 ++++ src/storage_backend.c | 1 + src/test.c | 2 + src/uml_driver.c | 2 + src/virsh.c | 26 ++++++ src/virterror.c | 9 ++ src/xml.c | 33 ++++++++ src/xml.h | 4 + tests/daemon-conf | 3 + 40 files changed, 1159 insertions(+), 4 deletions(-) create mode 100644 src/security.c create mode 100644 src/security.h create mode 100644 src/security_selinux.c create mode 100644 src/security_selinux.h

diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h index e32d40b..c8489e8 100644 --- a/include/libvirt/libvirt.h +++ b/include/libvirt/libvirt.h @@ -111,6 +111,68 @@ typedef enum { } virDomainCreateFlags;

/** + * VIR_SECURITY_LABEL_BUFLEN: + * + * Macro providing the maximum length of the virSecurityLabel label string. + * Note that this value is based on that used by Labeled NFS. + */ +#define VIR_SECURITY_LABEL_BUFLEN (4096 + 1) + +/** + * virSecurityLabel: + * + * a virSecurityLabel is a structure filled by virDomainGetSecurityLabel(), + * providing the security label and associated attributes for the specified + * domain. + * + */ +typedef struct _virSecurityLabel { + char label[VIR_SECURITY_LABEL_BUFLEN]; /* security label string */ + int enforcing; /* 1 if security policy is being enforced for domain */ +} virSecurityLabel; + +/** + * virSecurityLabelPtr: + * + * a virSecurityLabelPtr is a pointer to a virSecurityLabel. + */ +typedef virSecurityLabel *virSecurityLabelPtr; + +/** + * VIR_SECURITY_MODEL_BUFLEN: + * + * Macro providing the maximum length of the virSecurityModel model string. + */ +#define VIR_SECURITY_MODEL_BUFLEN (256 + 1) + +/** + * VIR_SECURITY_DOI_BUFLEN: + * + * Macro providing the maximum length of the virSecurityModel doi string. + */ +#define VIR_SECURITY_DOI_BUFLEN (256 + 1) + +/** + * virSecurityModel: + * + * a virSecurityModel is a structure filled by virNodeGetSecurityModel(), + * providing the per-hypervisor security model and DOI attributes for the + * specified domain. + * + */ +typedef struct _virSecurityModel { + char model[VIR_SECURITY_MODEL_BUFLEN]; /* security model string */ + char doi[VIR_SECURITY_DOI_BUFLEN]; /* domain of interpetation */ +} virSecurityModel; + +/** + * virSecurityModelPtr: + * + * a virSecurityModelPtr is a pointer to a virSecurityModel. + */ +typedef virSecurityModel *virSecurityModelPtr; + +/** * virNodeInfoPtr: * * a virNodeInfo is a structure filled by virNodeGetInfo() and providing @@ -417,6 +479,9 @@ char * virConnectGetCapabilities (virConnectPtr conn);

unsigned long long virNodeGetFreeMemory (virConnectPtr conn);

+int virNodeGetSecurityModel (virConnectPtr conn, + virSecurityModelPtr secmodel); + /* * Gather list of running domains */ @@ -506,6 +571,8 @@ int virDomainSetMaxMemory (virDomainPtr domain, int virDomainSetMemory (virDomainPtr domain, unsigned long memory); int virDomainGetMaxVcpus (virDomainPtr domain); +int virDomainGetSecurityLabel (virDomainPtr domain, + virSecurityLabelPtr seclabel);

/* * XML domain description diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index af97bfd..492c06c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -111,6 +111,68 @@ typedef enum { } virDomainCreateFlags;

/** + * VIR_SECURITY_LABEL_BUFLEN: + * + * Macro providing the maximum length of the virSecurityLabel label string. + * Note that this value is based on that used by Labeled NFS. + */ +#define VIR_SECURITY_LABEL_BUFLEN (4096 + 1) + +/** + * virSecurityLabel: + * + * a virSecurityLabel is a structure filled by virDomainGetSecurityLabel(), + * providing the security label and associated attributes for the specified + * domain. + * + */ +typedef struct _virSecurityLabel { + char label[VIR_SECURITY_LABEL_BUFLEN]; /* security label string */ + int enforcing; /* 1 if security policy is being enforced for domain */ +} virSecurityLabel; + +/** + * virSecurityLabelPtr: + * + * a virSecurityLabelPtr is a pointer to a virSecurityLabel. + */ +typedef virSecurityLabel *virSecurityLabelPtr; + +/** + * VIR_SECURITY_MODEL_BUFLEN: + * + * Macro providing the maximum length of the virSecurityModel model string. + */ +#define VIR_SECURITY_MODEL_BUFLEN (256 + 1) + +/** + * VIR_SECURITY_DOI_BUFLEN: + * + * Macro providing the maximum length of the virSecurityModel doi string. + */ +#define VIR_SECURITY_DOI_BUFLEN (256 + 1) + +/** + * virSecurityModel: + * + * a virSecurityModel is a structure filled by virNodeGetSecurityModel(), + * providing the per-hypervisor security model and DOI attributes for the + * specified domain. + * + */ +typedef struct _virSecurityModel { + char model[VIR_SECURITY_MODEL_BUFLEN]; /* security model string */ + char doi[VIR_SECURITY_DOI_BUFLEN]; /* domain of interpetation */ +} virSecurityModel; + +/** + * virSecurityModelPtr: + * + * a virSecurityModelPtr is a pointer to a virSecurityModel. + */ +typedef virSecurityModel *virSecurityModelPtr; + +/** * virNodeInfoPtr: * * a virNodeInfo is a structure filled by virNodeGetInfo() and providing @@ -417,6 +479,9 @@ char * virConnectGetCapabilities (virConnectPtr conn);

unsigned long long virNodeGetFreeMemory (virConnectPtr conn);

+int virNodeGetSecurityModel (virConnectPtr conn, + virSecurityModelPtr secmodel); + /* * Gather list of running domains */ @@ -506,6 +571,8 @@ int virDomainSetMaxMemory (virDomainPtr domain, int virDomainSetMemory (virDomainPtr domain, unsigned long memory); int virDomainGetMaxVcpus (virDomainPtr domain); +int virDomainGetSecurityLabel (virDomainPtr domain, + virSecurityLabelPtr seclabel);

/* * XML domain description diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index d68201f..2c3777d 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -61,6 +61,7 @@ typedef enum { VIR_FROM_UML, /* Error at the UML driver */ VIR_FROM_NODEDEV, /* Error from node device monitor */ VIR_FROM_XEN_INOTIFY, /* Error from xen inotify layer */ + VIR_FROM_SECURITY, /* Error from security framework */ } virErrorDomain;

@@ -154,6 +155,7 @@ typedef enum { VIR_WAR_NO_NODE, /* failed to start node driver */ VIR_ERR_INVALID_NODE_DEVICE,/* invalid node device object */ VIR_ERR_NO_NODE_DEVICE,/* node device not found */ + VIR_ERR_NO_SECURITY_MODEL, /* security model not found */ } virErrorNumber;

/** diff --git a/po/POTFILES.in b/po/POTFILES.in index 7461f93..5794479 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -22,6 +22,8 @@ src/proxy_internal.c src/qemu_conf.c src/qemu_driver.c src/remote_internal.c +src/security.c +src/security_selinux.c src/storage_backend.c src/storage_backend_disk.c src/storage_backend_fs.c diff --git a/python/generator.py b/python/generator.py index 0e8cca7..dcad499 100755 --- a/python/generator.py +++ b/python/generator.py @@ -342,6 +342,8 @@ skip_function = ( 'virCopyLastError', # Python API is called virGetLastError instead 'virConnectOpenAuth', # Python C code is manually written 'virDefaultErrorFunc', # Python virErrorFuncHandler impl calls this from C + 'virDomainGetSecurityLabel', # Needs investigation... + 'virNodeGetSecurityModel', # Needs investigation... 'virConnectDomainEventRegister', # overridden in virConnect.py 'virConnectDomainEventDeregister', # overridden in virConnect.py 'virSaveLastError', # We have our own python error wrapper diff --git a/qemud/Makefile.am b/qemud/Makefile.am index 924e8ad..d3ab817 100644 --- a/qemud/Makefile.am +++ b/qemud/Makefile.am @@ -133,6 +133,7 @@ if WITH_NODE_DEVICES endif endif

+libvirtd_LDADD += ../src/libvirt_driver_security.la libvirtd_LDADD += ../src/libvirt.la

if HAVE_POLKIT diff --git a/qemud/remote.c b/qemud/remote.c index 78dda42..7a91384 100644 --- a/qemud/remote.c +++ b/qemud/remote.c @@ -1345,6 +1345,76 @@ remoteDispatchDomainGetMaxVcpus (struct qemud_server *server ATTRIBUTE_UNUSED, }

static int +remoteDispatchDomainGetSecurityLabel(struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_error *rerr, + remote_domain_get_security_label_args *args, + remote_domain_get_security_label_ret *ret) +{ + virDomainPtr dom; + virSecurityLabel seclabel; + + dom = get_nonnull_domain(conn, args->dom); + if (dom == NULL) { + remoteDispatchConnError(rerr, conn); + return -1; + } + + memset(&seclabel, 0, sizeof seclabel); + if (virDomainGetSecurityLabel(dom, &seclabel) == -1) { + virDomainFree(dom); + remoteDispatchFormatError(rerr, "%s", _("unable to get security label")); + return -1; + } + + ret->label.label_len = strlen(seclabel.label) + 1; + if (VIR_ALLOC_N(ret->label.label_val, ret->label.label_len) < 0) { + virDomainFree(dom); + remoteDispatchOOMError(rerr); + return -1; + } + strcpy(ret->label.label_val, seclabel.label); + ret->enforcing = seclabel.enforcing; + virDomainFree(dom); + + return 0; +} + +static int +remoteDispatchNodeGetSecurityModel(struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_error *rerr, + void *args ATTRIBUTE_UNUSED, + remote_node_get_security_model_ret *ret) +{ + virSecurityModel secmodel; + + memset(&secmodel, 0, sizeof secmodel); + if (virNodeGetSecurityModel(conn, &secmodel) == -1) { + remoteDispatchFormatError(rerr, "%s", _("unable to get security model")); + return -1; + } + + ret->model.model_len = strlen(secmodel.model) + 1; + if (VIR_ALLOC_N(ret->model.model_val, ret->model.model_len) < 0) { + remoteDispatchOOMError(rerr); + return -1; + } + strcpy(ret->model.model_val, secmodel.model); + + ret->doi.doi_len = strlen(secmodel.doi) + 1; + if (VIR_ALLOC_N(ret->doi.doi_val, ret->doi.doi_len) < 0) { + remoteDispatchOOMError(rerr); + return -1; + } + strcpy(ret->doi.doi_val, secmodel.doi); + + return 0; +} + +static int remoteDispatchDomainGetOsType (struct qemud_server *server ATTRIBUTE_UNUSED, struct qemud_client *client ATTRIBUTE_UNUSED, virConnectPtr conn, diff --git a/qemud/remote_dispatch_args.h b/qemud/remote_dispatch_args.h index a19ab79..dce14f1 100644 --- a/qemud/remote_dispatch_args.h +++ b/qemud/remote_dispatch_args.h @@ -99,3 +99,4 @@ remote_node_device_get_parent_args val_remote_node_device_get_parent_args; remote_node_device_num_of_caps_args val_remote_node_device_num_of_caps_args; remote_node_device_list_caps_args val_remote_node_device_list_caps_args; + remote_domain_get_security_label_args val_remote_domain_get_security_label_args; diff --git a/qemud/remote_dispatch_prototypes.h b/qemud/remote_dispatch_prototypes.h index 3ffb164..bc8a899 100644 --- a/qemud/remote_dispatch_prototypes.h +++ b/qemud/remote_dispatch_prototypes.h @@ -184,6 +184,13 @@ static int remoteDispatchDomainGetSchedulerType( remote_error *err, remote_domain_get_scheduler_type_args *args, remote_domain_get_scheduler_type_ret *ret); +static int remoteDispatchDomainGetSecurityLabel( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_error *err, + remote_domain_get_security_label_args *args, + remote_domain_get_security_label_ret *ret); static int remoteDispatchDomainGetVcpus( struct qemud_server *server, struct qemud_client *client, @@ -576,6 +583,13 @@ static int remoteDispatchNodeGetInfo( remote_error *err, void *args, remote_node_get_info_ret *ret); +static int remoteDispatchNodeGetSecurityModel( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_error *err, + void *args, + remote_node_get_security_model_ret *ret); static int remoteDispatchNodeListDevices( struct qemud_server *server, struct qemud_client *client, diff --git a/qemud/remote_dispatch_ret.h b/qemud/remote_dispatch_ret.h index 563167f..136b1cc 100644 --- a/qemud/remote_dispatch_ret.h +++ b/qemud/remote_dispatch_ret.h @@ -86,3 +86,5 @@ remote_node_device_get_parent_ret val_remote_node_device_get_parent_ret; remote_node_device_num_of_caps_ret val_remote_node_device_num_of_caps_ret; remote_node_device_list_caps_ret val_remote_node_device_list_caps_ret; + remote_domain_get_security_label_ret val_remote_domain_get_security_label_ret; + remote_node_get_security_model_ret val_remote_node_get_security_model_ret; diff --git a/qemud/remote_dispatch_table.h b/qemud/remote_dispatch_table.h index 60f0e1c..6b7c9db 100644 --- a/qemud/remote_dispatch_table.h +++ b/qemud/remote_dispatch_table.h @@ -592,3 +592,13 @@ .args_filter = (xdrproc_t) xdr_remote_node_device_list_caps_args, .ret_filter = (xdrproc_t) xdr_remote_node_device_list_caps_ret, }, +{ /* DomainGetSecurityLabel => 118 */ + .fn = (dispatch_fn) remoteDispatchDomainGetSecurityLabel, + .args_filter = (xdrproc_t) xdr_remote_domain_get_security_label_args, + .ret_filter = (xdrproc_t) xdr_remote_domain_get_security_label_ret, +}, +{ /* NodeGetSecurityModel => 119 */ + .fn = (dispatch_fn) remoteDispatchNodeGetSecurityModel, + .args_filter = (xdrproc_t) xdr_void, + .ret_filter = (xdrproc_t) xdr_remote_node_get_security_model_ret, +}, diff --git a/qemud/remote_protocol.c b/qemud/remote_protocol.c index 249614a..a55a2dc 100644 --- a/qemud/remote_protocol.c +++ b/qemud/remote_protocol.c @@ -1166,6 +1166,43 @@ xdr_remote_domain_get_max_vcpus_ret (XDR *xdrs, remote_domain_get_max_vcpus_ret }

bool_t +xdr_remote_domain_get_security_label_args (XDR *xdrs, remote_domain_get_security_label_args *objp) +{ + + if (!xdr_remote_nonnull_domain (xdrs, &objp->dom)) + return FALSE; + return TRUE; +} + +bool_t +xdr_remote_domain_get_security_label_ret (XDR *xdrs, remote_domain_get_security_label_ret *objp) +{ + char **objp_cpp0 = (char **) (void *) &objp->label.label_val; + + if (!xdr_array (xdrs, objp_cpp0, (u_int *) &objp->label.label_len, REMOTE_SECURITY_LABEL_MAX, + sizeof (char), (xdrproc_t) xdr_char)) + return FALSE; + if (!xdr_int (xdrs, &objp->enforcing)) + return FALSE; + return TRUE; +} + +bool_t +xdr_remote_node_get_security_model_ret (XDR *xdrs, remote_node_get_security_model_ret *objp) +{ + char **objp_cpp1 = (char **) (void *) &objp->doi.doi_val; + char **objp_cpp0 = (char **) (void *) &objp->model.model_val; + + if (!xdr_array (xdrs, objp_cpp0, (u_int *) &objp->model.model_len, REMOTE_SECURITY_MODEL_MAX, + sizeof (char), (xdrproc_t) xdr_char)) + return FALSE; + if (!xdr_array (xdrs, objp_cpp1, (u_int *) &objp->doi.doi_len, REMOTE_SECURITY_DOI_MAX, + sizeof (char), (xdrproc_t) xdr_char)) + return FALSE; + return TRUE; +} + +bool_t xdr_remote_domain_attach_device_args (XDR *xdrs, remote_domain_attach_device_args *objp) {

diff --git a/qemud/remote_protocol.h b/qemud/remote_protocol.h index 912d8e3..e21972e 100644 --- a/qemud/remote_protocol.h +++ b/qemud/remote_protocol.h @@ -38,6 +38,9 @@ typedef remote_nonnull_string *remote_string; #define REMOTE_AUTH_TYPE_LIST_MAX 20 #define REMOTE_DOMAIN_BLOCK_PEEK_BUFFER_MAX 65536 #define REMOTE_DOMAIN_MEMORY_PEEK_BUFFER_MAX 65536 +#define REMOTE_SECURITY_MODEL_MAX VIR_SECURITY_MODEL_BUFLEN +#define REMOTE_SECURITY_LABEL_MAX VIR_SECURITY_LABEL_BUFLEN +#define REMOTE_SECURITY_DOI_MAX VIR_SECURITY_DOI_BUFLEN

typedef char remote_uuid[VIR_UUID_BUFLEN];

@@ -637,6 +640,32 @@ struct remote_domain_get_max_vcpus_ret { }; typedef struct remote_domain_get_max_vcpus_ret remote_domain_get_max_vcpus_ret;

+struct remote_domain_get_security_label_args { + remote_nonnull_domain dom; +}; +typedef struct remote_domain_get_security_label_args remote_domain_get_security_label_args; + +struct remote_domain_get_security_label_ret { + struct { + u_int label_len; + char *label_val; + } label; + int enforcing; +}; +typedef struct remote_domain_get_security_label_ret remote_domain_get_security_label_ret; + +struct remote_node_get_security_model_ret { + struct { + u_int model_len; + char *model_val; + } model; + struct { + u_int doi_len; + char *doi_val; + } doi; +}; +typedef struct remote_node_get_security_model_ret remote_node_get_security_model_ret; + struct remote_domain_attach_device_args { remote_nonnull_domain dom; remote_nonnull_string xml; @@ -1348,6 +1377,8 @@ enum remote_procedure { REMOTE_PROC_NODE_DEVICE_GET_PARENT = 115, REMOTE_PROC_NODE_DEVICE_NUM_OF_CAPS = 116, REMOTE_PROC_NODE_DEVICE_LIST_CAPS = 117, + REMOTE_PROC_DOMAIN_GET_SECURITY_LABEL = 118, + REMOTE_PROC_NODE_GET_SECURITY_MODEL = 119, }; typedef enum remote_procedure remote_procedure;

@@ -1474,6 +1505,9 @@ extern bool_t xdr_remote_domain_get_vcpus_args (XDR *, remote_domain_get_vcpus_ extern bool_t xdr_remote_domain_get_vcpus_ret (XDR *, remote_domain_get_vcpus_ret*); extern bool_t xdr_remote_domain_get_max_vcpus_args (XDR *, remote_domain_get_max_vcpus_args*); extern bool_t xdr_remote_domain_get_max_vcpus_ret (XDR *, remote_domain_get_max_vcpus_ret*); +extern bool_t xdr_remote_domain_get_security_label_args (XDR *, remote_domain_get_security_label_args*); +extern bool_t xdr_remote_domain_get_security_label_ret (XDR *, remote_domain_get_security_label_ret*); +extern bool_t xdr_remote_node_get_security_model_ret (XDR *, remote_node_get_security_model_ret*); extern bool_t xdr_remote_domain_attach_device_args (XDR *, remote_domain_attach_device_args*); extern bool_t xdr_remote_domain_detach_device_args (XDR *, remote_domain_detach_device_args*); extern bool_t xdr_remote_domain_get_autostart_args (XDR *, remote_domain_get_autostart_args*); @@ -1679,6 +1713,9 @@ extern bool_t xdr_remote_domain_get_vcpus_args (); extern bool_t xdr_remote_domain_get_vcpus_ret (); extern bool_t xdr_remote_domain_get_max_vcpus_args (); extern bool_t xdr_remote_domain_get_max_vcpus_ret (); +extern bool_t xdr_remote_domain_get_security_label_args (); +extern bool_t xdr_remote_domain_get_security_label_ret (); +extern bool_t xdr_remote_node_get_security_model_ret (); extern bool_t xdr_remote_domain_attach_device_args (); extern bool_t xdr_remote_domain_detach_device_args (); extern bool_t xdr_remote_domain_get_autostart_args (); diff --git a/qemud/remote_protocol.x b/qemud/remote_protocol.x index 2a6035b..ec5cbbb 100644 --- a/qemud/remote_protocol.x +++ b/qemud/remote_protocol.x @@ -115,6 +115,21 @@ const REMOTE_DOMAIN_BLOCK_PEEK_BUFFER_MAX = 65536; */ const REMOTE_DOMAIN_MEMORY_PEEK_BUFFER_MAX = 65536;

+/* + * Maximum length of a security model field. + */ +const REMOTE_SECURITY_MODEL_MAX = VIR_SECURITY_MODEL_BUFLEN; + +/* + * Maximum length of a security label field. + */ +const REMOTE_SECURITY_LABEL_MAX = VIR_SECURITY_LABEL_BUFLEN; + +/* + * Maximum length of a security DOI field. + */ +const REMOTE_SECURITY_DOI_MAX = VIR_SECURITY_DOI_BUFLEN; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN];

@@ -617,6 +632,20 @@ struct remote_domain_get_max_vcpus_ret { int num; };

+struct remote_domain_get_security_label_args { + remote_nonnull_domain dom; +}; + +struct remote_domain_get_security_label_ret { + char label<REMOTE_SECURITY_LABEL_MAX>; + int enforcing; +}; + +struct remote_node_get_security_model_ret { + char model<REMOTE_SECURITY_MODEL_MAX>; + char doi<REMOTE_SECURITY_DOI_MAX>; +}; + struct remote_domain_attach_device_args { remote_nonnull_domain dom; remote_nonnull_string xml; @@ -1223,7 +1252,10 @@ enum remote_procedure { REMOTE_PROC_NODE_DEVICE_DUMP_XML = 114, REMOTE_PROC_NODE_DEVICE_GET_PARENT = 115, REMOTE_PROC_NODE_DEVICE_NUM_OF_CAPS = 116, - REMOTE_PROC_NODE_DEVICE_LIST_CAPS = 117 + REMOTE_PROC_NODE_DEVICE_LIST_CAPS = 117, + + REMOTE_PROC_DOMAIN_GET_SECURITY_LABEL = 118, + REMOTE_PROC_NODE_GET_SECURITY_MODEL = 119 };

/* Custom RPC structure. */ diff --git a/src/Makefile.am b/src/Makefile.am index 3a798d2..6f98971 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -139,7 +139,7 @@ UML_DRIVER_SOURCES = \ NETWORK_DRIVER_SOURCES = \ network_driver.h network_driver.c

-# And finally storage backend specific impls +# Storage backend specific impls STORAGE_DRIVER_SOURCES = \ storage_driver.h storage_driver.c \ storage_backend.h storage_backend.c @@ -164,6 +164,12 @@ STORAGE_DRIVER_DISK_SOURCES = \ STORAGE_HELPER_DISK_SOURCES = \ parthelper.c

+# Security framework and drivers for various models +SECURITY_DRIVER_SOURCES = \ + security.h security.c + +SECURITY_DRIVER_SELINUX_SOURCES = \ + security_selinux.h security_selinux.c

NODE_DEVICE_DRIVER_SOURCES = \ node_device.c node_device.h @@ -377,6 +383,19 @@ libvirt_driver_nodedev_la_LDFLAGS += -module -avoid-version endif endif

+libvirt_driver_security_la_SOURCES = $(SECURITY_DRIVER_SOURCES) +if WITH_DRIVER_MODULES +mod_LTLIBRARIES += libvirt_driver_security.la +else +noinst_LTLIBRARIES += libvirt_driver_security.la +endif +if WITH_DRIVER_MODULES +libvirt_driver_security_la_LDFLAGS = -module -avoid-version +endif + +if HAVE_SELINUX +libvirt_driver_security_la_SOURCES += $(SECURITY_DRIVER_SELINUX_SOURCES) +endif

# Add all conditional sources just in case... EXTRA_DIST += \ @@ -395,7 +414,9 @@ EXTRA_DIST += \ $(STORAGE_DRIVER_DISK_SOURCES) \ $(NODE_DEVICE_DRIVER_SOURCES) \ $(NODE_DEVICE_DRIVER_HAL_SOURCES) \ - $(NODE_DEVICE_DRIVER_DEVKIT_SOURCES) + $(NODE_DEVICE_DRIVER_DEVKIT_SOURCES) \ + $(SECURITY_DRIVER_SOURCES) \ + $(SECURITY_DRIVER_SELINUX_SOURCES)

# # Build our version script. This is composed of three parts: diff --git a/src/capabilities.c b/src/capabilities.c index 3a23332..fb21d87 100644 --- a/src/capabilities.c +++ b/src/capabilities.c @@ -150,6 +150,8 @@ virCapabilitiesFree(virCapsPtr caps) { VIR_FREE(caps->host.migrateTrans);

VIR_FREE(caps->host.arch); + VIR_FREE(caps->host.secModel.model); + VIR_FREE(caps->host.secModel.doi); VIR_FREE(caps); }

@@ -599,6 +601,14 @@ virCapabilitiesFormatXML(virCapsPtr caps) virBufferAddLit(&xml, " </cells>\n"); virBufferAddLit(&xml, " </topology>\n"); } + + if (caps->host.secModel.model) { + virBufferAddLit(&xml, " <secmodel>\n"); + virBufferVSprintf(&xml, " <model>%s</model>\n", caps->host.secModel.model); + virBufferVSprintf(&xml, " <doi>%s</doi>\n", caps->host.secModel.doi); + virBufferAddLit(&xml, " </secmodel>\n"); + } + virBufferAddLit(&xml, " </host>\n\n");

diff --git a/src/capabilities.h b/src/capabilities.h index be3296c..db32291 100644 --- a/src/capabilities.h +++ b/src/capabilities.h @@ -78,6 +78,12 @@ struct _virCapsHostNUMACell { int *cpus; };

+typedef struct _virCapsHostSecModel virCapsHostSecModel; +struct _virCapsHostSecModel { + char *model; + char *doi; +}; + typedef struct _virCapsHost virCapsHost; typedef virCapsHost *virCapsHostPtr; struct _virCapsHost { @@ -90,6 +96,7 @@ struct _virCapsHost { char **migrateTrans; int nnumaCell; virCapsHostNUMACellPtr *numaCell; + virCapsHostSecModel secModel; };

typedef struct _virCaps virCaps; diff --git a/src/domain_conf.c b/src/domain_conf.c index 622665c..4d1a951 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -379,6 +379,16 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) VIR_FREE(def); }

+void virSecurityLabelDefFree(virDomainDefPtr def); + +void virSecurityLabelDefFree(virDomainDefPtr def) +{ + if (def->seclabel.model) + VIR_FREE(def->seclabel.model); + if (def->seclabel.label) + VIR_FREE(def->seclabel.label); +} + void virDomainDefFree(virDomainDefPtr def) { unsigned int i; @@ -437,6 +447,8 @@ void virDomainDefFree(virDomainDefPtr def) VIR_FREE(def->cpumask); VIR_FREE(def->emulator);

+ virSecurityLabelDefFree(def); + VIR_FREE(def); }

@@ -1818,6 +1830,34 @@ static int virDomainLifecycleParseXML(virConnectPtr conn, return 0; }

+static int +virSecurityLabelDefParseXML(virConnectPtr conn, + const virDomainDefPtr def, + xmlXPathContextPtr ctxt) +{ + char *p; + + if (virXPathNode(conn, "./seclabel", ctxt) == NULL) + return 0; + + p = virXPathStringLimit(conn, "string(./seclabel/label[1])", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + if (p == NULL) + goto error; + def->seclabel.label = p; + + p = virXPathStringLimit(conn, "string(./seclabel/@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + if (p == NULL) + goto error; + def->seclabel.model = p; + + return 0; + +error: + virSecurityLabelDefFree(def); + return -1; +}

virDomainDeviceDefPtr virDomainDeviceDefParse(virConnectPtr conn, virCapsPtr caps, @@ -2403,6 +2443,10 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, } VIR_FREE(nodes);

+ /* analysis of security label */ + if (virSecurityLabelDefParseXML(conn, def, ctxt) == -1) + goto error; + return def;

no_memory: @@ -3419,6 +3463,13 @@ char *virDomainDefFormat(virConnectPtr conn, goto cleanup;

virBufferAddLit(&buf, " </devices>\n"); + + if (def->seclabel.model) { + virBufferEscapeString(&buf, " <seclabel model='%s'>\n", def->seclabel.model); + virBufferEscapeString(&buf, " <label>%s</label>\n", def->seclabel.label); + virBufferAddLit(&buf, " </seclabel>\n"); + } + virBufferAddLit(&buf, "</domain>\n");

if (virBufferError(&buf)) diff --git a/src/domain_conf.h b/src/domain_conf.h index b6f6b43..553b437 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -407,6 +407,14 @@ struct _virDomainOSDef { char *bootloaderArgs; };

+/* Security configuration for domain */ +typedef struct _virSecurityLabelDef virSecurityLabelDef; +typedef virSecurityLabelDef *virSecurityLabelDefPtr; +struct _virSecurityLabelDef { + char *model; /* name of security model */ + char *label; /* security label string */ +}; + #define VIR_DOMAIN_CPUMASK_LEN 1024

/* Guest VM main configuration */ @@ -464,6 +472,7 @@ struct _virDomainDef {

/* Only 1 */ virDomainChrDefPtr console; + virSecurityLabelDef seclabel; };

/* Guest VM runtime state */ diff --git a/src/driver.h b/src/driver.h index 32d4257..005ea4c 100644 --- a/src/driver.h +++ b/src/driver.h @@ -181,6 +181,12 @@ typedef int typedef int (*virDrvDomainGetMaxVcpus) (virDomainPtr domain); typedef int + (*virDrvDomainGetSecurityLabel) (virDomainPtr domain, + virSecurityLabelPtr seclabel); +typedef int + (*virDrvNodeGetSecurityModel) (virConnectPtr conn, + virSecurityModelPtr secmodel); +typedef int (*virDrvDomainAttachDevice) (virDomainPtr domain, const char *xml); typedef int @@ -361,6 +367,8 @@ struct _virDriver { virDrvDomainPinVcpu domainPinVcpu; virDrvDomainGetVcpus domainGetVcpus; virDrvDomainGetMaxVcpus domainGetMaxVcpus; + virDrvDomainGetSecurityLabel domainGetSecurityLabel; + virDrvNodeGetSecurityModel nodeGetSecurityModel; virDrvDomainDumpXML domainDumpXML; virDrvListDefinedDomains listDefinedDomains; virDrvNumOfDefinedDomains numOfDefinedDomains; diff --git a/src/libvirt.c b/src/libvirt.c index 038a1ac..47dac90 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -4174,6 +4174,70 @@ error: return -1; }

+/** + * virDomainGetSecurityLabel: + * @domain: a domain object + * @seclabel: pointer to a virSecurityLabel structure + * + * Extract security label of an active domain. + * + * Returns 0 in case of success, -1 in case of failure, and -2 + * if the operation is not supported (caller decides if that's + * an error). + */ +int +virDomainGetSecurityLabel(virDomainPtr domain, virSecurityLabelPtr seclabel) +{ + virConnectPtr conn; + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + return -1; + } + + if (seclabel == NULL) { + virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__); + return -1; + } + + conn = domain->conn; + + if (conn->driver->domainGetSecurityLabel) + return conn->driver->domainGetSecurityLabel(domain, seclabel); + + virLibConnWarning(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + return -2; +} + +/** + * virNodeGetSecurityModel: + * @conn: a connection object + * @secmodel: pointer to a virSecurityModel structure + * + * Extract the security model of a hypervisor. + * + * Returns 0 in case of success, -1 in case of failure, and -2 if the + * operation is not supported (caller decides if that's an error). + */ +int +virNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr secmodel) +{ + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__); + return -1; + } + + if (secmodel == NULL) { + virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__); + return -1; + } + + if (conn->driver->nodeGetSecurityModel) + return conn->driver->nodeGetSecurityModel(conn, secmodel); + + virLibConnWarning(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + return -2; +}

/** * virDomainAttachDevice: diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9e9b3e5..6649f69 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -337,3 +337,4 @@ virXPathNode; virXPathNodeSet; virXPathString; virXMLPropString; +virXPathStringLimit; diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 1422e4c..0777d46 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -244,7 +244,8 @@ LIBVIRT_0.6.0 { virStoragePoolRef; virStorageVolRef; virNodeDeviceRef; - + virDomainGetSecurityLabel; + virNodeGetSecurityModel; } LIBVIRT_0.5.0;

LIBVIRT_0.6.1 { diff --git a/src/lxc_driver.c b/src/lxc_driver.c index aa417a9..69fe95f 100644 --- a/src/lxc_driver.c +++ b/src/lxc_driver.c @@ -1429,6 +1429,8 @@ static virDriver lxcDriver = { NULL, /* domainPinVcpu */ NULL, /* domainGetVcpus */ NULL, /* domainGetMaxVcpus */ + NULL, /* domainGetSecurityLabel */ + NULL, /* nodeGetSecurityModel */ lxcDomainDumpXML, /* domainDumpXML */ lxcListDefinedDomains, /* listDefinedDomains */ lxcNumDefinedDomains, /* numOfDefinedDomains */ diff --git a/src/openvz_driver.c b/src/openvz_driver.c index e004819..4a4be4c 100644 --- a/src/openvz_driver.c +++ b/src/openvz_driver.c @@ -1299,6 +1299,8 @@ static virDriver openvzDriver = { NULL, /* domainPinVcpu */ NULL, /* domainGetVcpus */ openvzDomainGetMaxVcpus, /* domainGetMaxVcpus */ + NULL, /* domainGetSecurityLabel */ + NULL, /* nodeGetSecurityModel */ openvzDomainDumpXML, /* domainDumpXML */ openvzListDefinedDomains, /* listDomains */ openvzNumDefinedDomains, /* numOfDomains */ diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 8f0193d..f2079e2 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -33,6 +33,7 @@ #include "domain_conf.h" #include "domain_event.h" #include "threads.h" +#include "security.h"

#define qemudDebug(fmt, ...) do {} while(0)

@@ -83,6 +84,8 @@ struct qemud_driver { virDomainEventQueuePtr domainEventQueue; int domainEventTimer; int domainEventDispatching; + + virSecurityDriverPtr securityDriver; };

/* Status needed to reconenct to running VMs */ diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 8bb948a..3e635aa 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -68,6 +68,7 @@ #include "memory.h" #include "uuid.h" #include "domain_conf.h" +#include "security.h"

#define VIR_FROM_THIS VIR_FROM_QEMU

@@ -351,6 +352,50 @@ next: return 0; }

+static int +qemudSecurityInit(struct qemud_driver *qemud_drv) +{ + int ret; + const char *doi, *model; + virCapsPtr caps; + virSecurityDriverPtr security_drv; + + ret = virSecurityDriverStartup(&security_drv); + if (ret == -1) { + qemudLog(QEMUD_ERR, _("Failed to start security driver")); + return -1; + } + /* No security driver wanted to be enabled: just return */ + if (ret == -2) + return 0; + + qemud_drv->securityDriver = security_drv; + doi = virSecurityDriverGetDOI(security_drv); + model = virSecurityDriverGetModel(security_drv); + + qemudLog(QEMUD_DEBUG, "Initialized security driver \"%s\" with " + "DOI \"%s\".\n", model, doi); + + /* + * Add security policy host caps now that the security driver is + * initialized. + */ + caps = qemud_drv->caps; + + caps->host.secModel.model = strdup(model); + if (!caps->host.secModel.model) { + qemudLog(QEMUD_ERR, _("Failed to copy secModel model: %s"), strerror(errno)); + return -1; + } + + caps->host.secModel.doi = strdup(doi); + if (!caps->host.secModel.doi) { + qemudLog(QEMUD_ERR, _("Failed to copy secModel DOI: %s"), strerror(errno)); + return -1; + } + + return 0; +}

/** * qemudStartup: @@ -443,6 +488,11 @@ qemudStartup(void) { if ((qemu_driver->caps = qemudCapsInit()) == NULL) goto out_of_memory;

+ if (qemudSecurityInit(qemu_driver) < 0) { + qemudShutdown(); + return -1; + } + if (qemudLoadDriverConfig(qemu_driver, driverConf) < 0) { goto error; } @@ -1130,6 +1180,15 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) { return -1; }

+static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm) +{ + if (vm->def->seclabel.label != NULL) + if (driver->securityDriver && driver->securityDriver->domainSetSecurityLabel) + return driver->securityDriver->domainSetSecurityLabel(conn, driver->securityDriver, + &vm->def->seclabel); + return 0; +} + static virDomainPtr qemudDomainLookupByName(virConnectPtr conn, const char *name);

@@ -1198,6 +1257,16 @@ static int qemudStartVMDaemon(virConnectPtr conn, return -1; }

+ /* + * Set up the security label for the domain here, before doing + * too much else. + */ + if (qemudDomainSetSecurityLabel(conn, driver, vm) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to set security label")); + return -1; + } + if (qemudExtractVersionInfo(emulator, NULL, &qemuCmdFlags) < 0) { @@ -2755,7 +2824,94 @@ cleanup: return ret; }

+static int qemudDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr seclabel) +{ + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + virDomainObjPtr vm; + const char *type; + int ret = -1; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(dom->uuid, uuidstr); + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (!(type = virDomainVirtTypeToString(vm->def->virtType))) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, + _("unknown virt type in domain definition '%d'"), + vm->def->virtType); + goto cleanup; + } + + /* + * Theoretically, the pid can be replaced during this operation and + * return the label of a different process. If atomicity is needed, + * further validation will be required. + * + * Comment from Dan Berrange: + * + * Well the PID as stored in the virDomainObjPtr can't be changed + * because you've got a locked object. The OS level PID could have + * exited, though and in extreme circumstances have cycled through all + * PIDs back to ours. We could sanity check that our PID still exists + * after reading the label, by checking that our FD connecting to the + * QEMU monitor hasn't seen SIGHUP/ERR on poll(). + */ + if (virDomainIsActive(vm)) { + if (driver->securityDriver && driver->securityDriver->domainGetSecurityLabel) { + if (driver->securityDriver->domainGetSecurityLabel(dom->conn, vm, seclabel) == -1) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to get security label")); + goto cleanup; + } + } + } + + ret = 0; + +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +} + +static int qemudNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr secmodel) +{ + struct qemud_driver *driver = (struct qemud_driver *)conn->privateData; + char *p; + + if (!driver->securityDriver) + return -2; + + p = driver->caps->host.secModel.model; + if (strlen(p) >= VIR_SECURITY_MODEL_BUFLEN-1) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("security model string exceeds max %d bytes"), + VIR_SECURITY_MODEL_BUFLEN-1); + return -1; + } + strcpy(secmodel->model, p); + + p = driver->caps->host.secModel.doi; + if (strlen(p) >= VIR_SECURITY_DOI_BUFLEN-1) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("security DOI string exceeds max %d bytes"), + VIR_SECURITY_DOI_BUFLEN-1); + return -1; + } + strcpy(secmodel->doi, p); + return 0; +}

+/* TODO: check seclabel restore */ static int qemudDomainRestore(virConnectPtr conn, const char *path) { struct qemud_driver *driver = conn->privateData; @@ -4510,6 +4666,8 @@ static virDriver qemuDriver = { NULL, /* domainGetVcpus */ #endif qemudDomainGetMaxVcpus, /* domainGetMaxVcpus */ + qemudDomainGetSecurityLabel, /* domainGetSecurityLabel */ + qemudNodeGetSecurityModel, /* nodeGetSecurityModel */ qemudDomainDumpXML, /* domainDumpXML */ qemudListDefinedDomains, /* listDomains */ qemudNumDefinedDomains, /* numOfDomains */ diff --git a/src/remote_internal.c b/src/remote_internal.c index eda6177..8948e67 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -2295,6 +2295,67 @@ done: return rv; }

+static int +remoteDomainGetSecurityLabel (virDomainPtr domain, virSecurityLabelPtr seclabel) +{ + remote_domain_get_security_label_args args; + remote_domain_get_security_label_ret ret; + struct private_data *priv = domain->conn->privateData; + + make_nonnull_domain (&args.dom, domain); + memset (&ret, 0, sizeof ret); + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_SECURITY_LABEL, + (xdrproc_t) xdr_remote_domain_get_security_label_args, (char *)&args, + (xdrproc_t) xdr_remote_domain_get_security_label_ret, (char *)&ret) == -1) { + return -1; + } + + if (ret.label.label_val != NULL) { + if (strlen (ret.label.label_val) >= sizeof seclabel->label) { + errorf (domain->conn, VIR_ERR_RPC, _("security label exceeds maximum: %zd"), + sizeof seclabel->label - 1); + return -1; + } + strcpy (seclabel->label, ret.label.label_val); + seclabel->enforcing = ret.enforcing; + } + + return 0; +} + +static int +remoteNodeGetSecurityModel (virConnectPtr conn, virSecurityModelPtr secmodel) +{ + remote_node_get_security_model_ret ret; + struct private_data *priv = conn->privateData; + + memset (&ret, 0, sizeof ret); + if (call (conn, priv, 0, REMOTE_PROC_NODE_GET_SECURITY_MODEL, + (xdrproc_t) xdr_void, NULL, + (xdrproc_t) xdr_remote_node_get_security_model_ret, (char *)&ret) == -1) { + return -1; + } + + if (ret.model.model_val != NULL) { + if (strlen (ret.model.model_val) >= sizeof secmodel->model) { + errorf (conn, VIR_ERR_RPC, _("security model exceeds maximum: %zd"), + sizeof secmodel->model - 1); + return -1; + } + strcpy (secmodel->model, ret.model.model_val); + } + + if (ret.doi.doi_val != NULL) { + if (strlen (ret.doi.doi_val) >= sizeof secmodel->doi) { + errorf (conn, VIR_ERR_RPC, _("security doi exceeds maximum: %zd"), + sizeof secmodel->doi - 1); + return -1; + } + strcpy (secmodel->doi, ret.doi.doi_val); + } + return 0; +} + static char * remoteDomainDumpXML (virDomainPtr domain, int flags) { @@ -6715,6 +6776,8 @@ static virDriver driver = { .domainPinVcpu = remoteDomainPinVcpu, .domainGetVcpus = remoteDomainGetVcpus, .domainGetMaxVcpus = remoteDomainGetMaxVcpus, + .domainGetSecurityLabel = remoteDomainGetSecurityLabel, + .nodeGetSecurityModel = remoteNodeGetSecurityModel, .domainDumpXML = remoteDomainDumpXML, .listDefinedDomains = remoteListDefinedDomains, .numOfDefinedDomains = remoteNumOfDefinedDomains, diff --git a/src/security.c b/src/security.c new file mode 100644 index 0000000..8dd2c9f --- /dev/null +++ b/src/security.c @@ -0,0 +1,133 @@ +/* + * Copyright (C) 2008 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * Authors: + * James Morris <jmorris@namei.org> + * + */ +#include <config.h> +#include <string.h> + +#include "virterror_internal.h" + +#include "security.h" + +#if HAVE_SELINUX +#include "security_selinux.h" +#endif + +static virSecurityDriverStatus testSecurityDriverProbe(void) +{ + return SECURITY_DRIVER_DISABLE; +} + +virSecurityDriver virTestSecurityDriver = { + .name = "test", + .probe = testSecurityDriverProbe, +}; + +static virSecurityDriverPtr security_drivers[] = { + &virTestSecurityDriver, +#ifdef HAVE_SELINUX + &virSELinuxSecurityDriver, +#endif +}; + +/* + * Probe each security driver: each should perform a test to see if it + * should be loaded, e.g. if the currently active host security mechanism + * matches. If the probe succeeds, initialize the driver and return it. + * + * Returns 0 on success, and -1 on error. If no security driver wanted to + * be enabled, then return -2 and let the caller determine what this really + * means. + */ +int +virSecurityDriverStartup(virSecurityDriverPtr * drv) +{ + unsigned int i; + + for (i = 0; i < (sizeof(security_drivers) / sizeof(security_drivers[0])); i++) { + virSecurityDriverPtr tmp = security_drivers[i]; + virSecurityDriverStatus ret = tmp->probe(); + + switch (ret) { + case SECURITY_DRIVER_ENABLE: + virSecurityDriverInit(tmp); + if (tmp->open(NULL, tmp) == -1) { + return -1; + } else { + *drv = tmp; + return 0; + } + break; + + case SECURITY_DRIVER_DISABLE: + break; + + default: + return -1; + } + } + return -2; +} + +void +virSecurityReportError(virConnectPtr conn, int code, const char *fmt, ...) +{ + va_list args; + char errorMessage[1024]; + + if (fmt) { + va_start(args, fmt); + vsnprintf(errorMessage, sizeof(errorMessage) - 1, fmt, args); + va_end(args); + } else + errorMessage[0] = '\0'; + + virRaiseError(conn, NULL, NULL, VIR_FROM_SECURITY, code, + VIR_ERR_ERROR, NULL, NULL, NULL, -1, -1, "%s", + errorMessage); +} + +/* + * Helpers + */ +void +virSecurityDriverInit(virSecurityDriverPtr drv) +{ + memset(&drv->_private, 0, sizeof drv->_private); +} + +int +virSecurityDriverSetDOI(virConnectPtr conn, + virSecurityDriverPtr drv, + const char *doi) +{ + if (strlen(doi) >= VIR_SECURITY_DOI_BUFLEN) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("%s: DOI \'%s\' is " + "longer than the maximum allowed length of %d"), + __func__, doi, VIR_SECURITY_DOI_BUFLEN - 1); + return -1; + } + strcpy(drv->_private.doi, doi); + return 0; +} + +const char * +virSecurityDriverGetDOI(virSecurityDriverPtr drv) +{ + return drv->_private.doi; +} + +const char * +virSecurityDriverGetModel(virSecurityDriverPtr drv) +{ + return drv->name; +} diff --git a/src/security.h b/src/security.h new file mode 100644 index 0000000..2ea9013 --- /dev/null +++ b/src/security.h @@ -0,0 +1,72 @@ +/* + * Copyright (C) 2008 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * Authors: + * James Morris <jmorris@namei.org> + * + */ +#ifndef __VIR_SECURITY_H__ +#define __VIR_SECURITY_H__ + +#include "internal.h" +#include "domain_conf.h" + +/* + * Return values for security driver probing: the driver will determine + * whether it should be enabled or disabled. + */ +typedef enum { + SECURITY_DRIVER_ENABLE = 0, + SECURITY_DRIVER_ERROR = -1, + SECURITY_DRIVER_DISABLE = -2, +} virSecurityDriverStatus; + +typedef struct _virSecurityDriver virSecurityDriver; +typedef virSecurityDriver *virSecurityDriverPtr; +typedef virSecurityDriverStatus (*virSecurityDriverProbe) (void); +typedef int (*virSecurityDriverOpen) (virConnectPtr conn, + virSecurityDriverPtr drv); +typedef int (*virSecurityDomainGetLabel) (virConnectPtr conn, + virDomainObjPtr vm, + virSecurityLabelPtr sec); +typedef int (*virSecurityDomainSetLabel) (virConnectPtr conn, + virSecurityDriverPtr drv, + virSecurityLabelDefPtr secdef); + +struct _virSecurityDriver { + const char *name; + virSecurityDriverProbe probe; + virSecurityDriverOpen open; + virSecurityDomainGetLabel domainGetSecurityLabel; + virSecurityDomainSetLabel domainSetSecurityLabel; + + /* + * This is internally managed driver state and should only be accessed + * via helpers below. + */ + struct { + char doi[VIR_SECURITY_DOI_BUFLEN]; + } _private; +}; + +/* Global methods */ +int virSecurityDriverStartup(virSecurityDriverPtr * drv); + +void +virSecurityReportError(virConnectPtr conn, int code, const char *fmt, ...) + ATTRIBUTE_FORMAT(printf, 3, 4); + +/* Helpers */ +void virSecurityDriverInit(virSecurityDriverPtr drv); +int virSecurityDriverSetDOI(virConnectPtr conn, + virSecurityDriverPtr drv, + const char *doi); +const char *virSecurityDriverGetDOI(virSecurityDriverPtr drv); +const char *virSecurityDriverGetModel(virSecurityDriverPtr drv); + +#endif /* __VIR_SECURITY_H__ */ diff --git a/src/security_selinux.c b/src/security_selinux.c new file mode 100644 index 0000000..65fcde7 --- /dev/null +++ b/src/security_selinux.c @@ -0,0 +1,108 @@ +/* + * Copyright (C) 2008 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * Authors: + * James Morris <jmorris@namei.org> + * + * SELinux security driver. + */ +#include <config.h> +#include <selinux/selinux.h> + +#include "security.h" +#include "security_selinux.h" + +#define SECURITY_SELINUX_VOID_DOI "0" + +static int +SELinuxSecurityDriverProbe(void) +{ + return is_selinux_enabled() ? SECURITY_DRIVER_ENABLE : SECURITY_DRIVER_DISABLE; +} + +static int +SELinuxSecurityDriverOpen(virConnectPtr conn, virSecurityDriverPtr drv) +{ + /* + * Where will the DOI come from? SELinux configuration, or qemu + * configuration? For the moment, we'll just set it to "0". + */ + virSecurityDriverSetDOI(conn, drv, SECURITY_SELINUX_VOID_DOI); + + return 0; +} + +static int +SELinuxSecurityDomainGetSecurityLabel(virConnectPtr conn, + virDomainObjPtr vm, + virSecurityLabelPtr sec) +{ + security_context_t ctx; + + if (getpidcon(vm->pid, &ctx) == -1) { + virSecurityReportError(conn, VIR_ERR_ERROR, _("%s: error calling " + "getpidcon(): %s"), __func__, + strerror(errno)); + return -1; + } + + if (strlen((char *) ctx) >= VIR_SECURITY_LABEL_BUFLEN) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("%s: security label exceeds " + "maximum length: %d"), __func__, + VIR_SECURITY_LABEL_BUFLEN - 1); + return -1; + } + + strcpy(sec->label, (char *) ctx); + free(ctx); + + sec->enforcing = security_getenforce(); + if (sec->enforcing == -1) { + virSecurityReportError(conn, VIR_ERR_ERROR, _("%s: error calling " + "security_getenforce(): %s"), __func__, + strerror(errno)); + return -1; + } + + return 0; +} + +static int +SELinuxSecurityDomainSetSecurityLabel(virConnectPtr conn, + virSecurityDriverPtr drv, + const virSecurityLabelDefPtr secdef) +{ + /* TODO: verify DOI */ + + if (!STREQ(drv->name, secdef->model)) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("%s: security label driver mismatch: " + "\'%s\' model configured for domain, but " + "hypervisor driver is \'%s\'."), + __func__, secdef->model, drv->name); + return -1; + } + + if (setexeccon(secdef->label) == -1) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("%s: unable to set security context " + "'\%s\': %s."), __func__, secdef->label, + strerror(errno)); + return -1; + } + return 0; +} + +virSecurityDriver virSELinuxSecurityDriver = { + .name = "selinux", + .probe = SELinuxSecurityDriverProbe, + .open = SELinuxSecurityDriverOpen, + .domainGetSecurityLabel = SELinuxSecurityDomainGetSecurityLabel, + .domainSetSecurityLabel = SELinuxSecurityDomainSetSecurityLabel, +}; diff --git a/src/security_selinux.h b/src/security_selinux.h new file mode 100644 index 0000000..1e32209 --- /dev/null +++ b/src/security_selinux.h @@ -0,0 +1,18 @@ +/* + * Copyright (C) 2008 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * Authors: + * James Morris <jmorris@namei.org> + * + */ +#ifndef __VIR_SECURITY_SELINUX_H__ +#define __VIR_SECURITY_SELINUX_H__ + +extern virSecurityDriver virSELinuxSecurityDriver; + +#endif /* __VIR_SECURITY_SELINUX_H__ */ diff --git a/src/storage_backend.c b/src/storage_backend.c index 8408f34..787630c 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -276,6 +276,7 @@ virStorageBackendUpdateVolTargetInfoFD(virConnectPtr conn, VIR_FREE(target->perms.label);

#if HAVE_SELINUX + /* XXX: make this a security driver call */ if (fgetfilecon(fd, &filecon) == -1) { if (errno != ENODATA && errno != ENOTSUP) { virReportSystemError(conn, errno, diff --git a/src/test.c b/src/test.c index 226fe2e..b3e6477 100644 --- a/src/test.c +++ b/src/test.c @@ -3501,6 +3501,8 @@ static virDriver testDriver = { NULL, /* domainPinVcpu */ NULL, /* domainGetVcpus */ NULL, /* domainGetMaxVcpus */ + NULL, /* domainGetSecurityLabel */ + NULL, /* nodeGetSecurityModel */ testDomainDumpXML, /* domainDumpXML */ testListDefinedDomains, /* listDefinedDomains */ testNumOfDefinedDomains, /* numOfDefinedDomains */ diff --git a/src/uml_driver.c b/src/uml_driver.c index c5a06a2..119765b 100644 --- a/src/uml_driver.c +++ b/src/uml_driver.c @@ -1854,6 +1854,8 @@ static virDriver umlDriver = { NULL, /* domainPinVcpu */ NULL, /* domainGetVcpus */ NULL, /* domainGetMaxVcpus */ + NULL, /* domainGetSecurityLabel */ + NULL, /* nodeGetSecurityModel */ umlDomainDumpXML, /* domainDumpXML */ umlListDefinedDomains, /* listDomains */ umlNumDefinedDomains, /* numOfDomains */ diff --git a/src/virsh.c b/src/virsh.c index 298dde0..ce39000 100644 --- a/src/virsh.c +++ b/src/virsh.c @@ -978,6 +978,7 @@ static const vshCmdOptDef opts_undefine[] = { {NULL, 0, 0, NULL} };

+/* XXX MAC policy for defining & undefining domains ?? */ static int cmdUndefine(vshControl *ctl, const vshCmd *cmd) { @@ -1539,6 +1540,8 @@ cmdDominfo(vshControl *ctl, const vshCmd *cmd) { virDomainInfo info; virDomainPtr dom; + virSecurityModel secmodel; + virSecurityLabel seclabel; int ret = TRUE, autostart; unsigned int id; char *str, uuid[VIR_UUID_STRING_BUFLEN]; @@ -1597,6 +1600,29 @@ cmdDominfo(vshControl *ctl, const vshCmd *cmd) autostart ? _("enable") : _("disable") ); }

+ /* Security model and label information */ + memset(&secmodel, 0, sizeof secmodel); + if (virNodeGetSecurityModel(ctl->conn, &secmodel) == -1) { + virDomainFree(dom); + return FALSE; + } else { + /* Only print something if a security model is active */ + if (secmodel.model[0] != '\0') { + vshPrint(ctl, "%-15s %s\n", _("Security model:"), secmodel.model); + vshPrint(ctl, "%-15s %s\n", _("Security DOI:"), secmodel.doi); + + /* Security labels are only valid for active domains */ + memset(&seclabel, 0, sizeof seclabel); + if (virDomainGetSecurityLabel(dom, &seclabel) == -1) { + virDomainFree(dom); + return FALSE; + } else { + if (seclabel.label[0] != '\0') + vshPrint(ctl, "%-15s %s (%s)\n", _("Security label:"), + seclabel.label, seclabel.enforcing ? "enforcing" : "permissive"); + } + } + } virDomainFree(dom); return ret; } diff --git a/src/virterror.c b/src/virterror.c index 42a7cf5..8d7dc93 100644 --- a/src/virterror.c +++ b/src/virterror.c @@ -151,6 +151,9 @@ static const char *virErrorDomainName(virErrorDomain domain) { case VIR_FROM_UML: dom = "UML "; break; + case VIR_FROM_SECURITY: + dom = "Security Labeling "; + break; } return(dom); } @@ -995,6 +998,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("Node device not found: %s"); break; + case VIR_ERR_NO_SECURITY_MODEL: + if (info == NULL) + errmsg = _("Security model not found"); + else + errmsg = _("Security model not found: %s"); + break; } return (errmsg); } diff --git a/src/xml.c b/src/xml.c index 9c27a10..d644641 100644 --- a/src/xml.c +++ b/src/xml.c @@ -77,6 +77,39 @@ virXPathString(virConnectPtr conn, }

/** + * virXPathStringLimit: + * @xpath: the XPath string to evaluate + * @maxlen: maximum length permittred string + * @ctxt: an XPath context + * + * Wrapper for virXPathString, which validates the length of the returned + * string. + * + * Returns a new string which must be deallocated by the caller or NULL if + * the evaluation failed. + */ +char * +virXPathStringLimit(virConnectPtr conn, + const char *xpath, + size_t maxlen, + xmlXPathContextPtr ctxt) +{ + char *tmp = virXPathString(conn, xpath, ctxt); + + if (tmp != NULL) { + if (strlen(tmp) >= maxlen) { + virXMLError(conn, VIR_ERR_INTERNAL_ERROR, + _("\'%s\' value longer than %Zd bytes in virXPathStringLimit()"), + xpath, maxlen); + return NULL; + } + } else + virXMLError(conn, VIR_ERR_INTERNAL_ERROR, + _("\'%s\' missing in virXPathStringLimit()"), xpath); + return tmp; +} + +/** * virXPathNumber: * @xpath: the XPath string to evaluate * @ctxt: an XPath context diff --git a/src/xml.h b/src/xml.h index da9d3b5..3102876 100644 --- a/src/xml.h +++ b/src/xml.h @@ -17,6 +17,10 @@ int virXPathBoolean (virConnectPtr conn, char * virXPathString (virConnectPtr conn, const char *xpath, xmlXPathContextPtr ctxt); +char * virXPathStringLimit(virConnectPtr conn, + const char *xpath, + size_t maxlen, + xmlXPathContextPtr ctxt); int virXPathNumber (virConnectPtr conn, const char *xpath, xmlXPathContextPtr ctxt, diff --git a/tests/daemon-conf b/tests/daemon-conf index 7a53eff..42f7d32 100755 --- a/tests/daemon-conf +++ b/tests/daemon-conf @@ -59,6 +59,9 @@ while :; do -e '/^libnuma: Warning: .sys not mounted or no numa system/d' \ err > k && mv k err

+ # Filter out this diagnostic, too. + sed '/^Initialized security driver/d' err > k && mv k err + printf '%s\n\n' "remoteReadConfigFile: $f: $param_name: $msg" > expected-err diff -u expected-err err || fail=1

Ok, I have added your patches and make syntax-check succeeds except it does not like po_check - --- po/POTFILES.in +++ po/POTFILES.in @@ -22,8 +22,6 @@ src/qemu_conf.c src/qemu_driver.c src/remote_internal.c - -src/security.c - -src/security_selinux.c src/storage_backend.c src/storage_backend_disk.c src/storage_backend_fs.c Makefile.maint: you have changed the set of files with translatable diagnostics; apply the above patch Since these files add translations, what do I need to do to get this to pass? -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkmbFqQACgkQrlYvE4MpobMw5gCgi7XlYLyd7Gol7WJq4MN/s1Ek SbsAoOg6k/6McRFcZnf6BGmohCByWTlS =Hcbu -----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jim Meyering wrote:

Daniel J Walsh <dwalsh@redhat.com> wrote:

[I removed the 1900+ lines of useless context]

...

Ok, I have added your patches and make syntax-check succeeds except it does not like

po_check --- po/POTFILES.in +++ po/POTFILES.in @@ -22,8 +22,6 @@ src/qemu_conf.c src/qemu_driver.c src/remote_internal.c -src/security.c -src/security_selinux.c src/storage_backend.c src/storage_backend_disk.c src/storage_backend_fs.c Makefile.maint: you have changed the set of files with translatable diagnostics; apply the above patch

Since these files add translations, what do I need to do to get this to pass?

That check passes for me, but I'm using git. Are you using CVS?

If so, have you cvs-added those two files? If you have not, that would explain it.

Otherwise, the check (which runs a script to search all version-controlled files) may need to be adjusted. Ok that is what I missed, I was not checking them in before running the test. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkmbINsACgkQrlYvE4MpobP4FgCffugZzfPrm/it38q0wVLsJRiN T6cAn1/Vdfmhoo2UF8hbaq2c9hH9/3qP =1iaw -----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Daniel P. Berrange wrote:

On Mon, Feb 23, 2009 at 05:46:36PM -0500, Daniel J Walsh wrote:

...

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Daniel P. Berrange wrote:

...

On Tue, Feb 17, 2009 at 04:52:08PM -0500, Daniel J Walsh wrote:

...

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Looks like qemu launched from libvirt wants to create pulseaudito files under /root/.pulse directory. Hmm, that sounds bad - it should not do this.

...

Seems strange, and we might want to consider changing the homedir for each qemu launched by libvirt.

/var/run/libvirt/qemu/DOMAIN

for example.

It seems qemu has to be able to write here or it blows up. What version of QEMU is this with - I think that needs to be fixed in QEMU

...

Will add selinux policy for now. I'd prefer not - AFAIK, QEMU should not be doing this - if PulseAudio is desired when running as root, then the admin should start it ahead of time, not have QEMU auto-spawn it. PA should only auto-spawn itself if running non-root in the desktop session IMHO.

Daniel

For some reason it is also trying to create /root/.kde directory and then link a socket to /tmp/ksocket-root.

Everything seems to be caused by sound.

I hacked out a libvirt that does not add the -esound qualifier to qemu and every thing works correctly in svirt with SELinux in enforcing mode.

Not really sure what the proper way to handle this? Should libvirt be execing qemu with the sound device if it is running as root? Will this work with the sound devices? What happens if libvirt is remote?

Configuring sound devices in QEMU when doing remote provisioning is pretty useless really. We need to tunnelling of audio stream from the QEMU instance to the client machine, over VNC / SPICE, or a parallel network audio transport.

I'm inclined to say we should set the SDL env variable to disable sound for instances run as root, and only use sound when launching the per-user unprivileged instances which are able to properly integrate with the sound daemon provided by the desktop session (ESD / PulseAudio / KDE)

Daniel How about a patch like this for now, took some liberties with your quote?

git diff origin qemu_conf.c diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 6f58ee8..32cdba2 100644 - --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1327,8 +1327,13 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG_LIT("-full-screen"); } - - /* Add sound hardware */ - - if (vm->def->nsounds) { + /* Add sound hardware iff you are not running as root */ + /* Configuring sound devices in QEMU when doing remote provisioning is + pretty useless really. We need to tunnelling of audio stream from + the QEMU instance to the client machine, over VNC / SPICE, or a + parallel network audio transport. */ + + if (getuid() && vm->def->nsounds) { int size = 100; char *modstr; if (VIR_ALLOC_N(modstr, size+1) < 0) -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkmkBEYACgkQrlYvE4MpobPD4QCeP/VNSkebMd7b86t+n8fx+T+s +w8AoJGkLnrd1hLUxFU/6o9zRSv2WQUE =4CAZ -----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Daniel P. Berrange wrote:

Just spotted one serious problem we need to address. The method 'qemudStartVMDaemon' quoted here is where we set the security label:

On Tue, Feb 17, 2009 at 11:20:17AM -0500, Daniel J Walsh wrote:

...

@@ -1178,6 +1237,16 @@ static int qemudStartVMDaemon(virConnect return -1; }

+ /* + * Set up the security label for the domain here, before doing + * too much else. + */ + if (qemudDomainSetSecurityLabel(conn, driver, vm) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to set security label")); + return -1; + } + if (qemudExtractVersionInfo(emulator, NULL, &qemuCmdFlags) < 0) {

Which ultimately calls the following method, which sets the context to use for the next exec call.

...

+static int +SELinuxSecurityDomainSetSecurityLabel(virConnectPtr conn, + virSecurityDriverPtr drv, + const virSecurityLabelDefPtr secdef) +{ + /* TODO: verify DOI */ + + if (!STREQ(drv->name, secdef->model)) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("%s: security label driver mismatch: " + "\'%s\' model configured for domain, but " + "hypervisor driver is \'%s\'."), + __func__, secdef->model, drv->name); + return -1; + } + + if (setexeccon(secdef->label) == -1) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("%s: unable to set security context " + "'\%s\': %s."), __func__, secdef->label, + strerror(errno)); + return -1; + } + return 0; +}

The problem is that between the point where we set the exec context, and the place where QEMU is finally exec'd, there is scope for several other programs to be exec'd. Also there are other threads running concurrently, and I'm under whether 'setexeccon' scope is per thread or per process.

I think we need to move place where we set the exec context to after the fork() call, ideally to be the very last call made before the actual execve().

We do not currently have an easy way todo this, but I have the exact same problem in my patches to integrate with cgroups - I need to add the new PID to the appropriate cgroup immediately before exec'ing. So i suggest the following patch whichs a generic callback to the virExec() call, so we can implant the neccessary logic after the fork() and just before the real execve(), and safely in the child process.

To use this, we'd make qemudStartVM() pass in a virExecHook callback which does the call to qemudDomainSetSecurityLabel()

Daniel

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -291,6 +291,7 @@ virEnumToString; virEventAddHandle; virEventRemoveHandle; virExec; +virExecWithHook; virSetNonBlock; virFormatMacAddr; virGetHostname; diff --git a/src/util.c b/src/util.c --- a/src/util.c +++ b/src/util.c @@ -199,7 +199,10 @@ __virExec(virConnectPtr conn, const fd_set *keepfd, pid_t *retpid, int infd, int *outfd, int *errfd, - int flags) { + int flags, + virExecHook hook, + void *data) +{ pid_t pid; int null, i, openmax; int pipeout[2] = {-1,-1}; @@ -411,6 +414,9 @@ __virExec(virConnectPtr conn, childerr != childout) close(childerr);

+ if (hook) + (hook)(data); + if (envp) execve(argv[0], (char **) argv, (char**)envp); else @@ -445,13 +451,16 @@ __virExec(virConnectPtr conn, }

int -virExec(virConnectPtr conn, - const char *const*argv, - const char *const*envp, - const fd_set *keepfd, - pid_t *retpid, - int infd, int *outfd, int *errfd, - int flags) { +virExecWithHook(virConnectPtr conn, + const char *const*argv, + const char *const*envp, + const fd_set *keepfd, + pid_t *retpid, + int infd, int *outfd, int *errfd, + int flags, + virExecHook hook, + void *data) +{ char *argv_str;

if ((argv_str = virArgvToString(argv)) == NULL) { @@ -462,7 +471,21 @@ virExec(virConnectPtr conn, VIR_FREE(argv_str);

return __virExec(conn, argv, envp, keepfd, retpid, infd, outfd, errfd, - flags); + flags, hook, data); +} + +int +virExec(virConnectPtr conn, + const char *const*argv, + const char *const*envp, + const fd_set *keepfd, + pid_t *retpid, + int infd, int *outfd, int *errfd, + int flags) +{ + return virExecWithHook(conn, argv, envp, keepfd, retpid, + infd, outfd, errfd, + flags, NULL, NULL); }

static int @@ -580,7 +603,7 @@ virRun(virConnectPtr conn,

if ((execret = __virExec(conn, argv, NULL, NULL, &childpid, -1, &outfd, &errfd, - VIR_EXEC_NONE)) < 0) { + VIR_EXEC_NONE, NULL, NULL)) < 0) { ret = execret; goto error; } diff --git a/src/util.h b/src/util.h --- a/src/util.h +++ b/src/util.h @@ -40,6 +40,21 @@ enum {

int virSetNonBlock(int fd);

+/* This will execute in the context of the first child + * immediately after fork() */ +typedef int (*virExecHook)(void *data); + +int virExecWithHook(virConnectPtr conn, + const char *const*argv, + const char *const*envp, + const fd_set *keepfd, + int *retpid, + int infd, + int *outfd, + int *errfd, + int flags, + virExecHook hook, + void *data); int virExec(virConnectPtr conn, const char *const*argv, const char *const*envp,

I have an update to the original patch which includes a test program and a changelog, but I guess I need to wait for this patch to be approved. http://people.fedoraproject.org/~dwalsh/SELinux/svirt.patch -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkmezMkACgkQrlYvE4MpobMM1ACg1TYPE0OyLzPHAohvx0LRva4Z wXkAoLUHS+yJMx4A0C/xz7tVs2Np3NLL =uu9y -----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jim Meyering wrote:

Daniel P. Berrange wrote:

...

Just spotted one serious problem we need to address. The method 'qemudStartVMDaemon' quoted here is where we set the security label: ...

Good catch.

...

To use this, we'd make qemudStartVM() pass in a virExecHook callback which does the call to qemudDomainSetSecurityLabel()

Daniel

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -291,6 +291,7 @@ virEnumToString; virEventAddHandle; virEventRemoveHandle; virExec; +virExecWithHook;

Looks right to me. ACK. This patch looks good to me and I have already rebased the svirt patch to match, waiting for this to get applied, then I will resubmit my patch. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkmiqhYACgkQrlYvE4MpobMkFACeMWEmQfwKMe4Cn7NNikPk9f3+ zlMAoK7DeqbpTKho4Kw/bZtuA86vtMJK =eVSb -----END PGP SIGNATURE-----

On Fri, Feb 27, 2009 at 03:37:55PM -0500, Daniel J Walsh wrote:

...

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Another patch off latest repository.

This patch does not require the XML to include a label, although this is still supported.

Implemented most of the comments from Jim. make check and make syntax-check passes, Added seclabeltest.c to run in tests, Updated capability.rng, although not really sure I did it right.

This patch will generate random MCS Labels and relabels the image files to match. Seems to work well on F11.

I had a few problems with label generation on my F11 machine - perhaps you have a newer version of the patch than the one I applied.

I found I need the following additional patch..

- Make domainGenSecurityLabel() give diagnostics for each type of error instead of using generic error message in caller - Change logic bug 'c1 == c2' to 'c1 < c2' - Change 'c%d,c%d' to 'c%d.c%d' - it doesn't like labels with the form "c210,c502" only wanting "c210.c502" This does not make sense. c210,c502 is valid. c210.c502 means include

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Daniel P. Berrange wrote: the range. c210, c211, c212...c502.

- Fix use of STREQ - no need for == 0 in there I am reworking this code to use INT instead of strings. - Use VIR_FREE/VIR_ALLOC for memory mgmt

With this I can successfully start several VMs, and see them all using different contexts, and see the files labelled

# ps -xZ | grep qemu | awk '{print $1}' system_u:system_r:qemu_t:s0:c35.c537 system_u:system_r:qemu_t:s0:c210.c502

# ls -Zl /var/lib/libvirt/images/ total 504 -rwxr-xr-x. 1 system_u:object_r:virt_image_t:s0:c210.c502 root root 1073741824 2009-03-03 12:15 demo2.img -rwxr-xr-x. 1 system_u:object_r:virt_image_t:s0:c35.c537 root root 1073741824 2009-03-03 11:49 demo.img

Daniel

Index: src/qemu_driver.c =================================================================== RCS file: /data/cvs/libvirt/src/qemu_driver.c,v retrieving revision 1.212 diff -u -p -r1.212 qemu_driver.c --- src/qemu_driver.c 3 Mar 2009 12:03:44 -0000 1.212 +++ src/qemu_driver.c 3 Mar 2009 12:25:47 -0000 @@ -1316,13 +1316,11 @@ static int qemudStartVMDaemon(virConnect

/* If you are using a SecurityDriver and there was no security label in database, then generate a security label for isolation */ - if (vm->def->seclabel.label == NULL && driver->securityDriver) { - if (driver->securityDriver->domainGenSecurityLabel(vm) < 0) { - qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to generate Security Label")); - return -1; - } - } + if (vm->def->seclabel.label == NULL && + driver->securityDriver && + driver->securityDriver->domainGenSecurityLabel && + driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0) + return -1;

FD_ZERO(&keepfd);

Index: src/security.h =================================================================== RCS file: /data/cvs/libvirt/src/security.h,v retrieving revision 1.1 diff -u -p -r1.1 security.h --- src/security.h 3 Mar 2009 09:44:42 -0000 1.1 +++ src/security.h 3 Mar 2009 12:25:47 -0000 @@ -37,7 +37,8 @@ typedef int (*virSecurityDomainRestoreIm typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn, virDomainObjPtr vm, virDomainDeviceDefPtr dev); -typedef int (*virSecurityDomainGenLabel) (virDomainObjPtr sec); +typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn, + virDomainObjPtr sec); typedef int (*virSecurityDomainGetLabel) (virConnectPtr conn, virDomainObjPtr vm, virSecurityLabelPtr sec); Index: src/security_selinux.c =================================================================== RCS file: /data/cvs/libvirt/src/security_selinux.c,v retrieving revision 1.1 diff -u -p -r1.1 security_selinux.c --- src/security_selinux.c 3 Mar 2009 10:06:49 -0000 1.1 +++ src/security_selinux.c 3 Mar 2009 12:25:47 -0000 @@ -24,6 +24,9 @@ #include "util.h" #include "memory.h"

+ +#define VIR_FROM_THIS VIR_FROM_SECURITY + static char default_domain_context[1024]; static char default_image_context[1024]; #define SECURITY_SELINUX_VOID_DOI "0" @@ -45,10 +48,11 @@ mcsAdd(const char *mcs) struct MCS *ptr;

for (ptr = mcsList; ptr; ptr = ptr->next) { - if (STREQ(ptr->mcs, mcs) == 0) + if (STREQ(ptr->mcs, mcs)) return -1; } - ptr = malloc(sizeof(struct MCS)); + if (VIR_ALLOC(ptr) < 0) + return -1; ptr->mcs = strdup(mcs); ptr->next = mcsList; mcsList = ptr; @@ -62,7 +66,7 @@ mcsRemove(const char *mcs) struct MCS *ptr = NULL;

for (ptr = mcsList; ptr; ptr = ptr->next) { - if (STREQ(ptr->mcs, mcs) == 0) { + if (STREQ(ptr->mcs, mcs)) { if (prevptr) prevptr->next = ptr->next; else { @@ -149,7 +153,8 @@ SELinuxInitialize(virConnectPtr conn) }

static int -SELinuxGenSecurityLabel(virDomainObjPtr vm) +SELinuxGenSecurityLabel(virConnectPtr conn, + virDomainObjPtr vm) { int rc = -1; char mcs[1024]; @@ -158,8 +163,11 @@ SELinuxGenSecurityLabel(virDomainObjPtr int c2 = 0; if ( ( vm->def->seclabel.label ) || ( vm->def->seclabel.model ) || - ( vm->def->seclabel.imagelabel )) + ( vm->def->seclabel.imagelabel )) { + virSecurityReportError(conn, VIR_ERR_ERROR, + "%s", _("security labellin already defined for VM")); return rc; + }

do { c1 = virRandom(1024); @@ -168,28 +176,40 @@ SELinuxGenSecurityLabel(virDomainObjPtr if ( c1 == c2 ) { sprintf(mcs, "s0:c%d", c1); } else { - if ( c1 == c2 ) - sprintf(mcs, "s0:c%d,c%d", c1, c2); + if ( c1 < c2 ) + sprintf(mcs, "s0:c%d.c%d", c1, c2); else - sprintf(mcs, "s0:c%d,c%d", c2, c1); + sprintf(mcs, "s0:c%d.c%d", c2, c1); } } while(mcsAdd(mcs) == -1);

vm->def->seclabel.label = SELinuxGenNewContext(default_domain_context, mcs); - if (! vm->def->seclabel.label) goto err; + if (! vm->def->seclabel.label) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("cannot generate selinux context for %s"), mcs); + goto err; + } vm->def->seclabel.imagelabel = SELinuxGenNewContext(default_image_context, mcs); - if (! vm->def->seclabel.imagelabel) goto err; + if (! vm->def->seclabel.imagelabel) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("cannot generate selinux context for %s"), mcs); + goto err; + } vm->def->seclabel.model = strdup(SECURITY_SELINUX_NAME); - if (! vm->def->seclabel.model) goto err; + if (! vm->def->seclabel.model) { + virReportOOMError(conn); + goto err; + } +

rc = 0; goto done; err: - free(vm->def->seclabel.label); vm->def->seclabel.label = NULL; - free(vm->def->seclabel.imagelabel); vm->def->seclabel.imagelabel = NULL; - free(vm->def->seclabel.model); vm->def->seclabel.model = NULL; + VIR_FREE(vm->def->seclabel.label); + VIR_FREE(vm->def->seclabel.imagelabel); + VIR_FREE(vm->def->seclabel.model); done: - free(scontext); + VIR_FREE(scontext); return rc; }

-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEUEARECAAYFAkmtOdQACgkQrlYvE4MpobMdwQCfQR3lSPnih5zd977k/wET4WqD rhIAmNYlqoogrM4KFb/trH4n5lxU2fc= =Bhc/ -----END PGP SIGNATURE-----

On Fri, Feb 20, 2009 at 02:38:05PM +0000, Daniel P. Berrange wrote:

I think we need to move place where we set the exec context to after the fork() call, ideally to be the very last call made before the actual execve().

We do not currently have an easy way todo this, but I have the exact same problem in my patches to integrate with cgroups - I need to add the new PID to the appropriate cgroup immediately before exec'ing. So i suggest the following patch whichs a generic callback to the virExec() call, so we can implant the neccessary logic after the fork() and just before the real execve(), and safely in the child process.

To use this, we'd make qemudStartVM() pass in a virExecHook callback which does the call to qemudDomainSetSecurityLabel()

I've committed this patch to CVS now.

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -291,6 +291,7 @@ virEnumToString; virEventAddHandle; virEventRemoveHandle; virExec; +virExecWithHook; virSetNonBlock; virFormatMacAddr; virGetHostname; diff --git a/src/util.c b/src/util.c --- a/src/util.c +++ b/src/util.c @@ -199,7 +199,10 @@ __virExec(virConnectPtr conn, const fd_set *keepfd, pid_t *retpid, int infd, int *outfd, int *errfd, - int flags) { + int flags, + virExecHook hook, + void *data) +{ pid_t pid; int null, i, openmax; int pipeout[2] = {-1,-1}; @@ -411,6 +414,9 @@ __virExec(virConnectPtr conn, childerr != childout) close(childerr);

+ if (hook) + (hook)(data); + if (envp) execve(argv[0], (char **) argv, (char**)envp); else @@ -445,13 +451,16 @@ __virExec(virConnectPtr conn, }

int -virExec(virConnectPtr conn, - const char *const*argv, - const char *const*envp, - const fd_set *keepfd, - pid_t *retpid, - int infd, int *outfd, int *errfd, - int flags) { +virExecWithHook(virConnectPtr conn, + const char *const*argv, + const char *const*envp, + const fd_set *keepfd, + pid_t *retpid, + int infd, int *outfd, int *errfd, + int flags, + virExecHook hook, + void *data) +{ char *argv_str;

if ((argv_str = virArgvToString(argv)) == NULL) { @@ -462,7 +471,21 @@ virExec(virConnectPtr conn, VIR_FREE(argv_str);

return __virExec(conn, argv, envp, keepfd, retpid, infd, outfd, errfd, - flags); + flags, hook, data); +} + +int +virExec(virConnectPtr conn, + const char *const*argv, + const char *const*envp, + const fd_set *keepfd, + pid_t *retpid, + int infd, int *outfd, int *errfd, + int flags) +{ + return virExecWithHook(conn, argv, envp, keepfd, retpid, + infd, outfd, errfd, + flags, NULL, NULL); }

static int @@ -580,7 +603,7 @@ virRun(virConnectPtr conn,

if ((execret = __virExec(conn, argv, NULL, NULL, &childpid, -1, &outfd, &errfd, - VIR_EXEC_NONE)) < 0) { + VIR_EXEC_NONE, NULL, NULL)) < 0) { ret = execret; goto error; } diff --git a/src/util.h b/src/util.h --- a/src/util.h +++ b/src/util.h @@ -40,6 +40,21 @@ enum {

int virSetNonBlock(int fd);

+/* This will execute in the context of the first child + * immediately after fork() */ +typedef int (*virExecHook)(void *data); + +int virExecWithHook(virConnectPtr conn, + const char *const*argv, + const char *const*envp, + const fd_set *keepfd, + int *retpid, + int infd, + int *outfd, + int *errfd, + int flags, + virExecHook hook, + void *data); int virExec(virConnectPtr conn, const char *const*argv, const char *const*envp,

Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|