[libvirt] [PATCH] rpc: fix handling of SSH auth failure code

Daniel P. Berrangé posted 1 patch 5 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180831101535.3148-1-berrange@redhat.com
Test syntax-check passed
src/rpc/virnetsshsession.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
[libvirt] [PATCH] rpc: fix handling of SSH auth failure code
Posted by Daniel P. Berrangé 5 years, 6 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: fix handling of SSH auth failure code
Posted by Andrea Bolognani 5 years, 6 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: fix handling of SSH auth failure code
Posted by Daniel P. Berrangé 5 years, 6 months ago
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 :|

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