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
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);
}
}
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
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
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@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
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 :|
© 2016 - 2024 Red Hat, Inc.