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@redhat.com> 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@redhat.com> --- 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