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
