Re: [libvirt PATCH] src: fix mixup of stack and heap allocated data in auth callback

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 05, 2020 at 06:25:20PM +0000, Richard W.M. Jones wrote:
> On Thu, Mar 05, 2020 at 04:54:10PM +0000, Daniel P. Berrangé wrote:
> > In the following recent change:
> > 
> >   commit db72866310d1e520efa8ed2d4589bdb5e76a1c95
> >   Author: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> >   Date:   Tue Jan 14 10:40:52 2020 +0000
> > 
> >     util: add API for reading password from the console
> > 
> > the fact that "bufptr" pointer may point to either heap or stack
> > allocated data was overlooked. As a result, when the strdup was
> > removed, we ended up returning a pointer to the local stack to
> > the caller. When the caller referenced this stack pointer they
> > got out garbage which fairly quickly resulted in a crash.
> > 
> > Switching from fgets() to getline() lets to avoid the need for
> > the stack allocated buffer entirely, which makes both cases
> > of the switch consistent.
> > 
> > Test case addition is inspired by the libguestfs test which
> > caught this bug.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> >  src/libvirt.c        | 30 +++++++++++------------
> >  tests/Makefile.am    |  2 ++
> >  tests/virsh-auth     | 57 ++++++++++++++++++++++++++++++++++++++++++++
> >  tests/virsh-auth.xml |  5 ++++
> >  4 files changed, 79 insertions(+), 15 deletions(-)
> >  create mode 100755 tests/virsh-auth
> >  create mode 100644 tests/virsh-auth.xml
> > 
> > diff --git a/src/libvirt.c b/src/libvirt.c
> > index a30eaa7590..cc9c2eb5a2 100644
> > --- a/src/libvirt.c
> > +++ b/src/libvirt.c
> > @@ -110,9 +110,8 @@ virConnectAuthCallbackDefault(virConnectCredentialPtr cred,
> >      size_t i;
> >  
> >      for (i = 0; i < ncred; i++) {
> > -        char buf[1024];
> > -        char *bufptr = buf;
> > -        size_t len;
> > +        char *buf = NULL;
> > +        size_t len = 0;
> >  
> >          switch (cred[i].type) {
> >          case VIR_CRED_EXTERNAL: {
> > @@ -136,16 +135,17 @@ virConnectAuthCallbackDefault(virConnectCredentialPtr cred,
> >              if (fflush(stdout) != 0)
> >                  return -1;
> >  
> > -            if (!fgets(buf, sizeof(buf), stdin)) {
> > -                if (feof(stdin)) { /* Treat EOF as "" */
> > -                    buf[0] = '\0';
> > -                    break;
> > +            if (getline(&buf, &len, stdin) < 0) {
> > +                if (!feof(stdin)) {
> > +                    return -1;
> >                  }
> > -                return -1;
> > +                /* Use creddefault on EOF */
> > +                buf = NULL;
> > +            } else {
> > +                len = strlen(buf);
> > +                if (len != 0 && buf[len-1] == '\n')
> > +                    buf[len-1] = '\0';
> >              }
> > -            len = strlen(buf);
> > -            if (len != 0 && buf[len-1] == '\n')
> > -                buf[len-1] = '\0';
> >              break;
> >  
> >          case VIR_CRED_PASSPHRASE:
> > @@ -155,9 +155,9 @@ virConnectAuthCallbackDefault(virConnectCredentialPtr cred,
> >              if (fflush(stdout) != 0)
> >                  return -1;
> >  
> > -            bufptr = virGetPassword();
> > -            if (STREQ(bufptr, ""))
> > -                VIR_FREE(bufptr);
> > +            buf = virGetPassword();
> > +            if (STREQ(buf, ""))
> > +                VIR_FREE(buf);
> >              break;
> >  
> >          default:
> > @@ -165,7 +165,7 @@ virConnectAuthCallbackDefault(virConnectCredentialPtr cred,
> >          }
> >  
> >          if (cred[i].type != VIR_CRED_EXTERNAL) {
> > -            cred[i].result = bufptr ? bufptr : g_strdup(cred[i].defresult ? cred[i].defresult : "");
> > +            cred[i].result = buf ? buf : g_strdup(cred[i].defresult ? cred[i].defresult : "");
> >              cred[i].resultlen = strlen(cred[i].result);
> >          }
> >      }
> 
> It's clearly better to use getline here instead of fgets and
> the large fixed-size stack buffer, so

Damn, doesn't exist on Mingw, so I'll have to rework to keep
fgets and fix it differently.


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 :|





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux