Re: [Libvirt-cim] [PATCH] provide a hack to borrow non-root ssh keys in migration

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(a)linux.vnet.ibm.com&gt; > --- > 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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ _______________________________________________ Libvirt-cim mailing list Libvirt-cim(a)redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim