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
