[libvirt] [PATCH] rpc: fix handling of SSH auth failure code
The result of libssh2_userauth_password is being assigned to 'ret' in one branch and 'rc' in the other branch. Checks are all done against the 'ret' variable, so one branch never does the correct check. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnetsshsession.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 35dc6c5356..54c3a6f7aa 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -706,9 +706,9 @@ virNetSSHAuthenticatePassword(virNetSSHSessionPtr sess, if (priv->password) { /* tunelled password authentication */ - if ((ret = libssh2_userauth_password(sess->session, - priv->username, - priv->password)) == 0) { + if ((rc = libssh2_userauth_password(sess->session, + priv->username, + priv->password)) == 0) { ret = 0; goto cleanup; } @@ -737,7 +737,7 @@ virNetSSHAuthenticatePassword(virNetSSHSessionPtr sess, goto cleanup; } - if (ret != LIBSSH2_ERROR_AUTHENTICATION_FAILED) + if (rc != LIBSSH2_ERROR_AUTHENTICATION_FAILED) break; VIR_FREE(password); @@ -750,7 +750,7 @@ virNetSSHAuthenticatePassword(virNetSSHSessionPtr sess, _("authentication failed: %s"), errmsg); /* determine exist status */ - if (ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) + if (rc == LIBSSH2_ERROR_AUTHENTICATION_FAILED) return 1; else return -1; -- 2.17.1
On Fri, 2018-08-31 at 11:15 +0100, Daniel P. Berrangé wrote:
The result of libssh2_userauth_password is being assigned to 'ret' in one branch and 'rc' in the other branch. Checks are all done against the 'ret' variable, so one branch never does the correct check.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnetsshsession.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
The fix itself is correct, so: Reviewed-by: Andrea Bolognani <abologna@redhat.com> However, see below.
/* determine exist status */ - if (ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) + if (rc == LIBSSH2_ERROR_AUTHENTICATION_FAILED) return 1; else return -1;
Since we have a cleanup label right after this, it would make sense to change both 'return' to 'ret = ': that would ensure the function has a single return and also prevent 'password' from leaking when an error which is not AUTHENTICATION_FAILED is returned by libssh2_userauth_password(). -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Aug 31, 2018 at 04:16:45PM +0200, Andrea Bolognani wrote:
On Fri, 2018-08-31 at 11:15 +0100, Daniel P. Berrangé wrote:
The result of libssh2_userauth_password is being assigned to 'ret' in one branch and 'rc' in the other branch. Checks are all done against the 'ret' variable, so one branch never does the correct check.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnetsshsession.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
The fix itself is correct, so:
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
However, see below.
/* determine exist status */ - if (ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) + if (rc == LIBSSH2_ERROR_AUTHENTICATION_FAILED) return 1; else return -1;
Since we have a cleanup label right after this, it would make sense to change both 'return' to 'ret = ': that would ensure the function has a single return and also prevent 'password' from leaking when an error which is not AUTHENTICATION_FAILED is returned by libssh2_userauth_password().
Yes, makes sense. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Andrea Bolognani -
Daniel P. Berrangé