diff mbox

rpc: libssh: don't crash when known_hosts file path is not provided

Message ID 6062162.CBVnNAEPkv@thyrus.usersys.redhat.com
State New
Headers show

Commit Message

Pino Toscano Jan. 10, 2017, 3:08 p.m. UTC 
On Monday, 9 January 2017 15:16:46 CET Peter Krempa wrote:
> When connecting as root, the "hostsfile" variable would be NULL due to

> the code leading to this point. This would result into a crash when

> attempting to set the known hosts file path.


Almost: the issue happens generally when known_hosts does not exist.
Considering the following code in src/rpc/virnetclient.c (both
virNetClientNewLibSSH2 and virNetClientNewLibssh have it):

    if (confdir) {
        if (!knownHostsPath) {
            if (virFileExists(confdir)) {
                virBufferAsprintf(&buf, "%s/known_hosts", confdir);
                if (!(knownhosts = virBufferContentAndReset(&buf)))
                    goto no_memory;
            }
        } else {
            if (VIR_STRDUP(knownhosts, knownHostsPath) < 0)
                goto cleanup;
        }
    }

knownHostsPath is left NULL if known_hosts is not passed as parameter,
and either one of:
a) confdir is NULL: reading virGetUserConfigDirectory (actually,
   virGetXDGDirectory) this happens only when XDG_CONFIG_HOME is not
   set, and neither is HOME
b) confdir is not NULL, but it does not exist

Regarding (a), there is no other "fix" doable within virNetClient
itself, since all the ways to determine the user home failed (and thus
no path can be determined).

Regarding (b), IMHO virNetClient could drop the virFileExists check,
letting the libssh and libssh2 drivers to handle:
- libssh seems to handle a non-existing file fine, creating its
  directories and the file itself on its own when saving
- the libssh2 already checks whether the file exists, using it only in
  that case

> To avoid deviating from the approach taken in the libssh2 driver set the

> file to /dev/null so that all entries are discarded unless explicitly

> specified.


While this works (ssh_write_knownhost should handle /dev as existing
already), IMHO it's better to make the libssh driver behave more close
to the libssh2, i.e. setting knownHostsFile only when considered valid.
What about the attached patch instead?

-- 
Pino Toscano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Peter Krempa Jan. 10, 2017, 3:49 p.m. UTC | #1 
On Tue, Jan 10, 2017 at 16:08:03 +0100, Pino Toscano wrote:
> On Monday, 9 January 2017 15:16:46 CET Peter Krempa wrote:

> > When connecting as root, the "hostsfile" variable would be NULL due to

> > the code leading to this point. This would result into a crash when

> > attempting to set the known hosts file path.

> 

> Almost: the issue happens generally when known_hosts does not exist.

> Considering the following code in src/rpc/virnetclient.c (both

> virNetClientNewLibSSH2 and virNetClientNewLibssh have it):

> 

>     if (confdir) {

>         if (!knownHostsPath) {

>             if (virFileExists(confdir)) {

>                 virBufferAsprintf(&buf, "%s/known_hosts", confdir);

>                 if (!(knownhosts = virBufferContentAndReset(&buf)))

>                     goto no_memory;

>             }

>         } else {

>             if (VIR_STRDUP(knownhosts, knownHostsPath) < 0)

>                 goto cleanup;

>         }

>     }


There is one more bug here. If 'knownHostsPath' is provided it should be
set regardless of 'confdir' so the logic is broken there.

> 

> knownHostsPath is left NULL if known_hosts is not passed as parameter,

> and either one of:

> a) confdir is NULL: reading virGetUserConfigDirectory (actually,

>    virGetXDGDirectory) this happens only when XDG_CONFIG_HOME is not

>    set, and neither is HOME

> b) confdir is not NULL, but it does not exist


So this is the root problem here, with the logic above the code would
crash if 'confdir' would not be set if you specify the path manually. It
does not crash if you do it, so 'confdir' indeed is not NULL'

> Regarding (a), there is no other "fix" doable within virNetClient

> itself, since all the ways to determine the user home failed (and thus

> no path can be determined).


I agree.

> 

> Regarding (b), IMHO virNetClient could drop the virFileExists check,

> letting the libssh and libssh2 drivers to handle:

> - libssh seems to handle a non-existing file fine, creating its

>   directories and the file itself on its own when saving

> - the libssh2 already checks whether the file exists, using it only in

>   that case


That seems like a reasonable thing to do. Along with fixing the logic.

> 

> > To avoid deviating from the approach taken in the libssh2 driver set the

> > file to /dev/null so that all entries are discarded unless explicitly

> > specified.

> 

> While this works (ssh_write_knownhost should handle /dev as existing

> already), IMHO it's better to make the libssh driver behave more close

> to the libssh2, i.e. setting knownHostsFile only when considered valid.

> What about the attached patch instead?

> 

> -- 

> Pino Toscano


> From=20a97d1baeefd32990730f4ec32501bf5fdb7b675b Mon Sep 17 00:00:00 2001

> From: Pino Toscano <ptoscano@redhat.com>

> Date: Tue, 10 Jan 2017 16:04:03 +0100

> Subject: [PATCH] rpc: libssh: allow a NULL known_hosts file

> 

> Make sure that virNetLibsshSessionSetHostKeyVerification accepts a NULL

> value for the path to the known_hosts file:

> - call ssh_options_set(SSH_OPTIONS_KNOWNHOSTS) anyway, using /dev/null,

>   otherwise libssh will use its default path

> - do not call ssh_write_knownhost when no known hosts file was set

> 

> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1406457

> ---

>  src/rpc/virnetlibsshsession.c | 41 +++++++++++++++++++++++++++--------------

>  1 file changed, 27 insertions(+), 14 deletions(-)

> 

> diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c

> index 5de6629..3ffe158 100644

> --- a/src/rpc/virnetlibsshsession.c

> +++ b/src/rpc/virnetlibsshsession.c

> @@ -303,6 +303,10 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)

>          if (!keyhashstr)

>              return -1;

>  

> +        /* If a key mismatch happens, then surely there was an existing

> +         * known_host file (with the different key). */

> +        sa_assert(sess->knownHostsFile);


While this assertion is true, it's pointless to have it here.

> +

>          /* host key verification failed */

>          virReportError(VIR_ERR_AUTH_FAILED,

>                         _("!!! SSH HOST KEY VERIFICATION FAILED !!!: "

> @@ -382,14 +386,16 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)

>              VIR_FREE(askKey.result);

>          }

>  

> -        /* write the host key file */

> -        if (ssh_write_knownhost(sess->session) < 0) {

> -            errmsg = ssh_get_error(sess->session);

> -            virReportError(VIR_ERR_LIBSSH,

> -                           _("failed to write known_host file '%s': %s"),

> -                           sess->knownHostsFile,

> -                           errmsg);

> -            return -1;

> +        /* write the host key file, if specified */

> +        if (sess->knownHostsFile) {

> +            if (ssh_write_knownhost(sess->session) < 0) {

> +                errmsg = ssh_get_error(sess->session);

> +                virReportError(VIR_ERR_LIBSSH,

> +                               _("failed to write known_host file '%s': %s"),

> +                               sess->knownHostsFile,

> +                               errmsg);

> +                return -1;

> +            }

>          }

>          /* key was accepted and added */

>          return 0;

> @@ -1172,13 +1178,20 @@ virNetLibsshSessionSetHostKeyVerification(virNetLibsshSessionPtr sess,

>              goto error;

>      }

>  

> -    /* set the known hosts file */

> -    if (ssh_options_set(sess->session, SSH_OPTIONS_KNOWNHOSTS, hostsfile) < 0)

> -        goto error;

> +    /* set the known hosts file, if specified */

> +    if (hostsfile) {

> +        if (ssh_options_set(sess->session, SSH_OPTIONS_KNOWNHOSTS, hostsfile) < 0)

> +            goto error;

>  

> -    VIR_FREE(sess->knownHostsFile);

> -    if (VIR_STRDUP(sess->knownHostsFile, hostsfile) < 0)

> -        goto error;

> +        VIR_FREE(sess->knownHostsFile);

> +        if (VIR_STRDUP(sess->knownHostsFile, hostsfile) < 0)

> +            goto error;

> +    } else {

> +        /* libssh does not support trying no known_host file at all:

> +         * hence use /dev/null here, without storing it as file */

> +        if (ssh_options_set(sess->session, SSH_OPTIONS_KNOWNHOSTS, "/dev/null") < 0)

> +            goto error;

> +    }


ACK to the last two hunks.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox

Patch

From=20a97d1baeefd32990730f4ec32501bf5fdb7b675b Mon Sep 17 00:00:00 2001
From: Pino Toscano <ptoscano@redhat.com>
Date: Tue, 10 Jan 2017 16:04:03 +0100
Subject: [PATCH] rpc: libssh: allow a NULL known_hosts file

Make sure that virNetLibsshSessionSetHostKeyVerification accepts a NULL
value for the path to the known_hosts file:
- call ssh_options_set(SSH_OPTIONS_KNOWNHOSTS) anyway, using /dev/null,
  otherwise libssh will use its default path
- do not call ssh_write_knownhost when no known hosts file was set

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1406457
---
 src/rpc/virnetlibsshsession.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c
index 5de6629..3ffe158 100644
--- a/src/rpc/virnetlibsshsession.c
+++ b/src/rpc/virnetlibsshsession.c
@@ -303,6 +303,10 @@  virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
         if (!keyhashstr)
             return -1;
 
+        /* If a key mismatch happens, then surely there was an existing
+         * known_host file (with the different key). */
+        sa_assert(sess->knownHostsFile);
+
         /* host key verification failed */
         virReportError(VIR_ERR_AUTH_FAILED,
                        _("!!! SSH HOST KEY VERIFICATION FAILED !!!: "
@@ -382,14 +386,16 @@  virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
             VIR_FREE(askKey.result);
         }
 
-        /* write the host key file */
-        if (ssh_write_knownhost(sess->session) < 0) {
-            errmsg = ssh_get_error(sess->session);
-            virReportError(VIR_ERR_LIBSSH,
-                           _("failed to write known_host file '%s': %s"),
-                           sess->knownHostsFile,
-                           errmsg);
-            return -1;
+        /* write the host key file, if specified */
+        if (sess->knownHostsFile) {
+            if (ssh_write_knownhost(sess->session) < 0) {
+                errmsg = ssh_get_error(sess->session);
+                virReportError(VIR_ERR_LIBSSH,
+                               _("failed to write known_host file '%s': %s"),
+                               sess->knownHostsFile,
+                               errmsg);
+                return -1;
+            }
         }
         /* key was accepted and added */
         return 0;
@@ -1172,13 +1178,20 @@  virNetLibsshSessionSetHostKeyVerification(virNetLibsshSessionPtr sess,
             goto error;
     }
 
-    /* set the known hosts file */
-    if (ssh_options_set(sess->session, SSH_OPTIONS_KNOWNHOSTS, hostsfile) < 0)
-        goto error;
+    /* set the known hosts file, if specified */
+    if (hostsfile) {
+        if (ssh_options_set(sess->session, SSH_OPTIONS_KNOWNHOSTS, hostsfile) < 0)
+            goto error;
 
-    VIR_FREE(sess->knownHostsFile);
-    if (VIR_STRDUP(sess->knownHostsFile, hostsfile) < 0)
-        goto error;
+        VIR_FREE(sess->knownHostsFile);
+        if (VIR_STRDUP(sess->knownHostsFile, hostsfile) < 0)
+            goto error;
+    } else {
+        /* libssh does not support trying no known_host file at all:
+         * hence use /dev/null here, without storing it as file */
+        if (ssh_options_set(sess->session, SSH_OPTIONS_KNOWNHOSTS, "/dev/null") < 0)
+            goto error;
+    }
 
     virObjectUnlock(sess);
     return 0;
-- 
2.7.4