[Libvirt-cim] [PATCH] provide a hack to borrow non-root ssh keys in migration
snmishra at linux.vnet.ibm.com
snmishra at linux.vnet.ibm.com
Fri Sep 14 20:12:48 UTC 2012
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 at 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 at 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 at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
> http://veillard.com/ | virtualization library http://libvirt.org/
>
> _______________________________________________
> Libvirt-cim mailing list
> Libvirt-cim at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim
More information about the Libvirt-cim
mailing list