[Libvirt-cim] [PATCH] provide a hack to borrow non-root ssh keys in migration
Daniel Veillard
veillard at redhat.com
Fri Sep 14 08:48:23 UTC 2012
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/
More information about the Libvirt-cim
mailing list