Thanks DV for looking at the patch. Wayne, do you have any other solution? DV, Can you please push this patch - https://www.redhat.com/archives/libvirt-cim/2012-July/msg00007.html Wayne, Are there any other patches to be reviewed/pushed? Thanks Sharad Mishra IBM Quoting Daniel Veillard <veillard@redhat.com>:
On Fri, Aug 24, 2012 at 06:18:29PM +0800, Wenchao Xia wrote:
This patch allow libvirt-cim to use non-root's ssh key, avoid using root's key to avoid exposing root's ssh login. Because libvirt-cim runs in root mode so it is hard to satisfy some server security rules especially about root's ssh key exposing. This is a walk around to improve it.
This has serious security implications, so I think this really must be documented in a very clear way: - how do you allow another key - how do you tell which one to pick, where and who can set this up - there is a copy done, it's hard to tell from which soure to which destination based just on code review - does that work with SELinux turned on ?
As is it's very hard to review the code to give an ACK,
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_VSMigrationService.c | 96 +++++++++++++++++++++++++++++++++++++++-- 1 files changed, 92 insertions(+), 4 deletions(-)
diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c index 76e3d25..55442ee 100644 --- a/src/Virt_VSMigrationService.c +++ b/src/Virt_VSMigrationService.c @@ -150,6 +150,7 @@ static CMPIStatus get_migration_uri(CMPIInstance *msd,
static char *dest_uri(const char *cn, const char *dest, + const char *dest_params, uint16_t transport) { const char *prefix; @@ -157,6 +158,7 @@ static char *dest_uri(const char *cn, const char *param = ""; char *uri = NULL; int rc; + int param_labeled = 0;
if (STARTS_WITH(cn, "Xen")) prefix = "xen"; @@ -197,16 +199,75 @@ static char *dest_uri(const char *cn, goto out; }
- if (!STREQC(param, "")) + if (!STREQC(param, "")) { rc = asprintf(&uri, "%s/%s", uri, param); + param_labeled = 1; + }
- if (rc == -1) + if (rc == -1) { uri = NULL; + goto out; + }
+ if (dest_params) { + if (param_labeled == 0) { + rc = asprintf(&uri, "%s?%s", uri, dest_params); + } else { + rc = asprintf(&uri, "%s%s", uri, dest_params); + } + if (rc == -1) { + uri = NULL; + goto out; + } + } out: return uri; }
+/* libvirt need private key specified must be placed in a directory owned by + root, because libvirt-cim now runs as root. So here the key would be copied, + up layer need to delete that key after migration. This method could allow + libvirt-cim borrow a non-root ssh private key, instead of using root's private + key, avoid security risk. */ +static int ssh_key_cp(const char *src, const char *dest)
though the function is named ssh_key_cp it can be used to copy anything to anywhere. Moreover it runs popen on a built command, where are src and dest coming from, they aren't validated for sanity. what happen if dest is "/tmp ; rm-rf /" ???
+{ + char *cmd = NULL; + int rc; + int ret = 0; + FILE *stream = NULL; + char buf[256]; + + rc = asprintf(&cmd, "cp -f %s %s", src, dest); + if (rc == -1) { + cmd = NULL; + ret = -1; + goto out; + } + + CU_DEBUG("excuting system cmd [%s].", cmd); + stream = popen(cmd, "r");
the "popen" man page says The command argument is a pointer to a null-terminated string contain‐ ing a shell command line. This command is passed to /bin/sh using the -c flag; interpretation, if any, is performed by the shell.
Sorry, the reason of that patch is supposedly to increase security and to me it's actually opening a huge security hole there. if you need to copy a file, do it cleanly, and verify the arguments are sane.
NACK
+ if (stream == NULL) { + CU_DEBUG("Failed to open pipe to run command"); + ret = -2; + goto out; + } + usleep(10000); + + buf[255] = 0; + while (fgets(buf, sizeof(buf), stream) != NULL) { + CU_DEBUG("Exception got: %s.", buf); + ret = -3; + goto out; + } + + out: + if (stream != NULL) { + pclose(stream); + } + free(cmd); + return ret; +} + static CMPIStatus get_msd_values(const CMPIObjectPath *ref, const char *destination, const CMPIArgs *argsin, @@ -217,6 +278,14 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, CMPIInstance *msd; uint16_t uri_type; char *uri = NULL; + const char *dest_params = NULL; + const char *ssh_hack_src = NULL; + const char *ssh_hack_dest = NULL; + int ret; + + cu_get_str_arg(argsin, "DestinationHostParams", &dest_params); + cu_get_str_arg(argsin, "SSH_Key_Src", &ssh_hack_src); + cu_get_str_arg(argsin, "SSH_Key_Dest", &ssh_hack_dest);
s = get_msd(ref, argsin, &msd); if (s.rc != CMPI_RC_OK) @@ -230,7 +299,7 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, if (s.rc != CMPI_RC_OK) goto out;
- uri = dest_uri(CLASSNAME(ref), destination, uri_type); + uri = dest_uri(CLASSNAME(ref), destination, dest_params, uri_type); if (uri == NULL) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, @@ -238,6 +307,19 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, goto out; }
+ if ((ssh_hack_src) && (ssh_hack_dest)) { + CU_DEBUG("hacking ssh keys src %s, dest %s.", + ssh_hack_src, ssh_hack_dest); + ret = ssh_key_cp(ssh_hack_src, ssh_hack_dest); + if (ret < 0) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to copy ssh key files"); + goto out; + } + } + + CU_DEBUG("Migrate tring to connect remote host with uri %s.", uri); *conn = virConnectOpen(uri); if (*conn == NULL) { CU_DEBUG("Failed to connect to remote host (%s)", uri); @@ -1537,7 +1619,7 @@ static CMPIStatus migrate_vs_host(CMPIMethodMI *self, const char *dhost = NULL; CMPIObjectPath *system; const char *name = NULL; - + cu_get_str_arg(argsin, "DestinationHost", &dhost); cu_get_ref_arg(argsin, "ComputerSystem", &system);
@@ -1608,6 +1690,9 @@ static struct method_handler vsimth = { .handler = vs_migratable_host, .args = {{"ComputerSystem", CMPI_ref, false}, {"DestinationHost", CMPI_string, false}, + {"DestinationHostParams", CMPI_string, true}, + {"SSH_Key_Src", CMPI_string, true}, + {"SSH_Key_Dest", CMPI_string, true}, {"MigrationSettingData", CMPI_instance, true}, {"NewSystemSettingData", CMPI_instance, true}, {"NewResourceSettingData", CMPI_instanceA, true}, @@ -1632,6 +1717,9 @@ static struct method_handler mvsth = { .handler = migrate_vs_host, .args = {{"ComputerSystem", CMPI_ref, false}, {"DestinationHost", CMPI_string, false}, + {"DestinationHostParams", CMPI_string, true}, + {"SSH_Key_Src", CMPI_string, true}, + {"SSH_Key_Dest", CMPI_string, true}, {"MigrationSettingData", CMPI_instance, true}, {"NewSystemSettingData", CMPI_instance, true}, {"NewResourceSettingData", CMPI_instanceA, true}, -- 1.7.1
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim