[PATCH 1/4] qemu_shim: Replace g_file_get_contents() with virFileReadAll()

Michal Privoznik mprivozn at redhat.com
Tue Mar 9 15:40:52 UTC 2021


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




More information about the libvir-list mailing list