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

ACK

Rich.

> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 83326dbaa9..3b5abcc12b 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -164,6 +164,7 @@ EXTRA_DIST = \
>  	xlconfigdata \
>  	xmconfigdata \
>  	xml2vmxdata \
> +	virsh-auth.xml \
>  	virstorageutildata \
>  	virfilecachedata \
>  	virresctrldata \
> @@ -406,6 +407,7 @@ test_scripts =
>  libvirtd_test_scripts = \
>  	libvirtd-fail \
>  	libvirtd-pool \
> +	virsh-auth \
>  	virsh-cpuset \
>  	virsh-define-dev-segfault \
>  	virsh-int-overflow \
> diff --git a/tests/virsh-auth b/tests/virsh-auth
> new file mode 100755
> index 0000000000..d548694190
> --- /dev/null
> +++ b/tests/virsh-auth
> @@ -0,0 +1,57 @@
> +#!/usr/bin/env python3
> +# run virsh to validate interactive auth
> +
> +# Copyright (C) 2020 Red Hat, Inc.
> +
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 2 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see
> +# <http://www.gnu.org/licenses/>.
> +
> +import os
> +import os.path
> +import sys
> +import subprocess
> +
> +builddir = os.getenv("abs_top_builddir")
> +if builddir is None:
> +   builddir = os.path.join(os.getcwd(), "..")
> +
> +srcdir = os.getenv("abs_top_srcdir")
> +if srcdir is None:
> +   srcdir = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
> +
> +uri = "test://" + os.path.join(srcdir, "tests", "virsh-auth.xml")
> +
> +virsh = os.path.join(builddir, "tools", "virsh")
> +
> +proc = subprocess.Popen([virsh, "-c", uri, "uri"],
> +                        universal_newlines=True,
> +                        start_new_session=True,
> +                        stdin=subprocess.PIPE,
> +                        stdout=subprocess.PIPE,
> +                        stderr=subprocess.PIPE)
> +out, err = proc.communicate("astrochicken")
> +
> +if proc.returncode != 0:
> +   print("virsh failed with code %d" % proc.returncode, file=sys.stderr)
> +   if out != "":
> +      print("stdout=%s" % out)
> +   if err != "":
> +      print("stderr=%s" % err)
> +   sys.exit(1)
> +
> +if uri not in out:
> +   print("Expected '%s' in '%s'" % (uri, out), file=sys.stderr)
> +   sys.exit(1)
> +
> +sys.exit(0)
> diff --git a/tests/virsh-auth.xml b/tests/virsh-auth.xml
> new file mode 100644
> index 0000000000..a045a0746e
> --- /dev/null
> +++ b/tests/virsh-auth.xml
> @@ -0,0 +1,5 @@
> +<node>
> +  <auth>
> +    <user>astrochicken</user>
> +  </auth>
> +</node>
> -- 
> 2.24.1

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html





[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