8:26 a.m.
See 4/4 for the list and explanation.
Michal Prívozník (4):
qemu_shim: Replace g_file_get_contents() with virFileReadAll()
virutil: Do not use g_get_host_name() to obtain hostname
syntax-check: Update list of gethostname exceptions
docs: Discourage use of some glib functions
build-aux/syntax-check.mk | 2 +-
docs/glib-adoption.rst | 17 +++++++++++++++++
src/qemu/qemu_shim.c | 18 +++++++++++-------
src/util/virutil.c | 12 +++++++++---
4 files changed, 38 insertions(+), 11 deletions(-)
--
2.26.2
8:26 a.m.
New subject: [PATCH 1/4] qemu_shim: Replace g_file_get_contents() with virFileReadAll()
The qemu_shim (compiled into virt-qemu-run-binary) reads several
files provided by user (XML definition of secret, value of the
secret, XML definition of domain) and it does so using
g_file_get_contents(). This is potentially dangerous, because
there is no limit on the size of files/buffers.
Since this is a standalone binary it's not critical as it can't
cause libvirtd to be OOM killed, but it's still worth fixing as I
am planning on discouraging people from using the GLib function.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_shim.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_shim.c b/src/qemu/qemu_shim.c
index c10598df4b..1b0c41a9d2 100644
--- a/src/qemu/qemu_shim.c
+++ b/src/qemu/qemu_shim.c
@@ -31,6 +31,10 @@
#define VIR_FROM_THIS VIR_FROM_QEMU
+/* Below, several files are read into buffers. This defines the
+ * maximum possible size for such buffer. 10MiB should be enough. */
+#define MAX_FILE_SIZE (10 * 1024 * 1024)
+
static GMutex eventLock;
static bool eventPreventQuitFlag;
static bool eventQuitFlag;
@@ -264,7 +268,7 @@ int main(int argc, char **argv)
g_autofree char *sxml = NULL;
g_autofree char *value = NULL;
virSecretPtr sec;
- size_t nvalue;
+ int nvalue;
if (!bits || bits[0] == NULL || bits[1] == NULL) {
g_printerr("%s: expected a pair of filenames for --secret
argument\n",
@@ -276,15 +280,15 @@ int main(int argc, char **argv)
g_printerr("%s: %lld: loading secret %s and %s\n",
argv[0], deltams(), bits[0], bits[1]);
- if (!g_file_get_contents(bits[0], &sxml, NULL, &error)) {
+ if (virFileReadAll(bits[0], MAX_FILE_SIZE, &sxml) < 0) {
g_printerr("%s: cannot read secret XML %s: %s\n",
- argv[0], bits[0], error->message);
+ argv[0], bits[0], virGetLastErrorMessage());
goto cleanup;
}
- if (!g_file_get_contents(bits[1], &value, &nvalue, &error)) {
+ if ((nvalue = virFileReadAll(bits[1], MAX_FILE_SIZE, &value)) < 0) {
g_printerr("%s: cannot read secret value %s: %s\n",
- argv[0], bits[1], error->message);
+ argv[0], bits[1], virGetLastErrorMessage());
goto cleanup;
}
@@ -332,9 +336,9 @@ int main(int argc, char **argv)
g_printerr("%s: %lld: fetching guest config %s\n",
argv[0], deltams(), argv[1]);
- if (!g_file_get_contents(argv[1], &xml, NULL, &error)) {
+ if (virFileReadAll(argv[1], MAX_FILE_SIZE, &xml) < 0) {
g_printerr("%s: cannot read %s: %s\n",
- argv[0], argv[1], error->message);
+ argv[0], argv[1], virGetLastErrorMessage());
goto cleanup;
}
--
2.26.2
8:56 a.m.
New subject: [PATCH 1/4] qemu_shim: Replace g_file_get_contents() with virFileReadAll()
On Tue, Mar 09, 2021 at 03:26:22PM +0100, Michal Privoznik wrote:
The qemu_shim (compiled into virt-qemu-run-binary) reads several
files provided by user (XML definition of secret, value of the
secret, XML definition of domain) and it does so using
g_file_get_contents(). This is potentially dangerous, because
there is no limit on the size of files/buffers.
Since this is a standalone binary it's not critical as it can't
cause libvirtd to be OOM killed, but it's still worth fixing as I
am planning on discouraging people from using the GLib function.
I'm not convinced actually. I think that almost all of the places
where we use virFileReadAll with a limit are in fact safe to use
with g_file_get_contents. Almost all the files are from trusted
sources - files exposed by the kernel, or files only writable
by root, or by the same user as libvirtd.
Almost no where does libvirt read files that are provided by an
untrusted user. Reading disk image headers, or QEMU save files
are the two main untrusted scenarios, and in both those cases
we wdon't actually want more than the header.
I think we probably get rid of virFileReadAll in the long term
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 :|
9:40 a.m.
New subject: [PATCH 1/4] qemu_shim: Replace g_file_get_contents() with virFileReadAll()
On 3/9/21 3:56 PM, Daniel P. Berrangé wrote:
On Tue, Mar 09, 2021 at 03:26:22PM +0100, Michal Privoznik wrote:
> The qemu_shim (compiled into virt-qemu-run-binary) reads several
> files provided by user (XML definition of secret, value of the
> secret, XML definition of domain) and it does so using
> g_file_get_contents(). This is potentially dangerous, because
> there is no limit on the size of files/buffers.
>
> Since this is a standalone binary it's not critical as it can't
> cause libvirtd to be OOM killed, but it's still worth fixing as I
> am planning on discouraging people from using the GLib function.
I'm not convinced actually. I think that almost all of the places
where we use virFileReadAll with a limit are in fact safe to use
with g_file_get_contents. Almost all the files are from trusted
sources - files exposed by the kernel, or files only writable
by root, or by the same user as libvirtd.
Almost no where does libvirt read files that are provided by an
untrusted user. Reading disk image headers, or QEMU save files
are the two main untrusted scenarios, and in both those cases
we wdon't actually want more than the header.
And virsh. But that again is standalone so probably no harm there.
But we also read TLS keys/certs, but those are safe - given that only
root should have access to them. My concern was that if we use
g_file_get_contents() somewhere, it may sneak into scenario where we
definitely don't want it. But maybe I'm worrying about nothing.
Michal
8:26 a.m.
New subject: [PATCH 2/4] virutil: Do not use g_get_host_name() to obtain hostname
The problem is that g_get_host_name() caches the hostname in a
thread local variable. Therefore, it doesn't reflect any
subsequent hostname changes. While this might be acceptable for
logs where the hostname is printed exactly once when the libvirtd
starts up, it is not optimal for virGetHostnameImpl() which is
what our public virConnectGetHostname() API calls. If the
hostname at the moment of the first API invocation happens to be
"localhost" or contains a dot, then no further hostname changes
will ever be reflected.
This reverts 26d9748ff11, partially.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virutil.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 227997c7be..118ceec0db 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -475,11 +475,17 @@ static char *
virGetHostnameImpl(bool quiet)
{
int r;
- const char *hostname;
- char *result = NULL;
+ char hostname[HOST_NAME_MAX+1], *result = NULL;
struct addrinfo hints, *info;
- hostname = g_get_host_name();
+ r = gethostname(hostname, sizeof(hostname));
+ if (r == -1) {
+ if (!quiet)
+ virReportSystemError(errno,
+ "%s", _("failed to determine host
name"));
+ return NULL;
+ }
+ NUL_TERMINATE(hostname);
if (STRPREFIX(hostname, "localhost") || strchr(hostname, '.')) {
/* in this case, gethostname returned localhost (meaning we can't
--
2.26.2
12:47 p.m.
New subject: [PATCH 2/4] virutil: Do not use g_get_host_name() to obtain hostname
On a Tuesday in 2021, Michal Privoznik wrote:
The problem is that g_get_host_name() caches the hostname in a
thread local variable. Therefore, it doesn't reflect any
subsequent hostname changes. While this might be acceptable for
logs where the hostname is printed exactly once when the libvirtd
starts up, it is not optimal for virGetHostnameImpl() which is
what our public virConnectGetHostname() API calls.
If the
hostname at the moment of the first API invocation happens to be
s/be/start with/
"localhost" or contains a dot, then no further hostname
changes
will ever be reflected.
This reverts 26d9748ff11, partially.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virutil.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:26 a.m.
New subject: [PATCH 3/4] syntax-check: Update list of gethostname exceptions
The only place where gethostname() is acceptable is in
virGetHostnameImpl() which lives in src/util/virutil.c.
Reflect this in the list of exceptions for the syntax-check rule.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
build-aux/syntax-check.mk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 2f4f932a5b..794ec326e4 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1910,7 +1910,7 @@ exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \
exclude_file_name_regexp--sc_prohibit_fork_wrappers = \
(^(src/(util/(vircommand|virdaemon)|lxc/lxc_controller)|tests/testutils)\.c$$)
-exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/vir(util|log)\.c$$
+exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/virutil\.c$$
exclude_file_name_regexp--sc_prohibit_internal_functions = \
^src/(util/(viralloc|virutil|virfile)\.[hc]|esx/esx_vi\.c)$$
--
2.26.2
12:47 p.m.
New subject: [PATCH 3/4] syntax-check: Update list of gethostname exceptions
On a Tuesday in 2021, Michal Privoznik wrote:
The only place where gethostname() is acceptable is in
virGetHostnameImpl() which lives in src/util/virutil.c.
Reflect this in the list of exceptions for the syntax-check rule.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
build-aux/syntax-check.mk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:26 a.m.
New subject: [PATCH 4/4] docs: Discourage use of some glib functions
Unfortunately, not all GLib functions provide the level of
security we want, or behave how we want. So far,
g_file_get_contents() and g_get_host_name() have been identified.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/glib-adoption.rst | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/docs/glib-adoption.rst b/docs/glib-adoption.rst
index f969ac80a1..bbc27fed6d 100644
--- a/docs/glib-adoption.rst
+++ b/docs/glib-adoption.rst
@@ -52,3 +52,20 @@ Objects
https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html
Prefer ``GObject`` instead.
+
+
+Forbidden functions
+===================
+
+There are some functions where Libvirt offers superior quality to GLib. In
+those cases, GLib functions must NOT be used and Libvirt functions should be
+used instead.
+
+``g_file_get_contents``
+ Use ``virFileReadAll`` instead. The GLib function reads the entire file
+ into the memory without possibility to provide any limit on the buffer
+ size.
+
+``g_get_host_name``
+ Prefer ``virGetHostname``. The GLib function caches the hostname and thus
+ does not reflect (possible) hostname changes.
--
2.26.2
1353
days inactive
1354
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Privoznik